Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
sylveon opened this issue Aug 6, 2020 · 7 comments
Assignees
Labels

Comments

@sylveon
Copy link

sylveon commented Aug 6, 2020

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 asklar added the Area-API label Aug 7, 2020
@asklar
Copy link
Member

asklar commented Aug 7, 2020

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
Copy link
Author

sylveon commented Aug 7, 2020

GetFirmwareEnvironmentVariableSize 😛

@asklar
Copy link
Member

asklar commented Aug 7, 2020

@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
Copy link
Author

sylveon commented Aug 7, 2020

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
Copy link

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
Copy link

riverar commented Aug 28, 2020

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
Copy link
Author

sylveon commented Sep 29, 2020

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

@AvriMSFT AvriMSFT self-assigned this Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants