Guard against Giac RUR crashes on large ideals - #154
Conversation
|
Sorry for the noise. I think the underlying issue is valid, but the implementation looks pretty botched. 😬. I should have looked more carefully before submitting. |
b88d364 to
9a39073
Compare
9a39073 to
8a24cef
Compare
|
Much more clean now, ready for review. |
|
I could find no reference confirming this issue in |
|
Thanks @NathanDunfield for looking into this. For the reproducer, please see the original PR body. Documentation of the bug itself is pretty sparse, and v2 is the exact cutover for the releases. Below is more detail courtesy of my agent. Fair challenge — I'd only observed the crash, I hadn't traced it. It's a real upstream memory bug in giac, fixed by Parisse in r70415 / geogebra/giac@4b33d76 (2025-04-26, "fix memory bug in gbasis/rur code reported by Zoltan"). There's no issue to link: giac has no public tracker — SourceForge exposes only code/discussion/mailman, and Reproducer without Sage or SnapPy, 4 lines of giac: On conda-forge Mechanism. Version boundary. I extracted I also checked the two other look-alike |
Summary
Giac versions before 2.0 corrupt memory when asked for a rational univariate
representation of an ideal in more than 15 variables. This change checks that
condition before calling Giac and raises a specific
ValueErrorcontaining thedetected Giac version and the actual number of variables.
The closed extended-Ptolemy caller catches that specific exception and reports
the limitation in terms of the triangulation. Its ideal has
2 * num_tetrahedra + 4variables, so with an affected Giac version thecalculation is limited to triangulations with at most five tetrahedra.
Giac 2.0 and later are not subject to the check and continue through the
existing RUR implementation.
Rationale
The RUR wrapper accepts arbitrary polynomial ideals and has non-Ptolemy
callers, so the generic error remains phrased in terms of variables. Only the
caller that has the manifold available translates the failure into
tetrahedron-specific language.
This deliberately does not modify the ideal, eliminate coordinates, or suggest
an in-place Giac upgrade that the surrounding Sage dependency set might not
support.
Reproduction
The following Dockerfile installs the affected Sage, Giac, and released SnapPy
versions and runs the calculation on the six-tetrahedron triangulation of
6_3(1,1):Build and run it:
The container exits nonzero after reporting the triangulation size and Giac's
allocator failure:
After this PR merges, the expected output of the above, when run with a SnapPy
release containing the change rather than the deliberately pinned 3.3.2
reproducer, is:
Testing
giac_rur.pydoctests: 17 passed.exception before Giac is invoked.
6_3(1,1): the closed-Ptolemy path reports six tetrahedra,16 variables, and the five-tetrahedron limit.
6_3(1,1)ideal: the 16-variable RURcompletes with component degrees
[6, 22]and multiplicities[1, 1].