Any chance std::fopen() call can be changed to std::fopen_s()?

[misuo]

In sendfile_defs_default.hpp the restinio open_file() function uses std::fopen().

Is there any chance this can be changed to using std::fopen_s()?

Should be a simple change:

inline file_descriptor_t
open_file( const char * file_path )
{
    file_descriptor_t file_descriptor;
        std::fopen_s( &file_descriptor, file_path, "rb" ); // return code ignored.
  ...
}

Background

I'm using MSVC and want to build with security enhancements in the CRT library, in which fopen_s() is prefered, see MS documentation.

When compiling my code with "treat warnings as errors" and using restinio, I see...

...\restinio\sendfile_defs_default.hpp(42,43): error C2220: the following warning is treated as an error
    file_descriptor_t file_descriptor = std::fopen( file_path, "rb" );
...\sendfile_defs_default.hpp(42,43): warning C4996: 'fopen': This function or variable may be unsafe. Consider using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. (compiling source file 

I do not want to define the _CRT_SECURE_NO_WARNINGS as this will disable security enhancements for the rest of the code. I think part of the problem is that open_file() is an inline function.

[eao197]

Hi!

I think it's possible to change RESTinio's code to do check of RESTINIO_USE_FOPEN_S preprocessor symbol. If such symbol is defined then RESTinio will use std::fopen_s instead of std::fopen.

In that case you have to add RESTINIO_USE_FOPEN_S to your project's predefined symbols.

[misuo]

BTW: It should probably not be std::fopen_s as this is not part of C++ (not defined in cstdio), so plain fopen_s without std namespace.

[ngrodzitski]

Why simply not to use fopen_s as the only way? Is there any case it is not available with compiler that already C++17 at minimum?

If fopen_s is always available then having RESTINIO_USE_FOPEN_S is redundant.

[eao197]

If I understand correctly, the presence of fopen_s isn't related to C++ standard. It's a part of C11 standard.

And MS complier is a strange thing: it has no appropriate support (AFAIK) for receint C standards, but provides a lot of functions with _s suffix and "secure alternatives".

[ngrodzitski]

https://en.cppreference.com/w/cpp/17

"Major portion of C11 standard library" => https://wg21.link/P0063R3 it does have "fopen_s" in it.

[eao197]

"Major portion of C11 standard library" => https://wg21.link/P0063R3 it does have "fopen_s" in it.

It's great. So we can just call fopen_s and wait while someone opens an issues with absence of fopen_s on his/her plaform :) Speaking seriously, if we can just use fopen_s then it's a really good thing.

[eao197]

What I don't understand is how to check the changes and how @misuo got the error because on Windows platform sendfile_defs_default.hpp should not be included, because on Windows sendfile_defs_win.hpp has to be included: https://github.com/Stiffstream/restinio/blob/2773e063ee301aa91b3055ef951613e1e4eda7e0/dev/restinio/sendfile.hpp#L30-L31

[misuo]

What I don't understand is how to check the changes and how @misuo got the error because on Windows platform sendfile_defs_default.hpp should not be included, because on Windows sendfile_defs_win.hpp has to be included:

sendfile_defs_default.hpp is included at the bottom of sendfile_defs_win.hpp when RESTINIO_ASIO_HAS_WINDOWS_OVERLAPPED_PTR is not defined. I'm not familiar with what that define do.

[eao197]

@misuo do you use Boost.Asio?

[eao197]

The RESTINIO_ASIO_HAS_WINDOWS_OVERLAPPED_PTR is defined here (if standalone Asio is used): https://github.com/Stiffstream/restinio/blob/1f95dbc72b94b575d1b860f569fe85511a1fff15/dev/restinio/asio_include.hpp#L54-L57 or here (if Boost.Asion is used): https://github.com/Stiffstream/restinio/blob/1f95dbc72b94b575d1b860f569fe85511a1fff15/dev/restinio/asio_include.hpp#L106-L109

If ASIO_HAS_WINDOWS_OVERLAPPED_PTR (or BOOST_ASIO_HAS_WINDOWS_OVERLAPPED_PTR) is not defined then Asio can't detect the presence of overlapped API. And it's a strange thing as for me. I don't understand how it can be. The corresponding fragment from Asio source code: https://github.com/chriskohlhoff/asio/blob/fa27820c05afd740fa2adc1ecfb9da5afe026443/asio/include/asio/detail/config.hpp#L934-L940 And this is how ASIO_HAS_IOCP is detected: https://github.com/chriskohlhoff/asio/blob/fa27820c05afd740fa2adc1ecfb9da5afe026443/asio/include/asio/detail/config.hpp#L790-L800 Maybe the ASIO_DISABLE_WINDOWS_OVERLAPPED_PTR is defined somehow?

[misuo]

@misuo do you use Boost.Asio?

No, only used as part of restinio. I do have the boost library in path and the only defines I have set for restinio is RESTINIO_FMT_HEADER_ONLY and FMT_HEADER_ONLY.

[eao197]

Thanks for clarification, @misuo

Anyway, it's a very strange situation because in the normal circumstances the sendfile_defs_default.hpp should not be included and you should not get errors about using std::fopen. Inclusion of the sendfile_defs_default.hpp also means that Win32-specific implementation of sendfile won't be used (and the default one isn't as efficient as Win32-specific one).

[eao197]

Hi, @misuo

The is the 0.7-dev-issue-199 branch. You can try it.

However, the main problem is that the sendfile_defs_default.hpp should never be included in the normal circumstances. The inclusion of the sendfile_defs_default.hpp means that RESTinio and Asio can detect the platform (and platform capabilities) properly. For example, the _WIN32_WINNT may not be defined at all or may have value less than 0x0401 (or there maybe definitions like ASIO_DISABLE_IOCP or ASIO_DISABLE_WINDOWS_OVERLAPPED_PTR).

[eao197]

@ngrodzitski

"Major portion of C11 standard library" => https://wg21.link/P0063R3 it does have "fopen_s" in it.

Unfortunately, that isn't that simple :(

cppreference says:

As with all bounds-checked functions, fopen_s only guaranteed to be available if __STDC_LIB_EXT1__ is defined by the implementation and if the user defines __STDC_WANT_LIB_EXT1__ to the integer constant 1 before including .

And that isn't not a case on Kubuntu 22.04, for example. So we can rely on presence of fopen_s if the compiler is not VC++.

@misuo

The current solution for your case is in 0.7-dev-issue-200 branch. It'll be great if you could check it. Otherwise the current version will be released "as is" in the upcoming 0.7.1 release (maybe on the next week).