Do not save/restore a pointer across an in-place realloc. - #1925
Merged
Conversation
Contributor
|
clang-tidy review says "All clean, LGTM! 👍" |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
For `p = realloc(p, sz)`, VisitBinaryOperator saved the pre-realloc pointer -- both the primal p and its adjoint _d_p -- to restore it in the reverse sweep. realloc frees the old block, so the restore installed a dangling pointer that the sweep then read and wrote, and the cleanup free() double-freed it. Valgrind reports the Invalid read/write and Invalid free in Gradient/Pointers.C once the clang false positives are suppressed. A growing or same-size realloc preserves the reused contents, so the reallocated pointer stays valid for every reverse access. Detect an in-place realloc -- the LHS is realloc's own pointer argument -- and skip both the primal and the derivative store/restore, leaving p and _d_p in place. A shrinking in-place realloc stays unhandled (its reverse sweep may read the freed tail out of bounds, as before); no test exercises it.
vgvassilev
force-pushed
the
valgrind-realloc
branch
from
July 26, 2026 16:21
4f858fc to
5e4e47c
Compare
Contributor
|
clang-tidy review says "All clean, LGTM! 👍" |
Contributor
|
clang-tidy review says "All clean, LGTM! 👍" |
vgvassilev
added a commit
that referenced
this pull request
Jul 31, 2026
Since #1925 an in-place `p = realloc(p, n)` keeps the reallocated pointer for both p and its adjoint instead of saving and restoring it, because realloc frees the old block and a saved pointer would dangle. That is correct only when the buffer grows or keeps its size: a shrinking realloc leaves the reverse sweep addressing a buffer smaller than the indices the forward pass wrote, reading past its end. Track each differentiated pointer's allocation size in a size_t shadow, set at malloc/calloc/realloc. At an in-place realloc, save the pre-realloc size in the forward pass and, in the reverse sweep, call clad::reverse_realloc to resize both the primal and adjoint buffers back to it so the pre-realloc accesses stay in bounds; the adjoint's re-grown tail is zeroed so fresh derivatives start at 0. Without a tracked size the previous keep-the-pointer behaviour still applies. Add Gradient/ReallocShrink.C, which is Memcheck-clean under the valgrind row only with this change; update Gradient/Pointers.C for the shadows.
vgvassilev
added a commit
that referenced
this pull request
Jul 31, 2026
Since #1925 an in-place `p = realloc(p, n)` keeps the reallocated pointer for both p and its adjoint instead of saving and restoring it, because realloc frees the old block and a saved pointer would dangle. That is correct only when the buffer grows or keeps its size: a shrinking realloc leaves the reverse sweep addressing a buffer smaller than the indices the forward pass wrote, reading past its end. Track each differentiated pointer's allocation size in a size_t shadow, set at malloc/calloc/realloc. At an in-place realloc, save the pre-realloc size in the forward pass and, in the reverse sweep, call clad::reverse_realloc to resize both the primal and adjoint buffers back to it so the pre-realloc accesses stay in bounds; the adjoint's re-grown tail is zeroed so fresh derivatives start at 0. Without a tracked size the previous keep-the-pointer behaviour still applies. Add Gradient/ReallocShrink.C, which is Memcheck-clean under the valgrind row only with this change; update Gradient/Pointers.C for the shadows.
vgvassilev
added a commit
that referenced
this pull request
Aug 1, 2026
Since #1925 an in-place `p = realloc(p, n)` keeps the reallocated pointer for both p and its adjoint instead of saving and restoring it, because realloc frees the old block and a saved pointer would dangle. That is correct only when the buffer grows or keeps its size: a shrinking realloc leaves the reverse sweep addressing a buffer smaller than the indices the forward pass wrote, reading past its end. Track the pointer's allocation size in a size_t shadow set at malloc/calloc/realloc. At an in-place realloc, capture the pre- and post-realloc sizes in the forward pass and, in the reverse sweep, call clad::reverse_realloc to resize both the primal and adjoint buffers back to the pre-realloc size so the earlier accesses stay in bounds; the adjoint's re-grown tail is zeroed so fresh derivatives start at 0. Each realloc captures its own sizes, so chained reallocs unwind correctly. Only pointers actually reallocated in place need the shadow, so DiffCollector records them on the DiffRequest during planning -- reusing the traversal it already runs -- and reverse mode shadows exactly those, leaving every other allocation untouched. The shadow rides on the pointer's existing adjoint record rather than a separate map. Without a tracked size the previous keep-the-pointer behaviour still applies. Add Gradient/ReallocShrink.C, covering a shrinking and a chained shrink-then-grow realloc, Memcheck-clean under the valgrind row only with this change; update Gradient/Pointers.C for the shadows.
vgvassilev
added a commit
that referenced
this pull request
Aug 1, 2026
Since #1925 an in-place `p = realloc(p, n)` keeps the reallocated pointer for both p and its adjoint instead of saving and restoring it, because realloc frees the old block and a saved pointer would dangle. That is correct only when the buffer grows or keeps its size: a shrinking realloc leaves the reverse sweep addressing a buffer smaller than the indices the forward pass wrote, reading past its end. Track the pointer's allocation size in a size_t shadow set at malloc/calloc/realloc. At an in-place realloc, capture the pre- and post-realloc sizes in the forward pass and, in the reverse sweep, call clad::reverse_realloc to resize both the primal and adjoint buffers back to the pre-realloc size so the earlier accesses stay in bounds; the adjoint's re-grown tail is zeroed so fresh derivatives start at 0. Each realloc captures its own sizes, so chained reallocs unwind correctly. Only pointers actually reallocated in place need the shadow, so DiffCollector records them on the DiffRequest during planning -- reusing the traversal it already runs -- and reverse mode shadows exactly those, leaving every other allocation untouched. The shadow rides on the pointer's existing adjoint record rather than a separate map. Without a tracked size the previous keep-the-pointer behaviour still applies. Add Gradient/ReallocShrink.C, covering a shrinking and a chained shrink-then-grow realloc, Memcheck-clean under the valgrind row only with this change; update Gradient/Pointers.C for the shadows.
vgvassilev
added a commit
that referenced
this pull request
Aug 1, 2026
Since #1925 an in-place `p = realloc(p, n)` keeps the reallocated pointer for both p and its adjoint instead of saving and restoring it, because realloc frees the old block and a saved pointer would dangle. That is correct only when the buffer grows or keeps its size: a shrinking realloc leaves the reverse sweep addressing a buffer smaller than the indices the forward pass wrote, reading past its end. Track the pointer's allocation size in a size_t shadow set at malloc/calloc/realloc. At an in-place realloc, capture the pre- and post-realloc sizes in the forward pass and, in the reverse sweep, call clad::reverse_realloc to resize both the primal and adjoint buffers back to the pre-realloc size so the earlier accesses stay in bounds; the adjoint's re-grown tail is zeroed so fresh derivatives start at 0. Each realloc captures its own sizes, so chained reallocs unwind correctly. Only pointers actually reallocated in place need the shadow, so DiffCollector records them on the DiffRequest during planning -- reusing the traversal it already runs -- and reverse mode shadows exactly those, leaving every other allocation untouched. The shadow rides on the pointer's existing adjoint record rather than a separate map. Without a tracked size the previous keep-the-pointer behaviour still applies. Add Gradient/ReallocShrink.C, covering a shrinking and a chained shrink-then-grow realloc, Memcheck-clean under the valgrind row only with this change; update Gradient/Pointers.C for the shadows.
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.
For
p = realloc(p, sz), VisitBinaryOperator saved the pre-realloc pointer -- both the primal p and its adjoint _d_p -- to restore it in the reverse sweep. realloc frees the old block, so the restore installed a dangling pointer that the sweep then read and wrote, and the cleanup free() double-freed it. Valgrind reports the Invalid read/write and Invalid free in Gradient/Pointers.C once the clang false positives are suppressed.A growing or same-size realloc preserves the reused contents, so the reallocated pointer stays valid for every reverse access. Detect an in-place realloc -- the LHS is realloc's own pointer argument -- and skip both the primal and the derivative store/restore, leaving p and _d_p in place. A shrinking in-place realloc stays unhandled (its reverse sweep may read the freed tail out of bounds, as before); no test exercises it.