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

[Claudio L.]

(08-01-2016 01:31 AM)matthiaspaul Wrote:  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.

I had a deeper look at these issues. I don't know where specifically you mention that a variant of the BPB needed to be detected. This implementation avoids anything that's non-standard, it's based directly on an official specification by MS (second external link on Wikipedia). I reviewed the code and it seems to comply. Can you point to any examples where it does not?

(08-01-2016 01:31 AM)matthiaspaul Wrote:  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...

This seems to be only happening on one (yours?) non-standard implementation of FAT 12/16, incompatible with the FAT32 spec.
While this is simple to support, I'm not sure it's in the best interest of the newRPL project to put much effort into supporting these minor variants.

(08-01-2016 01:31 AM)matthiaspaul Wrote:  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.

This is fine, although the MS official specification of FAT I linked above actually says the high bytes of cluster number must be set to 0 on FAT16/FAT12 implementations.

(08-01-2016 01:31 AM)matthiaspaul Wrote:  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).

Same as before, this code is consistent with the MS FAT specification, so it's not a bug at all and should be compatible with all other compliant FAT implementations.

(08-01-2016 01:31 AM)matthiaspaul Wrote:  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.

That's exactly the reason why it's commented out: the bits must not be changed. This function does read-modify-write on an existing entry, only to update the size, date, etc. when you close the file, and it only modifies what needs to be modified, then writes back.

(08-01-2016 01:31 AM)matthiaspaul Wrote:  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;
}
...

Is this something standard that many undelete programs use? or it's just the undelete feature of DR-DOS? I wonder if it's worth supporting or not (same as the other changes you suggested). I can't find any specifications other than the sections of Wikipedia, most of them written also by you so it seems pertinent to ask you the question.
I don't mean to disrespect your work on DR-DOS, I'm just not trying to rewrite DR-DOS but RPL :-), disk access is a secondary feature that as long as it's standards compliant doesn't deserve much attention, especially while we have only 20% of the RPL commands implemented.
These bugs you found so far are not actual bugs, but minor incompatibilities with a particular variant of FAT that's not widely in use today (unless you tell me Linux and Windows have incorporated these changes lately, then I'll be forced to do it).
Now these code changes seem simple and doable, so I might incorporate them just for good measures to bullet-proof the code in case of corrupted data.
Now if you volunteer to improve and maintain newRPL's file system... that's a different story, then you can do all these changes and more while I work on RPL :-)