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

fiopen handling of non-standard flags #2093

Open
FrankHB opened this issue Aug 4, 2021 · 3 comments
Open

fiopen handling of non-standard flags #2093

FrankHB opened this issue Aug 4, 2021 · 3 comments
Labels
question Further information is requested vNext Breaks binary compatibility

Comments

@FrankHB
Copy link

FrankHB commented Aug 4, 2021

The implementation strategy in https://github.com/microsoft/STL/blob/main/stl/src/fiopen.cpp is quite unnatrual, which leaves to problems for non-standard flags, even the expect behaviors are not well-documented.

What does ios_base::_Nocreate exactly mean?

By

mode |= ios_base::in; // file must exist
, ios_base::_Nocreate implies ios_base::in. But what happens for a write-only mode, i.e. ios_base::out or ios_base::app? Is it the tradition meaning of "nocreate" in this case?

TOCTTOU issue on handling io_base::_Noreplace

This one seems a real bug. Between

&& (fp = _Xfsopen(filename, 0, prot)) != nullptr) { // file must not exist, close and fail
and
if ((fp = _Xfsopen(filename, n, prot)) == nullptr) {
, if a file named by filename is created, the check is voided, so the flag is useless in this case. It looks like the same to the infamous anti-pattern of multiple calls to POSIX open to make sure the exclusive open-and-create semantics. The idiomatic correct way is using O_CREAT | O_EXCL in a single call, to enforce the atomicity.

Related implementation question

The concerned functions are implemented by the stdio extension function _fsopen. Why not use lowio (i.e. _wsopen) instead? This can easily resolve the correctness problems mentioned above, and it could be a bit more efficient due to the simpler mode translation and parsing (in the underlying implementation; once you have the implementation-specific knowledge, it seems not difficult to bypass the redundant passing like __acrt_stdio_parse_mode in _wfdopen) even if an object referenced by FILE* is still needed to be created later.

If lowio is not an appropriate choice for some coding policies I don't know, is it considerable to use C11-style "x" modes in

"r", "w", "w", "a", "rb", "wb", "wb", "ab", "r+", "w+", "a+", "r+b", "w+b", "a+b", nullptr};
?

@FrankHB FrankHB added the question Further information is requested label Aug 4, 2021
@frederick-vs-ja
Copy link
Contributor

This is probably fixed by #3065.

@StephanTLavavej StephanTLavavej added the vNext Breaks binary compatibility label Mar 11, 2023
@StephanTLavavej
Copy link
Member

Nothing passes _Nocreate so we should remove it in vNext and investigate the other questions here.

@frederick-vs-ja
Copy link
Contributor

@FrankHB

Perhaps _Noreplace was broken before but it's probably fixed by #3065 by using C11 "x" mode in fopen (with the addition of C++23 noreplace), unless UCRT's fopen is broken.

Per the following line:

mode &= ~(ios_base::ate | ios_base::_Nocreate);

I believe MSVC STL's ios_base::_Nocreate is not only implying ios_base::in, but also equivalent to it. So it doesn't mean any non-standardized "traditional meaning".

@StephanTLavavej

As we removed ios_base::hexfloat before (#4345), I think we can remove _Nocreate from <xiosbase> before vNext. Although we may need to preserve the related logic in fiopen.cpp for ABI compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested vNext Breaks binary compatibility
Projects
None yet
Development

No branches or pull requests

3 participants