[C++, JSON] Fix nullptr access when reading a key with a default value. #6375
Conversation
github-actions
bot
added
c#
c++
codegen
vglavnyy
force-pushed
the
fixes_for_sorted_by_key
branch
from
a6f6490 to
e308a1d
Compare
|
@mustiikhalil thank you. |
|
CI failed with PHP and with clang-format. Both are not related to the changes. |
src/idl_parser.cpp
Outdated
| ECHECK(atot(field_value.constant.c_str(), *this, &val)); \ | ||
| ECHECK(atot(field->value.constant.c_str(), *this, &valdef)); \ | ||
| builder_.AddElement(field_value.offset, val, valdef); \ | ||
| if(field->key) \ |
There was a problem hiding this comment.
I don't understand how this fixes it.. this may fix it for JSON data but the problem is still there when using binary data created elsewhere?
I assumed that the fix had to be in the actual sorting code.
There was a problem hiding this comment.
This problem arises in Parser::ParseVector only.
Binary data never use SimpleQsort.
CreateVectorOfSortedTables and CreateVectorOfSortedStructs use std::sort with comparators.
https://github.com/google/flatbuffers/blob/0800976533ac2b54790635a5eef8b5d9dbc5e8eb/include/flatbuffers/flatbuffers.h#L1982-L-1987.
But you are right, the Parser::ParseTable isn't a good place for this fix.
There was a problem hiding this comment.
I think key fields should be implicitly declared as required fields (both string and scalars).
At least it should be required if a struct or a table is used inside Vector container.
We can set it as required in FBS-2.0.
There was a problem hiding this comment.
I moved key checking into Parser::ParseVector.
CI failure doesn't connect with these changes.
There was a problem hiding this comment.
Yes, key implying required makes a lot of sense for strings. For scalars, it shouldn't really matter, though if it simplifies our key lookup, let's do it.
vglavnyy
force-pushed
the
fixes_for_sorted_by_key
branch
from
818e9e3 to
1a6e42e
Compare
|
Is this otherwise ready to land? |
This commit fixes handling of default and NULL `key` fields in `Parser::ParseVector` (google#5928). The JSON generator updated. It outputs `key` fields even if the `--force-defaults` option is inactive. Additional test cases for `key` added.
vglavnyy
force-pushed
the
fixes_for_sorted_by_key
branch
from
1a6e42e to
6a740a9
Compare
|
It is ready. |
|
Thanks! |
3 participants
This commit improves the workaround for #5928.
--force-defaultswhen a key field is set to its default value.keyadded.