Fix JSON parsing memory corruption - Fix Mixed types nested children removal #15798
Conversation
karthikeyann
added
bug
karthikeyann
added
3 - Ready for Review
cpp/src/io/json/json_column.cu
Outdated
| @@ -594,8 +594,7 @@ void make_device_json_column(device_span<SymbolT const> input, | |||
| col.validity = | |||
| cudf::detail::create_null_mask(col.num_rows, cudf::mask_state::ALL_NULL, stream, mr); | |||
| col.type = json_col_t::StringColumn; | |||
| col.child_columns.clear(); // their references should be deleted too. | |||
| col.column_order.clear(); | |||
| // destroy references of all child columns after | |||
There was a problem hiding this comment.
Sorry, I'm not understanding this comment.
Destroy references of all child columns after what?
There was a problem hiding this comment.
This is my only comment about this PR as well. Happy to approve once this is fixed.
There was a problem hiding this comment.
'after this step'.
| cudf::io::json_reader_options::builder(cudf::io::source_info{json_str.c_str(), json_str.size()}) | ||
| .lines(true) | ||
| .recovery_mode(cudf::io::json_recovery_mode_t::RECOVER_WITH_NULL) | ||
| .normalize_single_quotes(true) |
There was a problem hiding this comment.
Nit: Since we do not have single quotes in the input string, do we need to enable single quotes normalization here? Another minor nit is that we can remove normalize_whitespace call since whitespace normalization is disabled by default.
There was a problem hiding this comment.
Right, those options are not required. I used the same options as repro code for the bug, but I reduced the input json to minimal.
| ASSERT_EQ(result.metadata.schema_info[0].children[0].children.size(), 2); | ||
| EXPECT_EQ(result.metadata.schema_info[0].children[0].children[0].name, "offsets"); | ||
| // types | ||
| EXPECT_EQ(result.tbl->get_column(0).type().id(), cudf::type_id::STRUCT); |
There was a problem hiding this comment.
Question: Can type checks also be ASSERT_EQ? I think the types condition must hold since the goal of the test is to ensure correct mixed types behaviour,
There was a problem hiding this comment.
The EXPECT_EQ will fail the test as appropriate.
The difference is that ASSERT_EQ will abort the test which should really only be used when the remaining code may fail/segfault.
I think the usage of EXPECT_EQ and ASSERT_EQ are correctly implemented here.
There was a problem hiding this comment.
Makes sense, thank you for the explanation!
|
/merge |
Description
Fixes #15750
The references of deleted child columns are not removed, which caused segfault, and also memory errors (found with valgrind). This fix removes references of child columns and deletes them recursively.
Checklist