tristanpenman / valijson

Header-only C++ library for JSON Schema validation, with support for many popular parsers
BSD 2-Clause "Simplified" License
351 stars 105 forks source link

Compiling with GCC12 spells a warning because of -Wstringop-overread #166

Closed ilario-gottardello closed 2 years ago

ilario-gottardello commented 2 years ago

Hello, I am using valijson bundled with qtjson, and compiling with GCC12 I got a warning like this:

In function ‘move’, inlined from ‘_S_move’ at /usr/include/c++/12/bits/basic_string.h:433:21, inlined from ‘_M_replace’ at /usr/include/c++/12/bits/basic_string.tcc:531:22, inlined from ‘replace’ at /usr/include/c++/12/bits/basic_string.h:2171:19, inlined from ‘extractReferenceToken’ at /home/.../valijson/src/valijson_qtjson.h:2713:26, inlined from ‘resolveJsonPointer’ at /home/.../valijson/src/valijson_qtjson.h:2781:23: /usr/include/c++/12/bits/char_traits.h:419:57: warning: ‘builtin_memmove’ reading 2 or more bytes from a region of size 1 [-Wstringop-overread] 419 | return static_cast<char_type*>(__builtin_memmove(s1, s2, n)); | ^ /home/.../valijson/src/valijson_qtjson.h: In function ‘resolveJsonPointer’: /home/.../valijson/src/valijson_qtjson.h:2712:24: note: source object ‘c’ of size 1 2712 | const char c = decodePercentEncodedChar(token.substr(n + 1, 2)); | ^

The warning is emitted because in function extractReferenceToken, at line:

token.replace(n, 3, &c, 1);

I think replace expect &c to be a C string, so lasting with 0. But it is not, as it is a char. I modified the code like this:

token.replace(n, 3, 1, c);

I think it should fix the issue. I tried on my project and it seems to work.

Thank you very much Have a nice day

tristanpenman commented 2 years ago

Thanks for looking into this. While it seems like the old version should work, it is easy to imagine that this was relying on undefined behaviour. I changed the code to use the overload of replace that you have suggested. This is in 78ac8a7.

tristanpenman commented 2 years ago

Closing this for now, as this change should address the issue. Feel free to re-open if the warning still occurs.