Repository navigation
fix(LBIE): two more out-of-bounds reads (QUAD refine signs, getGrad) - #763
Merged
Merged
Conversation
Found under AddressSanitizer while fixing the compute_qef crash (#762); neither is that crash. QUAD: Octree::assign_refine_sign_quad visited every edge the surface crosses, including edges on the volume's boundary, where find_oc_id gives -1 for the cells outside the grid. It then called get_err_grad(-1), which reads orig_vol about 18 KB before its start, and read and wrote vtx_idx_arr_refine[-1]. polygonize_quad never makes a quad for such an edge (is_min_edge treats an off-grid neighbour as refined and rejects it), so both loops now skip it. An edge has either all four cells on the grid or two or three off it, so an off-grid cell could never complete the 4-of-4 or 3-of-4 / opposite-pair conditions anyway: only the undefined reads change. Any surface that leaves the volume reached it; CI's QUAD tests use a sphere inside the box. heap-buffer-overflow READ of size 4, 4 bytes before the 18724-byte vtx_idx_arr_refine (4681 octree cells) Octree::assign_refine_sign_quad octree.cpp:6528 Octree::polygonize_quad octree.cpp:5958 getGrad (OPTIMIZATION): t1, d(jacobian)/d(v0) for the edges v1-v0, v2-v0, v3-v0, indexed its second factors 1-based: v1[3], v2[3], v3[3] (z) read past the float[3] arrays, and the z line used [2] for y. With the indices lowered, all three lines equal dJ/dv0 (checked against central differences on 1000 random tetrahedra, max error 8e-10; the old z line was off by up to 4.8). ASan: stack-buffer-overflow in LbieMesherTest.HexaDoMeshOptimize, reading past mmv3 toward mmv4, the point optimization()'s line search fills next (still uninitialized on the first call). That is a likely cause of the NaN vertices arm64 Release left in that test. Tests: - New LbieMesherTest.QuadMeshesASurfaceThatLeavesTheVolume (r = 1.2 sphere, default and fine error). - HexaDoMeshOptimize checks finite coordinates again (expect_geoframe_valid); the arm64 Release lane is the real check of that. - Without the fixes ASan flags both tests in 40/40 runs (OMP_NUM_THREADS 1, 2, 8, default); with them 40/40 clean. lbie_mesher_test 37/37 ASan-clean apart from HexaVerticesStayInsideAVolumeTheSurfaceLeaves, whose read #762 fixes; algorithm_test 10/10 and geometry_test 111/111 clean.
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
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.
Why
AddressSanitizer found two more out-of-bounds reads in the LBIE mesher while I was fixing the compute_qef crash in #762. Neither one is that crash, and the two PRs are independent: they merge cleanly together.
QUAD:
assign_refine_sign_quadindexed cells outside the gridOctree::assign_refine_sign_quadranfind_oc_idon every edge the surface crosses. That includes edges on the volume's boundary, wherexyz2octcellreturns -1 for the cells outside the grid. The function then:get_err_grad(-1), which readsorig_volabout 18 KB before its start, too far away for ASan to flag;vtx_idx_arr_refine[-1], which ASan does flag;vtx_idx_arr_refine[-1] = 1.Any surface that leaves the volume reaches this. CI's QUAD tests never did, because they mesh a sphere inside the box.
OPTIMIZATION:
getGradindexed 1-basedOctree::getGradcomputest1, the derivative of the Jacobian with respect to v0 for the edges v1-v0, v2-v0 and v3-v0. Its second factors were indexed 1-based:v1[3],v2[3]andv3[3]stood for z and read past thefloat[3]arrays. ASan reports this as a stack-buffer-overflow inLbieMesherTest.HexaDoMeshOptimize.[2]where it meant y. That read stays in bounds but computes the wrong quantity.v3[3]reads towardmmv4, the point thatoptimization()'s line search fills next, andmmv4is still uninitialized on the first call. That is a likely cause of the couple of NaN vertices arm64 Release left in that test.What
assign_refine_sign_quad: both of its loops skip an edge that has a cell outside the grid.is_min_edgealready does the same forpolygonize_quad, which never makes a quad for such an edge. An edge has either all four of its cells on the grid or two or three off it, so an off-grid cell could never have completed the 4-of-4 or 3-of-4 / opposite-pair conditions anyway. Only the undefined reads change.getGrad: the three second-factor indices are lowered by one. All threet1lines now equal dJ/dv0: checked against central differences on 1000 random tetrahedra, the maximum error is 8e-10. The old z line was off by up to 4.8.Tests
LbieMesherTest.QuadMeshesASurfaceThatLeavesTheVolumemeshes a sphere of radius 1.2 in its [-1, 1]³ box as QUAD, at the default error and at a fine one.HexaDoMeshOptimizechecks finite coordinates again (expect_geoframe_validinstead of the topology-only check). This PR's macOS arm64 Release lane is the real test of that.lbie_mesher_testis 37/37 clean apart fromHexaVerticesStayInsideAVolumeTheSurfaceLeaves, whose read fix(LBIE): keep getVertGrad on the grid (intermittent crash in compute_qef) #762 fixes;algorithm_test10/10 andgeometry_test111/111, clean.Follow-up