possible infinite loop in sys::windows::fill_utf16_buf

[rkuhn]

Looking at the code without prior knowledge of Windows system call semantics, I see an infinite loop if the syscall were to fill up the buffer exactly: when k == n on line 195, the loop will be rerun with no changes, which should result in the exact same outcome if the syscall is idempotent.

Even if Windows guarantees in some way that this cannot happen, would it not be clearer to handle this case in an obvious fashion?

[ChrisDenton]

Here's one of the docs on how filling a buffer usually works in Win32:

If the function succeeds, the return value is the length, in TCHARs, of the string copied to lpBuffer, not including the terminating null character. If the return value is greater than nBufferLength, the return value is the length, in TCHARs, of the buffer required to hold the path.

Note the difference in lengths between success and failure. So there's two potential cases where k==n:

• Success: If the buffer size (with NULL) were to be equal to the successfully copied size (without NULL) then the buffer would be too small to hold the full string (NULL included). So this can't happen.
• Failure: If the buffer size (with NULL) were to be equal to the buffer size needed (with NULL) then the string would have been successfully copied into the buffer. So this can't happen.

So Windows does indeed guarantee this can't happen.

[rkuhn]

OTOH GetFinalPathNameByHandleA on some versions returns the size written including the NUL terminator, so for that function the infinite loop could happen, right?

The doubling logic that checks for the insufficient buffer error code must be there for some other function that I have not yet found, it would be great to have a comment as to where this behavior is needed.

[ChrisDenton]

As far as I know Rust only ever uses the Unicode version of functions (they end in W) and never the "ANSI" ones (they end in A). The W functions are more consistent across Windows versions.

I'm not at all against adding a comment (and returning an error?) in this case but it's not something that should ever happen with the kinds of APIs Rust uses.

[ChrisDenton]

Come to think of it fill_utf16_buf pretty much implies using the W functions. It uses a u16 buffer, where as the A functions use a u8 buffer. Using this function with the A functions would be very broken.

[rkuhn]

Still, it would be more robust to change the k >= n into k > n, so treat this presumed impossible case as success (absent other evidence) rather than looping.

[ChrisDenton]

My issue with assuming success is that we can't know what a hypothetical Win32 function is doing in this case. For example, it may have truncated the output or it may even be returning an error code, not a size. We can only guess. Best case is we guess right and it works. Worse case is we guess wrong and it causes a bug that only becomes apparent later on, making the source of the bug hard to track down.

Having given more thought to this, I'm leaning towards using unreachable. That would clearly document that the case is thought impossible, while also causing a panic if the function is misused.

It might also be good to clearly document the expected return for the f1 function being passed in. E.g.:

• On success returns the size of the copied string excluding the terminating null.
• On error returns zero unless the last-error code is set to ERROR_INSUFFICIENT_BUFFER.
• If the last-error code is ERROR_INSUFFICIENT_BUFFER returns the buffer size required, including space for a terminating null.

[rkuhn]

Yes, that sounds like a good improvement! Since this Win32 behavior is not generically documented, it might also be good to note the exact syscalls that this function has been tested with. I should probably mention that I don’t have a Windows system to develop or test this on, I was merely perusing the code out of curiosity — otherwise I’d volunteer a PR.