You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Root cause: Two thread-safety bugs in utils/geom.c, exposed by Meep's recently added OMP parallization of set_chi1inv (NanoComp/meep#3166).
Fix 1 (primary — caused the crash of test_mode_coeffs.py): Remove the geom_fix_object_ptr(&o) call from geom_get_bounding_box(). For Prism objects, this call triggered reinit_prism(), which frees and re-mallocs all internal prism arrays. When multiple OMP threads call this concurrently on the same Prism, it causes double-free/use-after-free, corrupting heap metadata and producing the free(): unaligned chunk detected in tcache 2 error.
Fix 2 (secondary — would cause incorrect results): Change intersect_line_segment_with_prism() to use a stack-allocated variable-length array (VLA) (double slist[num_vertices + 2]) instead of the shared prsm->workspace.items buffer. This prevents data races during concurrent intersection calculations.
Verification: all 6 tests in Meep's test_mode_coeffs.py now pass with OMP_NUM_THREADS=1, 2, and 4.
Fix 1 (primary — caused the crash of test_mode_coeffs.py): Remove the geom_fix_object_ptr(&o) call from geom_get_bounding_box(). For Prism objects, this call triggered reinit_prism(), which frees and re-mallocs all internal prism arrays. When multiple OMP threads call this concurrently on the same Prism, it causes double-free/use-after-free, corrupting heap metadata and producing the free(): unaligned chunk detected in tcache 2 error.
This changes the semantics. But it's probably a good idea anyway since fix_object is expensive and should really only be done once at the beginning of all geometry calculations, and in fact we are already doing this.. (In Meep, it is called in the geom_epsilon constructor, and in MPB and pympb it is called in init_epsilon.
Change intersect_line_segment_with_prism() to use a stack-allocated variable-length array (VLA) (double slist[num_vertices + 2]) instead of the shared prsm->workspace.items buffer.
This is potentially dangerous for a large prism because stack space is limited. In particular, it could crash for prisms more than about 100,000 elements.
For the prism list, in practice the slist normally doesn't need to be equal to the number of vertices or anything close, since it is very unlikely you will have so many intersections. What we could do to make it both thread-safe and memory-safe would be something like:
Stack allocate a moderate number of intersections, e.g. double slist_stack[1024]; double *slist = slist_stack;, that will easily fit in the stack.
In intersect_line_with_prism, add an extra argument int slist_len for the length of slist (passing 1024)
Change the line from slist[num_intersections++] = s; to something like if (num_intersections++ < slist_len) slist[num_intersections-1] = s. Then, at the end of that loop, add a check if (num_intersections > slist_len) return num_intersections; to return immediately
In the caller intersect_line_segment_with_prism, check if (num_intersections > 1024) { slist = (double *) malloc(num_intersections * sizeof(double)); num_intersections = intersect_line_with_prism(prsm, pc, dc, slist, num_intersections); } to re-do the intersect_line_with_prism calculation with a heap-allocated array, as a slow fallback path. At the end of the function, also add if (num_intersections > 1024) free(slist); to deallocated it in this case.
Get rid of prsm->workspace, which is no longer used.
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
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.
Root cause: Two thread-safety bugs in
utils/geom.c, exposed by Meep's recently added OMP parallization ofset_chi1inv(NanoComp/meep#3166).Fix 1 (primary — caused the crash of
test_mode_coeffs.py): Remove thegeom_fix_object_ptr(&o)call fromgeom_get_bounding_box(). For Prism objects, this call triggeredreinit_prism(), which frees and re-mallocs all internal prism arrays. When multiple OMP threads call this concurrently on the same Prism, it causes double-free/use-after-free, corrupting heap metadata and producing thefree(): unaligned chunk detected in tcache 2error.Fix 2 (secondary — would cause incorrect results): Change
intersect_line_segment_with_prism()to use a stack-allocated variable-length array (VLA) (double slist[num_vertices + 2]) instead of the sharedprsm->workspace.itemsbuffer. This prevents data races during concurrent intersection calculations.Verification: all 6 tests in Meep's
test_mode_coeffs.pynow pass withOMP_NUM_THREADS=1,2, and4.@Luochenghuang