fix(update): correct scalar-broadcast for GUID/STR and all column widths#252
Merged
Merged
Conversation
ray_update broadcast a scalar atom into a full column through a fixed 8-byte `elem` buffer with a (ct==BOOL?1:8) stride. For a GUID column (16-byte payload, stored in ->obj) ray_vec_append then read 16 bytes from the 8-byte stack buffer — an ASan stack-buffer-overflow / crash — and it also copied from the wrong source field (->i64 instead of ->obj). The narrow-int and temporal types only worked by relying on union aliasing on little-endian. All three broadcast sites (WHERE, all-rows, and the BY new-column path) now use a 16-byte buffer, copy ray_elem_size(ct) bytes, and source GUID payloads from ->obj. The BY new-column site also gained the STR handling the other two already had, so a string-valued new column no longer copies garbage. alter (store_typed_elem) and upsert (append_atom_to_col + ray_elem_size copy loop, fixed earlier in this branch) already handle every type. Added update-broadcast regression tests covering I32/I16/Date/Timestamp type preservation and the GUID overflow (all-rows and WHERE).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #250, which merged before this commit was pushed — so the
updateportion of that audit never landed. This carries it forward unchanged onto current master.Problem
ray_updatebroadcasts a scalar atom into a full column through a fixed 8-byteelemstack buffer with a(ct==BOOL)?1:8stride. For a GUID column (16-byte payload, stored in->obj)ray_vec_appendthen reads 16 bytes from the 8-byte buffer — an ASan stack-buffer-overflow / crash — and it also copies from the wrong source field (->i64instead of->obj). The narrow-int and temporal types only worked by relying on union aliasing on little-endian.Repro (crashes under ASan before the fix):
Fix
All three broadcast sites (WHERE, all-rows, and the BY new-column path) now use a 16-byte buffer, copy
ray_elem_size(ct)bytes, and source GUID payloads from->obj. The BY new-column site also gained the STR handling the other two already had, so a string-valued new column no longer copies garbage.alter(store_typed_elem) andupsert/insert(handled in #250) already cover every type.Tests
Added update-broadcast regression tests to
table/update.rfl: I32/I16/Date/Timestamp type preservation, plus the GUID overflow (all-rows and WHERE).Full suite green (
make test, 0 failed).