HACK: windows: Work around broken AssocQueryStringW() not returning actual string length

[MarijnS95]

Wine has a bug where calling AssocQueryStringW() with a buffer doesn't actually update line_len to the actual number of bytes returned, resulting in webbrowser failing on seeing a bunch of unexpected nul bytes in the array:

Error { kind: InvalidInput, message: "nul byte found in provided data" }

This is reported and fixed upstream via:

https://bugs.winehq.org/show_bug.cgi?id=59402
https://gitlab.winehq.org/wine/wine/-/merge_requests/10072

I still need to write tests to get it merged, and even then it might take a while to trickle down into releases and ultimately Steam Proton. Hence have this ugly workaround to catch cases where nul characters end up in the output and reduce the string length by hand.

[amodm]

Thanks, @MarijnS95. I appreciate your continued contributions to maintaining this crate.

I was initially considering if we should replace the condition with if line_len == BUF_SIZE && is_wine(), but on more thought I'm not fully sure if it adds any further value. If line_len == BUF_SIZE, then most likely we're in a bad situation anyway, and the only way out is scanning for null terminator.

Given that this scenario exposed a bug related to line_len, I'm wondering if the check should instead be a stricter: if line_len >= BUF_SIZE. But I'll let it be your call.

The PR LGTM. But if you're marked it as draft, so I'll wait for you to update its status, post which I'll merge & release.

[MarijnS95]

@amodm thanks! This was marked draft because I wasn't too happy with having a windows-emulation-specific workaround in client code, but in hindsight the extra validation probably doesn't hurt.

The specific wine condition only happens where line_len doesn't get updated. It is however true that *pcchOut can be increased to contain the required string length if the original buffer size is too small. By default the function returns STRSAFE_E_INSUFFICIENT_BUFFER and a not-null-terminated string, and with ASSOCF_NOTRUNCATE the buffer isn't touched and returns E_POINTER, but still with the required size in *pcchOut.

Neither case should pass through because of != S_OK check on the error ¹, but if it does return S_OK we just panic in Rust later because line_len is out of bounds for cmdline_u16.

Allowing this fallback path through for those scenarios too with the suggested >= is unlikely to help, because the NUL terminator isn't found (unless we set ASSOCF_NOTRUNCATE which leaves the entire buffer default-initialized with NUL characters).

In any other case where line_len > BUF_SIZE while the function returns S_OK, cmdline_u16 would be filled with to-us-unknown contents (likely unterminated string) and we should just panic too.

There's one argument to be made that my workaround code (and trace log) also triggers for the one valid case where the string including NUL character is exactly BUF_SIZE, but it'll just find the terminator in the exact same spot.

Footnotes

1. On a side-note, would it be helpful to return the underlying error to the caller via std::io::Error::from_raw_os_error()? ↩

[amodm]

This is now out as v1.2.1.

About return the raw os error, I'm not fully sure because any meaningful use of it will involve the caller to have platform specific interpretation which isn't a good API for this crate IMHO. Alternatively, if we were to think of such an API change as a diagnostic tool only, perhaps that's probably better handled via logging instead of returning a code.

[MarijnS95]

Awesome, thanks for the quick release!

You're right that having such a low-level error isn't too useful in the public API, but using the dynamic fn Error::source() -> Option<...dyn Error> might alleviate that somewhat. This info might not be useful to most users anyway, but perhaps to developers in a debug-printed stack of source() errors like what anyhow does.