gty404 commented on code in PR #1022:
URL: https://github.com/apache/iceberg-cpp/pull/1022#discussion_r4290254464
##########
src/iceberg/avro/avro_direct_decoder.cc:
##########
@@ -160,26 +160,32 @@ Status DecodeStructToBuilder(const ::avro::NodePtr&
avro_node, ::avro::Decoder&
// Build a map from Avro field index to projection index (cached per struct
schema)
// -1 means the field should be skipped
- const FieldProjection* cache_key = projections.data();
+ // Empty projections all share one data() pointer, so the Avro node is part
of the key.
+ const DecodeContext::ProjectionCacheKey cache_key{projections.data(),
avro_node.get()};
Review Comment:
Good point, thanks. The out-of-bounds read depends on what the heap happens
to contain, so the end-to-end test is only reliable under ASAN. I added
`DirectDecoderTest.ProjectionCacheKeyedPerAvroNodeForEmptyStructs`, which calls
`DecodeAvroToBuilder` directly and asserts the cache has one entry per Avro
node with sizes matching each node's `leaves()` (1, 6 and the root's 2). Note
that without the fix the decode itself still reads out of bounds before the
assertion is reached, so it fails by crash or by assertion depending on the
platform, but it can no longer pass silently. Done in a419c023.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
