newRPL: [UPDATED April 27-2017] Firmware for testing available for download

[matthiaspaul]

While I don't have the time to really dig deep into the sources, I have had a closer look and already identified a few bugs and compatibility issues. In some cases, the necessary changes are local enough, so that I can provide improved/fixed source code excerpts, in other cases I'll just give some hints.

Some FAT related data structures have evolved over time or have entries where the proper interpretation depends on various conditions. Several bugs in the implementation are down to not checking the presence of particular data structure versions (f.e. BPB variants) or not testing on the conditions under which they are valid.

https://sourceforge.net/p/newrpl/sources...extentry.c

...
while(FSReadLL(buffer,32,dir,fs)==32)
{
if(buffer[0]==0) return FS_EOF;
if(buffer[0]==0xe5) continue; // DELETED ENTRY, USE NEXT ENTRY
if( (buffer[11]&FSATTR_LONGMASK) == FSATTR_LONGNAME) {
// TREAT AS LONG FILENAME
...

The LONGMASK (0x0F) condition above is incomplete. On FAT12 and FAT16 volumes, the attribute combination 0x0F can also occur as part of a valid pending delete file under DELWATCH, however, actual VFAT LFN entries always have the cluster value at 0x1A set to 0x0000 and the length entry at 0x1C happens to never become 0x00000000 under these conditions. This check does not work for FAT32 volumes, but released versions of DELWATCH didn't support FAT32, and would have to use additional sanity checks, anyway. Consequently, the clause above should be expanded as follows (pseudo-code) in order to avoid any misinterpretation:

if ((buffer[0x0B] & FSATTR_LONGMASK) == FSATTR_LONGNAME) && // attr is 0x0F
(
(type == TYPE_FAT32) || // additional test on FAT12/FAT16 only
(((u16*)buffer[0x1A] == 0) && ((u32*)buffer[0x1C] != 0)) // cluster 0, size > 0
) {
// TREAT AS LONG FILENAME
...

Note, that this code uses boolean shortcut evaluation and that it deliberately does not test for the high-part of the cluster value at offset 0x14, as this is valid only on FAT32, and the additional test does not apply to FAT32.

Another similar test occurs somewhat further down in the source code:

...
// VERIFY THAT SHORT ENTRY FOLLOWS LONG NAME
if (((ptr[11] & FSATTR_LONGMASK) == FSATTR_LONGNAME) ||
(*ptr == 0) || (*ptr == 0xE5)) {
// VALID SHORT ENTRY NOT FOUND
...

I haven't checked if similar tests exists in other files as well.

There's another bug when retrieving the start cluster value:

entry->FirstCluster = buffer[26] + (buffer[27]<<8) + (buffer[20]<<16) + (buffer[21]<<24);

The code above is only valid on FAT32 volumes; on FAT12 and FAT16 it would have to read as follows, as the 16-bit entry at 0x14 holds other information on those volumes:

entry->FirstCluster = buffer[0x1A] + (buffer[0x1B]<<8);

So, this could become something like:

entry->FirstCluster = buffer[0x1A] + (buffer[0x1B]<<8);
if (type == TYPE_FAT32)
entry->FirstCluster += (buffer[0x14]<<16) + (buffer[0x15]<<24);

I've seen similar code sections in quite a few other source files as well (you probably know them by heart, whereas I haven't written them down yet) and they need to be changed as well. In general, it is invalid to assume that the 16-bit entry at 0x14 in directory entries is 0 on FAT12 and FAT16 volumes.

There's another potential problem here: The contents of entries in directory entries, which are not used by the implementation, must not be changed. So, in the above example, the code most probably would still have to store away the value at offset 0x14 even on FAT12 and FAT16 volumes for later restoration (just not treat it as cluster entry).

See, for example, this file:

https://sourceforge.net/p/newrpl/sources...direntry.c
fsupdatedirentry.c:

...
mainentry = buffer + 32*(file->DirEntryNum-1);
// write new properties
mainentry[11] = file->Attr;
// mainentry[12] = file->NTRes;
mainentry[13] = file->CrtTmTenth;
WriteInt32(mainentry+14, file->CreatTimeDate);
WriteInt16(mainentry+18, file->LastAccDate);
WriteInt16(mainentry+20, file->FirstCluster>>16);
WriteInt16(mainentry+26, file->FirstCluster);
WriteInt32(mainentry+28, (file->Attr&FSATTR_DIR) ? 0 : file->FileSize);
WriteInt32(mainentry+22, file->WriteTimeDate);
...

Unless this would be on a FAT32 volume, it would be invalid to overwrite the contents of the 16-bit entry at offset 20 with the high cluster (unless this would hold the original contents when opening the file - however, this would make it necessary to change the usage of the FirstCluster variable as cluster variable).

For some reasons, restoring the original value of the NTRes byte was commented out here. This byte is used for various purposes. It is important that the implementation does not change the contents of bits 7-5 and 2-0. It may change bits 3 and 4 when dealing with LFNs, and it may clear all bits when creating a new file. However, in an already existing file, the bits must not be changed.

In the following file:

https://sourceforge.net/p/newrpl/sources...direntry.c
fsdeletedirentry.c

the following excerpt can be found:

...
mainentry = buffer;

for (f = 0; f < file->DirEntryNum; ++f, mainentry += 32) {
mainentry[0] = 0xE5;
}
...

This should be changed to something like the following in order to allow files to be undeleted without having to enter the first character of the files again:

...
mainentry = buffer;

for (f = file->DirEntryNum; f > 0; --f, mainentry += 32) {
if (f == 1) {
mainentry[0x0D] = mainentry[0]; // save 1st char of SFN at 0x0D for later undeletion (overwrite no longer needed creation ms)
(u32*)mainentry[0x0E] = 0; // clear 0x0E..0x11 creation date & time of deleted file
}
mainentry[0] = 0xE5;
}
...

So much for now, but there's more... ;-)

Hope it helps,

Matthias