CE-Programming / toolchain

Toolchain and libraries for C/C++ programming on the TI-84+ CE calculator series
https://ce-programming.github.io/toolchain/index.html
GNU Lesser General Public License v3.0
505 stars 54 forks source link

FATDRVCE allows lowercase chars in filename #475

Open TheMiningTeamYT opened 8 months ago

TheMiningTeamYT commented 8 months ago

If you create a file/directory using the functions in FATDRVCE (specifically fat_Create), you can use lowercase characters in the name. This is illegal for short (non-LFN) filenames on FAT32 (or any other variant of FAT afaik). This results in files/directories that appear, and can be opened either using fat_OpenFile, or on Linux (and I presume any other Unix system), but that can not be opened on Windows, as they apparently don't exist.

This also means that filenames are case sensitive. If you attempt to open a file named file by opening FILE, it won't work if it was created by fat_Create.

This could be fixed by converting each character of the filename to uppercase using a toupper call before actually creating the file. I noticed poking around in the assembly (fatdrvce.asm) that you already have a function for ensuring filenames are legal before using them (util_get_fat_name). A few toupper calls in there should do it. I'd do it myself and make a pull request if I understood the assembly.

The code I encountered this bug can be found here. I entered a world, took a screenshot by pressing the window button, and then attempted to open the shots folder.

TheMiningTeamYT commented 8 months ago

I've also been experiencing random resets and filesystem corruption, but I'm not sure if that's the libraries fault or my fault. It might be because I'm not calling msd_Close in usb.c, but that's because with my 16GB SanDisk Cruzer Micro, msd_Close causes my calculator to freeze. I might create an issue for that, but I suspect the bug might be specific to this 1 flash drive, since my other spare flash drive that works with my calculator, a 128MB "Nexdisk" that might actually be 20 years old, doesn't encounter that bug. However, the Nexdisk is super slow, the Cruzer is not, and I don't have any other flash drives that work with my calculator, so it is what it is. :(

mateoconlechuga commented 8 months ago

I believe it should actually be an error if lowercase letters are used. Additionally, I think your code has an issue here: https://github.com/TheMiningTeamYT/TI84CE-Blocks-3D/blob/main/src/usb.h#L6-L8

Note that the callback types have to be specified before including the appropriate header, like in the example.

TheMiningTeamYT commented 8 months ago

Well, as of now lowercase names are completely allowed. Throwing an error if lowercase letters are used isn't the best idea IMO because typically (such as on DOS) lowercase letters are converted to uppercase for short file names, rather than being considered invalid entirely.

Also, thanks for letting me know about those defines. However, for what it's worth, fixing that didn't change the compiled code so far as I can tell.

mateoconlechuga commented 8 months ago

I'm trying to decide if it is better to specify what is on the drive though. Having a lowercase name that doesn't actually exist doesn't make much sense? Why do you think the name the user supplies should be modified?

TheMiningTeamYT commented 8 months ago

The reason is that it's the typical behavior of operating systems dealing with FAT32 short names. Even operating systems that support LFN names will convert the filename to uppercase (and truncate it to 8.3), then add an LFN entry for the full lowercase filename.

mikaey commented 8 months ago

Also -- to quote the FAT specification -- "Lower case characters are not allowed."

And -- "Short file names passed to the file system are always converted to upper case and their original case value is lost."

https://academy.cba.mit.edu/classes/networking_communications/SD/FAT.pdf, section 6.1, pages 24-25

TheMiningTeamYT commented 8 months ago

So I've gotten some sleep and time to think about this and I realize I wasn’t being very clear with my reasoning or doing a very good job understanding your argument.

Lowercase letters are not allowed in FAT short (8.3) filenames according to the spec (as @mikaey pointed out). So, you have to do something to get rid of those lowercase letters. I don’t believe we should throw an error because the FAT spec calls for converting lowercase letters to uppercase, for the purposes of short filenames, rather than throwing an error, and every FAT implementation I’ve ever used follows this behavior, except for this one.

That’s why the name the user supplies should be modified. It’s what the spec calls for, and it’s what everyone else does, and not doing so causes compatibility problems, as I documented in the original issue.

TheMiningTeamYT commented 8 months ago

I've also been experiencing random resets and filesystem corruption, but I'm not sure if that's the libraries fault or my fault. It might be because I'm not calling msd_Close in usb.c, but that's because with my 16GB SanDisk Cruzer Micro, msd_Close causes my calculator to freeze. I might create an issue for that, but I suspect the bug might be specific to this 1 flash drive, since my other spare flash drive that works with my calculator, a 128MB "Nexdisk" that might actually be 20 years old, doesn't encounter that bug. However, the Nexdisk is super slow, the Cruzer is not, and I don't have any other flash drives that work with my calculator, so it is what it is. :(

By the way, should I make an issue about this? About msd_Close causing my calculator to freeze?

mateoconlechuga commented 8 months ago

I think you've convinced me - hopefully in your program you can work around it your code for now until I get the time to fix it. Yes, please make another issue and document the drive you are using (maybe an Amazon link too)