Skip to content

avoid sign extension in char_traits<signed char>::to_int_type - #5336

Open
Angadi56 wants to merge 1 commit into
nlohmann:developfrom
Angadi56:signed-char-to-int-type
Open

avoid sign extension in char_traits<signed char>::to_int_type#5336
Angadi56 wants to merge 1 commit into
nlohmann:developfrom
Angadi56:signed-char-to-int-type

Conversation

@Angadi56

Copy link
Copy Markdown
Contributor

char_traits is specialized here for unsigned char, signed char and std::byte so that byte containers can be handed to the parser, and each specialization widens a character into a 64-bit int_type whose eof() is all-ones. The signed char specialization casts the character straight to int_type, and since every byte above 0x7F is negative in that type, the conversion sign-extends: 0xFF becomes 0xFFFFFFFFFFFFFFFF, which is exactly eof(). So for an input whose element type is signed char, a 0xFF byte is read as end of input and whatever follows it is dropped without an error, which means a document can carry trailing data past a strict check: from_cbor on 01 FF 41 42 43 returns 1 instead of raising parse_error.110, and parse on true followed by 0xFF behaves the same way. Every other byte above 0x7F picks up the same high bits and stops matching its case label, so CBOR 0xF5 is reported as an invalid byte and a plain UTF-8 é is rejected as ill-formed. I noticed it while lining the three specializations up against each other, since the unsigned char one cannot sign-extend and the std::byte one deliberately goes through std::to_integer<unsigned char>, leaving signed char as the only one that widens a negative value. Casting through unsigned char first gives all three the same 0..255 range, and because to_char_type maps the value back with a single cast the round trip is unchanged for every byte.

  • The changes are described in detail, both the what and why.
  • If applicable, an existing issue is referenced.
  • The Code coverage remained at 100%. A test case for every new line of code.
  • If applicable, the documentation is updated.
  • The source code is amalgamated by running make amalgamate.

Signed-off-by: Angadi Yashaswini <angadi@digiscrypt.com>
@nlohmann

This comment was marked as outdated.

@nlohmann nlohmann left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but AppVeyor is complaining about the newly added test code:

unit-type_traits.cpp(69,70): warning C4309: 'static_cast': truncation of constant value
unit-deserialization.cpp(442,447): warning C4309: 'static_cast': truncation of constant value

These are the static_cast<signed char>(0x80), 0xFF, 0xC3, 0xA9 literals - none of those fit in signed char's range (-128..127), so MSVC flags the constant narrowing as an error even through an explicit cast.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants