wosix_* APIs returning 32-bit int

[datacore-skumar]

When HANDLE in 64bit windows kernel is 64bits, how can wosix_ APIs return 32bit int? /

• Thin wrapper for the POSIX IO calls, to translate to HANDLEs
• Currently it "hopes" that HANDLE value will fit in type "int".
• This could be improved in future. */ https://github.com/openzfsonwindows/openzfs/blob/windows/lib/libspl/include/os/windows/wosix.h#L91

[lundman]

It returns 64bit, but the structs in libzfs uses "int". Which is 32bit on my Unix - I assume the same on Windows. So potentially any HANDLE > INT32_MAX could fail.

No?

[datacore-skumar]

https://github.com/openzfsonwindows/openzfs/blob/windows/lib/libspl/os/windows/posix.c#L903 CreateFile() (inside the wosix_open()) returns 64-bit HANDLE which is very likely to be > INT32MAX. This is being truncated to 32-bit and stored by the caller. Now there is no way to recover original 64-bits(corresponding to the HANDLE) from these 32-bits. That is the fundamental problem. Hence we don't see how wosix* is meant to work, what are your thoughts on this?

[lundman]

This is indeed the problem that I refer to in the comment. So far when by test runs, HANDLE starts around 180, 208 etc. I have not seen a HANDLE number even over a 1000. But, tiny VMs so you guys have most likely different experience.

So one option is to change all the "int fd" into a zfs_fd; type, which can stay int on Unix and HANDLE on Windows. But when I tested it, the size of the commit was rather large.

Another way is simply to simulate Unix process fd table. So we return an index, like 4, and look up in internal array (or whatever) to get real HANDLE.

How frequently have you seen the HANDLEs approaching 32bit limit (observed as opposed to theoretical). "Iff" Windows store HANDLEs based on processes (like Unix) and zfs/zpool/zdb are always short lived processes that exit when done, will we ever reach int32 limit?

Give me your thoughts?

[lundman]

I suppose one could possibly tell compiler to just make "int" be 64bit, as long as we ensure uint32_t isn't - for the on-disk structs in use by zdb etc.

[nazar554]

That seems to be implemented by CRT Either by using _open or _open_osfhandle with CreateFile

Low-Level I/O File handling

[lundman]

I was unable to open disks with _open as well as the /dev/zfs device node for ioctl. I'm unsure if I tried CreateFile with _open_osfhandle and ioctl() - but if you can get the original HANDLE back from a fd, it could certainly be done. It would be equivalent to option 2 above. (Assuming it can still be handled in the one call to close() - ie, it doesn't add HANDLE to also be closed or you can fetch that HANDLE for close, or, if each function, for example, lseek, goes to get HANDLE then releases it, that it doesn't get much slower).

Throw a ASSERT3U(h, <=, INT_MAX); here https://github.com/openzfsonwindows/openzfs/blob/windows/lib/libspl/os/windows/posix.c#L924

and see if you can make it trigger? I absolutely think it should be fixed, and welcome PRs, but I'm just not convinced the priority is all that high (on my) todo - unless you can demonstrate it gets in the way :)

Checking how ReactOS (Yeah I know, it's not Windows sources) when diving deep from CreateFile() we end up:

        /* Get the current handle table */
        HandleTable = PsGetCurrentProcess()->ObjectTable;
    }

    /* Increment the handle count */
    Status = ObpIncrementHandleCount(Object,
                                     AccessState,
                                     AccessMode,
                                     HandleAttributes,
                                     PsGetCurrentProcess(),
                                     OpenReason);

....

    Handle = ExCreateHandle(HandleTable, &NewEntry);

....

        Value = InterlockedExchangePointer((PVOID*)&HandleTable->TableCode, (PVOID)TableBase);

So the Table is connected to a Process, and it is stored in an array (which appears to be able to grow, with levels, neat) Which means the process table consumes 8 bytes per HANDLE, and the process would then need INTMAX / 8 (open?) HANDLES to overflow. Suggests if you open a HANDLE, it'll be something like 220, then next open HANDLE will be 228.

Did I get that right?

Of course that doesn't even consider SOCKETS, which is a separate beast on Windows. But I don't think we use those in userland?

https://github.com/reactos/reactos/blob/3fa57b8ff7fcee47b8e2ed869aecaf4515603f3f/ntoskrnl/ob/obhandle.c#L2665 https://github.com/reactos/reactos/blob/3fa57b8ff7fcee47b8e2ed869aecaf4515603f3f/ntoskrnl/ob/obhandle.c#L1625 https://github.com/reactos/reactos/blob/3fa57b8ff7fcee47b8e2ed869aecaf4515603f3f/ntoskrnl/ex/handle.c#L491

[imtiazdc]

Thanks @lundman for the detailed investigation on this one. Yes, it didn't get in our way and we could get zdb to work. I think we can move on and think of addressing this later if at all it becomes an issue.