Potential OpenNtfsDxe code issues

[mikebeaton]

OpenNtfsDxe.efi possible FileReadDir Issues

I was looking at FileReadDir, which is used to return EFI_FILE_INFO about a directory entry, one file at a time, when FileRead is called on a directory.

I was looking into this because the implementation here interacts badly with ls from Open Shell. @MikhailKrichanov kindly helped me to identify that this is because UEFI Shell is only allocating enough space for the largest FAT32 directory entry (and it will never reallocate more, even if it is told by a file system that more is needed, see UefiFileHandleLib.c line 509), whereas OpenNtfsDxe is always requiring that the allocated buffer be large enough for the largest possible NTFS directory entry (which is larger than the largest FAT32 entry; and whether or not the buffer actually needs to be this large, in any given case).

• I think that it would be nice/preferable to re-architect this, so that OpenNtfsDxe only requires the buffer to be as large as needed for each entry actually found; however on looking into this in more detail, I can see that everything below FileReadDir simply assumes that the buffer is already of this maximum size (which is at least safe, since that's what the top level of the call stack is requiring)
• Note that the UEFI spec does say "If the Buffer is not large enough to hold the current [emphasis added] directory entry, then EFI_BUFFER_TOO_SMALL is returned and the current file position is not updated."

Additionally, when tracing down the call stack involved in this, I found other code features which look to me possibly questionable.

FileReadDir calls NtfsDir, and NtfsDir declares that it returns an OUT param of type EFI_NTFS_FILE: this declaration is straight up incorrect (at least when called with DIR_HOOK value for final parameter, as it is here). In fact it eventually returns data of type EFI_FILE_INFO (as we need it to). Obviously having an incorrect return type here makes the code hard to read and maintain.

Additionally, NtfsDir (lines 29-30) actually starts to populate a couple of the fields in this buffer as data of the 'wrong' EFI_NTFS_FILE type, but these are eventually overwritten later by EFI_FILE_INFO data. In terms of available space for this, this is only safe because we already know that the buffer is large enough for the largest possibly NTFS directory entry. Additionally, as far as I can see, this is not even a case of temporarily re-using the caller-allocated memory as a scratch pad for an alternative purpose - i.e. nowhere in the call stack that I can make out are the two populated fields of the 'wrong' type (namely EFI_NTFS_FILE.RootFile and EFI_NTFS_FILE.MftFile) ever actually used - but perhaps I am missing something?

In terms of what happens with this mixed type pointer moving further down, the full call stack is FileRead->FileReadDir->NtfsDir->IterateDir->ListFile->NtfsDirHook. The same pointer is passed all the way down this chain. IterateDir and then ListFile both label it as VOID *FileOrCtx, then eventually NtfsDirHook labels and populates it as EFI_FILE_INFO * (as we actually want). Again this switching of pointer types I think is somewhat dubious, and makes the code hard to read and maintain (but at least the two intermediate methods make it clear in their param naming that some mixed pointer type is being used).

Additional points:

I think the if (Length == MAX_PATH) test at line 188 of Open.c can never evaluate to true: the maximum length of a string copied safely to a buffer of size MAX_PATH is MAX_PATH - 1.

Also, I think the if (Length == 0) test at line 199 of Open.c can never evaluate to true: we have just increased the length to a minimum of 1, in lines 193-196.

Additionally, mIndexCounter (as used in this call tree) is a 'static' var. Actually, it's not even static, it's shared between two files (Open.c and Index.c), so I'm not sure if m is the correct preface, in that case?
I'm also not quite sure if there is a possibilty of different directory reads colliding in the use of this global var (e.g. if different directories are being listed at once), although I believe not, and it is rather just a not-so-nice way of passing a parameter down to the bottom of the call stack (which is safe, at least, as long as everything is single threaded).

Finally, though I appreciate that this is just a naming issue, and others might disagree: the constant defining the max. size that an NTFS directory entry can occupy is MINIMUM_INFO_LENGTH which I think is arguably confusing naming. I appreciate that this is the minium which a caller must allocate, in the current implementation, but it is not the minimum a caller has to allocate to get results in any possible implementation: in particular, not if we only complained when the buffer is too small for the actual directory entry being listed; rather, it is the maximum a caller could possibly ever need to allocate to be sure of always getting all results; so the name seems to me confusing, and MAXIMUM_INFO_LENGTH would maybe be less so?

Note that similar issues, of naming (if agreed that that is an issue), and (more importantly) of requiring the caller to always allocate the maximum that could possibly be needed, even if less is actually needed, apply to FileGetInfo and MINIMUM_FS_INFO_LENGTH as well. (I have not so far traced down that other call stack to see if any other more or less minor code issues might apply there too.)

Suggestions

• The unreachable code should be removed
• The misleading pointer types in some functions should at least be changed to VOID * and renamed to show that they are multivalent (as there are in some other, but not all, such functions, as noted above)
• The case where it seems data of the wrong type is partially populated, then overwritten, should be checked (and fixed unless I'm misunderstanding and that data is, at least, being used as temporary scratch data)
• Ideally both instances identified above (involving MINIMUM_INFO_LENGTH, and MINIMUM_FS_INFO_LENGTH), should only require the caller to have allocated what is actually needed for a given call, corresponding to a given file path (in which case UEFI Shell ls would already succeed for shorter NTFS paths, even though it would fail - and it would be the shell's fault! - for longer paths)
  • I appreciate this would take a fair amount of re-architecture, because the available size is not passed down, and everything below the top level already assumes that the a buffer of the maxiumum possible size is always allocated (e.g. for zeroing, etc.)