microsoft / Windows-Dev-Performance

A repo for developers on Windows to file issues that impede their productivity, efficiency, and efficacy
MIT License
438 stars 21 forks source link

GetFirmwareEnvironmentVariable(Ex) does not provide a way to know the UEFI variable's size #29

Open sylveon opened 4 years ago

sylveon commented 4 years ago

Currently, this method and derivatives does not provide a way to know the exact size needed to store the resulting UEFI variable, meaning that the API must be called in a loop until it does not fail - this is bad API design and extremely inefficient.

Most APIs in Windows which can result in variable-sized data provide a way to retrieve its size before, for example GetWindowText has the matching GetWindowTextLength. While this does give the potential for race conditions, repeatedly calling the API in a loop has an even worse potential for race conditions.

asklar commented 4 years ago

Thanks for the feedback @sylveon! There are a number of win32 APIs that follow this pattern (this particular API goes all the way back to Windows XP SP1). To change the shape of the API means we'd have to create a new API with a different name (as these are plain C APIs) while keeping the existing ones working as they do today, since we can't break users of these existing APIs. I don't know if there are plans to make a new API to change the calling pattern, but UEFI variables should normally be pretty stable over time though, so I think the potential for race conditions/getting different values across a few calls over a couple milliseconds is pretty small.

sylveon commented 4 years ago

GetFirmwareEnvironmentVariableSize 😛

asklar commented 4 years ago

@sylveon :) My point though is: a) this data is unlikely to change rapidly so race conditions are unlikely to happen b) Nobody is blocked by this particular API following this pattern; there are a lot of much more-often used APIs that follow this try-again-with-a-bigger-buffer pattern (e.g. lots of Nt* and user32 apis). c) if the API is supposed to allocate the data, then now you need a second API that knows how to free this buffer from the correct heap in win32. Maybe a more natural pattern would be for a new WinRT API that returns an HSTRING since it's expected that the caller in that case will call WindowsDeleteString when they're done with it.

In any case, I don't think there is broad applicability to this one API being updated esp. having a working version, so if I had to guess, this API won't see a new version in the near future.

sylveon commented 4 years ago

I agree it's not critical, but a nice to have.

In my experience, the APIs following the try again with a bigger buffer pattern will return the size required in some manner upon calling it with a too small or null buffer, so that you only have to call the API twice, not repeatedly until you've got a big enough buffer. For example, GetModuleFileName.

While a WinRT API would be nice, I don't think a string is correct because UEFI variables often contain binary data, not strings.

driver1998 commented 4 years ago

I guess it can follow the traditional Win32 way of doing things: call the API with NULL which returns the required size, then call it again with a buffer of that size.

riverar commented 4 years ago

Thanks for the feedback @sylveon! There are a number of win32 APIs that follow this pattern (this particular API goes all the way back to Windows XP SP1).

@asklar I'm not familiar with other Win32 APIs that work this way. Which ones are you referring to when you say that? I think you're confusing this reported issue with the (generally accepted) pattern of calling an API twice, first with a zero-sized buffer or nullptr to retrieve the target size.

sylveon commented 3 years ago

Yes, that's the pattern I would expect from the API, not "try again until it hasn't failed"