collection of findings with VBOXFS

[erdmann]

In this ticket I will add findings against the VBOXFS code. On each comment I will state what version of VBOXFS I have been using.

[erdmann]

r218:

vboxfs.cpp, "GetEmptyEAs":
this:

KernCopyIn?(&feal, peaop->fpFEAList, sizeof(feal));
KernCopyIn?(&geal, peaop->fpGEAList, sizeof(geal));

should be:

KernCopyIn?(&cbFeal, peaop->fpFEAList, sizeof(cbFeal));
KernCopyIn?(&cbGeal, peaop->fpGEAList, sizeof(cbGeal));

because as a minimum OS/2 should always pass the "cbList" element but it is not guaranteed that a FEA or GEA structure will follow (it's a variable sized list which can contain 0 elements).

[Valery V. Sedletski]

cbFeal and cbGeal are not yet defined at this point. Except this, I got an error when
copying files because I changed sizeof(feal) to cbFeal in a similar place. When I changed it back, everything repaired. (Look at FIL_QUERYALLEAS option in FS32_PATHINFO, PI_RETRIEVE part. If I change sizeof(feal) in KernCopyOut? at the end, to cbList, copy operation will fail).

Though, your note about looks valid. Need to check this, of course.

[erdmann]

r218:
vboxfsfind.cpp, "FillFindBuf?":
there is a misunderstanding of the additional "position" field that exists if FF_GETPOS (0x01) is specified for flags.
This field is NOT the same as the "oNextEntryOffset" field in the 32-bit FILEFINDBUF3/FILEFINDBUF4 structures. The "position" field can contain arbitrary data that allows the FS to resume a search from the last position if had been processing. The "oNextEntryOffset" field in the 32-bit structures is instead an offset from one FILDFINDBUF3/4 element to the next.
The kernel will take care that the filefind structure of the FS is properly transformed into the FILEFINDBUF3/4 structure that the application is using.
Which also means that you don't return this "position" field if you do not return a filefind structure and it also means that you can set this field to plain 0 if you do not intend to use it on resuming a search (a search is resumed in FS32_FINDNEXT). You NEVER return this field if FF_GETPOS is NOT set.
If FF_GETPOS is set, you need to return a "position" field for every found file/directory element and not just for the first element (I am not 100% about the last sentence but I believe so, to be checked).

[erdmann]

about comment #2:
I did NOT say:
KernCopyIn??(&cbFeal, peaop->fpFEAList, cbFeal);
instead I said:
KernCopyIn??(&cbFeal, peaop->fpFEAList, sizeof(cbFeal));

sizeof(cbFeal) is 4. The intention is to just copy the length field as a first step before looking any further.

[Valery V. Sedletski]

Wrt oNextEntryOffset: Indeed, I confused this field with another field. oNextEntryOffset
is added by the kernel, not the IFS, and it is the offset to the next structure. I used this logic from fat32.ifs as a template. This field is called ulOrdinal there, not oNextEntryOffset. But the logic is almost the same. No idea why I decided that that field is the same as oNextEntryOffset.

Also, no idea why I'm copying this field in some cases where flags != FF_GETPOS. Need to check this. I need to recall the logic. Last time I touched this code was a long time ago, so I forgot some things.

[Valery V. Sedletski]

Replying to erdmann:

about comment #2:
I did NOT say:
KernCopyIn??(&cbFeal, peaop->fpFEAList, cbFeal);
instead I said:
KernCopyIn??(&cbFeal, peaop->fpFEAList, sizeof(cbFeal));

sizeof(cbFeal) is 4. The intention is to just copy the length field as a first step before looking any further.

I mean, cbFeal, not cbList (the 1st field of FEALIST structure). If FEA array is
empty, cbFeal == sizeof(cbList) == 4.

[erdmann]

What I meant:
on the initial KernCopyIn?, only copy 4 bytes which is equivalent to sizeof(cbFeal) and also sizeof(cbGeal). Do not copy more than this because there might not be more than this field.

[erdmann]

about "FillFindBuf?": I think the "skip" flag needs to be looked at. I doubt that it is necessary.
The rule is simple: if flags == FF_GETPOS then you always need to prepend the "position" field to the filefind structure (for EACH filefind structure you return. The IFS spec states "...must store in the first DWORD of the per-file attributes structure … This would imply that this field exists for each filefind structure and therefore for each file/directory found). This is true for all info levels.
If you don't actually care about this value on a subsequent F32_FINDFROMNAME, you can enter any value for "position" that you want, in that case I propose that you just enter 0UL.
If you look at FS32_FINDFROMNAME, you will notice that it has a parameter "position". This is exactly that value that you save in "position" on a FS32_FINDFIRST,FS32_FINDNEXT call. The FS32_FINDFROMNAME can then resume with a filename and this additional position info.

[Valery V. Sedletski]

The "skip" flag is necessary to skip some files that are not matched by DOS attribute
mask. So, it is absolutely necessary.

[erdmann]

r218:
VBoxFS.cpp, "GetEmptyEAs":
the allocated buffer length for pFeal is too small. You have to precompute how large that buffer will be by cycling through the pGeal list and computing FEA structure + filename length for each entry to return in FEAList and summing up. Once you know that full length you can then allocate the pFeal structure. You will need to ensure that this value does not exceed the initial value of pFEAList->cbList-4 (where the 4 is for the 4 bytes of the cbList field itself. As an alternative you can start with an initial length count of 4 and compare to pFEAList->cbList directly).
You might even want to count the number of EAs that you can handle while in the summing up loop described above by always comparing against pFEAList->cbList on each loop iteration.
Once this is all done you can generate FEAlist and use KernCopyOut? to copy your populated FEAList to the original address in user space. You can then even conveniently take the length value that you summed up in your GEA checking loop.
By the way: there is no need to copy anything from the FEAList. It is output only. You just need the pointer to this list to populate it.
Another by the way: you already have the summing up loop in your code. The result that is interesting to you is ulFeaSize. And only if you do NOT return with ERROR_EAS_DIDNT_FIT will you allocate the FEAList buffer of this length and use it.

You are also incorrectly returning when ERROR_EAS_DIDNT_FIT. Since cbList is "very big" you need to do a special KernCopyOut?:

   if (ulFeaSize > ulMaxSize)
      {
      /* this is what HPFS.IFS returns */
      /* when a file does not have any EAs */
      ULONG cbList = 0xEF;
      KernCopyOut(peaop->fpFEAList,&cbList,sizeof(cbList));
      rc = ERROR_EAS_DIDNT_FIT;
      }

move the following KernCopyOut? into the "else" branch.

[erdmann]

r218:
VBOXFSFind.cpp, "FillFindBuf?": I recommend to move the thunking calls for peaop->fGEAList and peaop->fFEAList into GetEmptyEAS itself and remove them from anywhere else. That is much more understandable and less error prone.

It is also incorrect to thunk these two pointers and copy them back into the EAOP return structure as is done here:

    if (level == FIL_QUERYEASFROMLIST || level == FIL_QUERYEASFROMLISTL)
    {
        KernCopyIn(&eaop, pbData, sizeof (EAOP));
        pFeal = (PFEALIST)RTMemAlloc(MIN_EA_SIZE);
        
        if (! pFeal)
        {
            hrc = ERROR_NOT_ENOUGH_MEMORY;
            goto FILLFINDBUFEXIT;
        }

        eaop.fpFEAList = pFeal;
        eaop.fpGEAList = (PGEALIST)KernSelToFlat((ULONG)eaop.fpGEAList);
    }

    memset(pbData, 0, cbData);

    if (level == FIL_QUERYEASFROMLIST || level == FIL_QUERYEASFROMLISTL)
    {
        KernCopyOut(pbData, &eaop, sizeof(EAOP));
        pbData += sizeof(EAOP);
        cbData -= sizeof(EAOP);
    }

These pointers HAVE to remain 16:16 far pointers in the returned kernel structure.
In fact the EAOP structure has to remain completely UNCHANGED in the return buffer.

[erdmann]

If you skip a file then do not return ANYTHING. In other words, do also not return a "position" field if FF_GETPOS is set. Currently you do, which is wrong.