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

compiler error: directive output may be truncated writing between 2 and 8 bytes #2572

Closed
1 task done
stephanecharette opened this issue Jan 7, 2021 · 16 comments · Fixed by #2874
Closed
1 task done
Assignees
Labels
release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation

Comments

@stephanecharette
Copy link

When attempting to build on a 64-bit ARM device (NVIDIA Jetson Xavier NX) I'm getting the following compiler error:

src-json/json.hpp: In member function ‘void nlohmann::detail::serializer<BasicJsonType>::dump_escaped(const string_t&, bool) [with BasicJsonType = nlohmann::basic_json<>]’:
src-json/json.hpp:15935:10: error: ‘%.2X’ directive output may be truncated writing between 2 and 8 bytes into a region of size 3 [-Werror=format-truncation=]
     void dump_escaped(const string_t& s, const bool ensure_ascii)
          ^~~~~~~~~~~~
src-json/json.hpp:15935:10: note: directive argument in the range [0, 2147483647]
In file included from /usr/include/stdio.h:862:0,
                 from /usr/include/c++/7/cstdio:42,
                 from /usr/include/c++/7/ext/string_conversions.h:43,
                 from /usr/include/c++/7/bits/basic_string.h:6361,
                 from /usr/include/c++/7/string:52,
                 from /usr/include/c++/7/bits/locale_classes.h:40,
                 from /usr/include/c++/7/bits/ios_base.h:41,
                 from /usr/include/c++/7/ios:42,
                 from /usr/include/c++/7/istream:38,
                 from /usr/include/c++/7/fstream:38,
                 from src-common/Project.hpp:11,
                 from src-config/Config.cpp:3:
/usr/include/aarch64-linux-gnu/bits/stdio2.h:65:44: note: ‘__builtin___snprintf_chk’ output between 3 and 9 bytes into a destination of size 3
        __bos (__s), __fmt, __va_arg_pack ());
                                            ^
cc1plus: all warnings being treated as errors

What is the issue you have?

I'm using the "single include" updated a few days ago with the year update, 085d497

I see this problem has been reported in the past, but as a side issue in another ticket, and without resolution: #1117.

Build with the following g++ flags on ARM device:

ADD_DEFINITIONS ("-Wall -Wextra -Werror -Wno-unused-parameter")

Including json.hpp causes our build to fail. Will need to setup exceptions around the json include file.

Which compiler and operating system are you using?

> g++ --version
g++ (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0

> cat /etc/lsb-release 
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=18.04
DISTRIB_CODENAME=bionic
DISTRIB_DESCRIPTION="Ubuntu 18.04.5 LTS"

This is the standard NVIDIA image for the Jetson Xavier NX device.

Which version of the library did you use?

  • latest release version 3.9.1
@stephanecharette
Copy link
Author

Workaround for now:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wformat-truncation"
#include "json.hpp"
#pragma GCC diagnostic pop

@gregmarr
Copy link
Contributor

gregmarr commented Jan 7, 2021

I think this is evaluating this code and not taking into account that the max value of byte is only 255.

std::string sn(3, '\0');
(std::snprintf)(&sn[0], sn.size(), "%.2X", byte);

@stephanecharette
Copy link
Author

Probably. Note I'm happy with the workaround I posted a few hours ago if you're sure there isn't a bug. Other than that 1 compiler issue, everything seems to be working fine, I've already got it parsing and outputting json files.

@nlohmann nlohmann removed the kind: bug label Jan 8, 2021
@nlohmann
Copy link
Owner

nlohmann commented Jan 8, 2021

I don't see anything we can do here:

  • %.2X, means we want to print at least 2 digits.
  • The value type std::uint8_t, so the maximal value is 255, or 0xFF.

Therefore, we will print exactly two digits, and a buffer of size 3 is sufficient. So the warning is a false positive, because the output will never be truncated.

However, as the code is only called in case of an exception (i.e., it is not performance critical), we may refactor it to silence the warning. Could it make sense to change std::string sn(3, '\0'); to std::string sn(8, '\0'); here?

@stephanecharette
Copy link
Author

Also see: https://stackoverflow.com/a/57780456/13022 where using % can tell the compiler that the range of value is limited.

But like I said, I'm happy with my current workaround.

@nlohmann
Copy link
Owner

nlohmann commented Jan 8, 2021

As I never saw this warning (though testing with GCC 11 and -Wformat-truncation=2), could you please try if it disappears when changing

(std::snprintf)(&sn[0], sn.size(), "%.2X", byte);

to

(std::snprintf)(&sn[0], sn.size(), "%.2X", byte % 256);

?

@stephanecharette
Copy link
Author

I went to try your proposed fix...and I cannot reproduce anymore. I'm very confused. I've changed quite a lot in the project over the last few days, added several Boost libraries and other things, so our CMake files have also changed considerably. I tried to pare it down to see if I can get it to happen again, but I'm sorry Niels, I don't know exactly how to reproduce the error any more.

Regardless, thanks for the great JSON C++ library.

@nlohmann
Copy link
Owner

nlohmann commented Jan 9, 2021

You're welcome!

@nlohmann nlohmann closed this as completed Jan 9, 2021
@nlohmann nlohmann added the solution: invalid the issue is not related to the library label Jan 9, 2021
@stephanecharette
Copy link
Author

So this just happened again! I figured out how to get it to repro. In debug mode, I'm not seeing it. Only when I make a release mode builds do I see this error.

I tried several different ways to fix it, first with "% 256" and "(byte % 256)", but was not successful.

In the end, I've gone back to my workaround as described in my 2nd comment.

@gregmarr
Copy link
Contributor

I think changing 3 to 8 in the size is probably fine. It is still within the small buffer optimization of MSVC, libc++ and libstdc++ and so should not trigger an allocation.

@nlohmann nlohmann reopened this Jan 15, 2021
@nlohmann nlohmann removed the solution: invalid the issue is not related to the library label Jan 15, 2021
@nlohmann nlohmann self-assigned this Jan 15, 2021
@nlohmann
Copy link
Owner

Can you please try if the error is gone in branch https://github.com/nlohmann/json/tree/issue2572?

@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Jan 15, 2021
@stephanecharette
Copy link
Author

I get exactly the same error with the original json.hpp, and with the one in that branch:

In file included from /home/stephane/src/Charlotte/src-common/Hummingbirds.hpp:48:0,
                 from /home/stephane/src/Charlotte/src-config/hai_Config.cpp:3:
/home/stephane/src/Charlotte/src-json/json.hpp: In member function ‘void nlohmann::detail::serializer<BasicJsonType>::dump_escaped(const string_t&, bool) [with BasicJsonType = nlohmann::basic_json<>]’:
/home/stephane/src/Charlotte/src-json/json.hpp:15935:10: error: ‘__builtin___snprintf_chk’ output may be truncated before the last format character [-Werror=format-truncation=]
     void dump_escaped(const string_t& s, const bool ensure_ascii)
          ^~~~~~~~~~~~
In file included from /usr/include/stdio.h:862:0,
                 from /usr/include/c++/7/cstdio:42,
                 from /usr/include/c++/7/ext/string_conversions.h:43,
                 from /usr/include/c++/7/bits/basic_string.h:6361,
                 from /usr/include/c++/7/string:52,
                 from /usr/include/c++/7/bits/locale_classes.h:40,
                 from /usr/include/c++/7/bits/ios_base.h:41,
                 from /usr/include/c++/7/ios:42,
                 from /usr/include/c++/7/istream:38,
                 from /usr/include/c++/7/fstream:38,
                 from /home/stephane/src/Charlotte/src-common/Hummingbirds.hpp:15,
                 from /home/stephane/src/Charlotte/src-config/hai_Config.cpp:3:
/usr/include/aarch64-linux-gnu/bits/stdio2.h:65:44: note: ‘__builtin___snprintf_chk’ output between 3 and 9 bytes into a destination of size 8
        __bos (__s), __fmt, __va_arg_pack ());
                                            ^

I see the following changes when I diff the old and new file:

> diff ../src-json/json.hpp*
16055c16055
<                             std::string sn(8, '\0');
---
>                             std::string sn(3, '\0');
16149c16149
<                     std::string sn(8, '\0');
---
>                     std::string sn(3, '\0');
25598d25597
< 

@nlohmann
Copy link
Owner

Hm. Maybe I should set it to 9 bytes - I overlooked that number in the previous warnings.

@nlohmann
Copy link
Owner

I updated the branch to use 9 bytes to address

output between 3 and 9 bytes into a destination of size 8

@stephanecharette Can you please try again?

@gardner-bp
Copy link

I ran into the exact same issue with g++ on 64-bit ARM Nvidia Jetson TX2.
I can confirm that the 9 byte modification makes this release build warning disappear.
The warning doesn't appear for debug builds either way.

nlohmann added a commit that referenced this issue Jul 15, 2021
@nlohmann nlohmann added this to the Release 3.9.2 milestone Jul 15, 2021
harry75369 pushed a commit to harry75369/json that referenced this issue Aug 8, 2021
@jmccabe
Copy link

jmccabe commented Nov 10, 2021

I don't see anything we can do here:

  • %.2X, means we want to print at least 2 digits.
  • The value type std::uint8_t, so the maximal value is 255, or 0xFF.

Therefore, we will print exactly two digits, and a buffer of size 3 is sufficient. So the warning is a false
positive, because the output will never be truncated.

Sorry to come a bit late to this, also sorry if I'm missing something, but...

Am I right in thinking that you have, nominally, a byte (8-bits) that you want to represent as a 2-digit, zero-prefixed, hex value stored into a 3-character string?

You say, correctly, that "%.2X" means you want to print at least 2 digits which, based on the "." will give you results that are prefixed with "0" if the value can be represented in one character (i.e. value 0 to F hex will result in 00 to 0F), which, if my assumption is correct, is what you want to achieve.

I'm slightly confused by the "want to print at least 2 digits"; why not "want to print 2 digits"? In which case, wouldn't:

std::string sn(3, '\0');
(std::snprintf)(&sn[0], sn.size(), "%02X", byte);

have the desired effect rather than going down the route of making the string longer for no real reason other than to work round what looks like an issue with the compiler (to me - who's having the same problem).

If I change that line to %02X I can compile with gcc 7.5 (ish) for ARM (32-bit in our case) without the format-truncation warning.

I realise that this issue is closed, but I wondered if, perhaps, the intention would be clearer by using %02X with a 3 character string than %.2X with a 9 character string.

(Mmm - maybe I should've checked that this still exists in the most recent code; I see this has changed to:

std::stringstream ss;
ss << std::uppercase << std::setfill('0') << std::setw(2) << std::hex << (byte | 0);
JSON_THROW(type_error::create(316, "invalid UTF-8 byte at index " + std::to_string(i) + ": 0x" + ss.str(), BasicJsonType()));

)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants