microsoft / STL

MSVC's implementation of the C++ Standard Library.
Other
10.21k stars 1.51k forks source link

`fiopen` handling of non-standard flags #2093

Open FrankHB opened 3 years ago

FrankHB commented 3 years ago

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 https://github.com/microsoft/STL/blob/cc515bbb42dbbfba184527ad2e7c093a78c10442/stl/src/fiopen.cpp#L52, 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 https://github.com/microsoft/STL/blob/cc515bbb42dbbfba184527ad2e7c093a78c10442/stl/src/fiopen.cpp#L71 and https://github.com/microsoft/STL/blob/cc515bbb42dbbfba184527ad2e7c093a78c10442/stl/src/fiopen.cpp#L80, 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 https://github.com/microsoft/STL/blob/cc515bbb42dbbfba184527ad2e7c093a78c10442/stl/src/fiopen.cpp#L14?

frederick-vs-ja commented 1 year ago

This is probably fixed by #3065.

StephanTLavavej commented 1 year ago

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

frederick-vs-ja commented 7 months ago

@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: https://github.com/microsoft/STL/blob/be81252ed1f5e5fc6d77faca0b5dbbbdae8143a2/stl/src/fiopen.cpp#L64

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.