chriskohlhoff / asio

Asio C++ Library
http://think-async.com/Asio
4.84k stars 1.2k forks source link

warning: 'sprintf' is deprecated #1148

Open Shebuka opened 1 year ago

Shebuka commented 1 year ago

Using Xcode 14.0.1 with Apple clang version 14.0.0 (clang-1400.0.29.102) I get this deprecation warning:

asio/ip/impl/network_v6.ipp:104:3: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only. Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]

This points to sprintf(prefix_len, "/%u", prefix_length_);. Because prefix_len is a char prefix_len[16]; the fix for this is quite simple, replace the line with with snprintf(prefix_len, 16, "/%u", prefix_length_);

similar warning in other places:

asio/ip/impl/network_v4.ipp:135:3: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only. Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]

asio/detail/impl/socket_ops.ipp:2522:7: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only. Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]

mojca commented 1 year ago

Same problem here, except that the code we were building contained -Werror and the CI jobs suddenly started failing after merely installing "security update from Apple" (I think that just command-line tools were updated).

MarkOates commented 1 year ago

Also seeing this on my end. It's resulting in some noisy compile logs. I'm commenting here because it looks like this particular issue has the most traction.

@Shebuka It looks like a relatively trivial fix. Anybody know if the maintainers are warm to pull requests with this change?

MarkOates commented 2 months ago

It's possible to disable the compile warning at the point of include. It doesn't fix the underlying problem, but if you're wanting to silence all the compiler noise (in my case) this is an option:

#if defined(__clang__)
  #pragma clang diagnostic push
  #pragma clang diagnostic ignored "-Wdeprecated-declarations"
#elif defined(__GNUC__) || defined(__GNUG__)
  #pragma GCC diagnostic push
  #pragma GCC diagnostic ignored "-Wdeprecated-declarations"
#endif

#include <asio.hpp>

#if defined(__clang__)
  #pragma clang diagnostic pop
#elif defined(__GNUC__) || defined(__GNUG__)
  #pragma GCC diagnostic pop
#endif

I've run this on gcc (windows) and clang (mac), and it seems to have worked.

Shebuka commented 2 months ago

I think (but I'm not sure) this was solved in the https://github.com/chriskohlhoff/asio/releases/tag/asio-1-26-0 release. In the https://github.com/chriskohlhoff/asio/releases/tag/asio-1-28-0 that I'm using now it's solved 100%.

stephanlachnit commented 2 months ago

Note: this is not solved in 1.28

Shebuka commented 2 months ago

Note: this is not solved in 1.28

Are you sure to use the latest version?

To double-check I've just searched sprintf in the source and it's not present anymore in the library: https://github.com/search?q=repo%3Achriskohlhoff%2Fasio%20sprintf&type=code

stephanlachnit commented 1 month ago

We can't use >=1.30 due to a GCC bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114968), and unfortunately there is one instance of sprintf still present in 1.28 here: https://github.com/chriskohlhoff/asio/blob/54e3a844eb5f31d25416de9e509d9c24fa203c32/asio/include/asio/detail/impl/socket_ops.ipp#L2584

It is also present in your search if go expand the match list, or search for sprintf(: https://github.com/search?q=repo%3Achriskohlhoff%2Fasio+sprintf%28&type=code

Shebuka commented 1 month ago

Rookie mistake on the search, my bad 😅

Right now I'm building the 1.28 with Xcode 15.2 / Apple clang version 15.0.0 (clang-1500.1.0.2.5) and I have no warnings about sprintf.

We can't use >=1.30 due to a GCC bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114968), and unfortunately there is one instance of sprintf still present in 1.28 here:

https://github.com/chriskohlhoff/asio/blob/54e3a844eb5f31d25416de9e509d9c24fa203c32/asio/include/asio/detail/impl/socket_ops.ipp#L2584

But snprintf is safe. Were you referencing the line 2586?

https://github.com/chriskohlhoff/asio/blob/54e3a844eb5f31d25416de9e509d9c24fa203c32/asio/include/asio/detail/impl/socket_ops.ipp#L2586

It is also present in your search if go expand the match list, or search for sprintf(: https://github.com/search?q=repo%3Achriskohlhoff%2Fasio+sprintf%28&type=code

That line and all the others that are still present are under ifdef #else of either ASIO_HAS_SNPRINTF or ASIO_HAS_SECURE_RTL. I would guess that by using GCC your config doesn't define ASIO_HAS_SNPRINTF because it's checking clang as the compiler. Why ASIO_HAS_SNPRINTF was introduced instead of using ASIO_HAS_SECURE_RTL is unknown to me... Can you try to define ASIO_HAS_SNPRINTF manualy?

stephanlachnit commented 1 month ago

We can't use >=1.30 due to a GCC bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114968), and unfortunately there is one instance of sprintf still present in 1.28 here: https://github.com/chriskohlhoff/asio/blob/54e3a844eb5f31d25416de9e509d9c24fa203c32/asio/include/asio/detail/impl/socket_ops.ipp#L2584

But snprintf is safe. Were you referencing the line 2586?

https://github.com/chriskohlhoff/asio/blob/54e3a844eb5f31d25416de9e509d9c24fa203c32/asio/include/asio/detail/impl/socket_ops.ipp#L2586

Woops, misclick. Yes I meant that line.

It is also present in your search if go expand the match list, or search for sprintf(: https://github.com/search?q=repo%3Achriskohlhoff%2Fasio+sprintf%28&type=code

That line and all the others that are still present are under ifdef #else of either ASIO_HAS_SNPRINTF or ASIO_HAS_SECURE_RTL. I would guess that by using GCC your config doesn't define ASIO_HAS_SNPRINTF because it's checking clang as the compiler. Why ASIO_HAS_SNPRINTF was introduced instead of using ASIO_HAS_SECURE_RTL is unknown to me... Can you try to define ASIO_HAS_SNPRINTF manualy?

We use several OS/compiler combinations, but on MacOS we use clang via brew - we don't set ASIO_HAS_SNPRINTF manually.

But I think the actual solution would be to use sprintf_s like in all the other instances.

stephanlachnit commented 1 month ago

Right, I think I have the issue. We use clang via brew, and this doesn't set the snprintf check correctly that was introduced here: https://github.com/chriskohlhoff/asio/commit/1a2dd21c9865bb3e16829551b5e4c030bd743741

I provided a fix in https://github.com/chriskohlhoff/asio/pull/1510

Shebuka commented 1 month ago

Nice find and fix in PR! 👍