Skip to content

Guard against Giac RUR crashes on large ideals - #154

Open
maresb wants to merge 1 commit into
3-manifolds:masterfrom
maresb:fix-giac-rur-variable-limit
Open

Guard against Giac RUR crashes on large ideals#154
maresb wants to merge 1 commit into
3-manifolds:masterfrom
maresb:fix-giac-rur-variable-limit

Conversation

@maresb

@maresb maresb commented Jul 30, 2026

Copy link
Copy Markdown

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 ValueError containing the
detected 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 + 4 variables, so with an affected Giac version the
calculation 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):

# syntax=docker/dockerfile:1
FROM mambaorg/micromamba:2.8.1

RUN micromamba install --yes --name base --channel conda-forge \
        python=3.12 \
        sage=10.8 \
        giac=1.9.0.21 \
        pip \
    && micromamba clean --all --yes

ARG MAMBA_DOCKERFILE_ACTIVATE=1
RUN python -m pip install --no-cache-dir snappy==3.3.2

COPY --chown=$MAMBA_USER:$MAMBA_USER <<'PY' /tmp/reproduce.py
import snappy
from snappy.dev.extended_ptolemy import complexVolumesClosed
from snappy.dev.extended_ptolemy import extended

M = snappy.ManifoldHP('6_3')
M.dehn_fill((1, 1))
I = extended.ptolemy_ideal_for_filled(M)

print('tetrahedra:', M.num_tetrahedra(), flush=True)
print('variables:', len(I.ring().gens()), flush=True)
complexVolumesClosed.compute_representative_ptolemys_and_full_var_dict(M, 120)
PY

CMD ["python", "/tmp/reproduce.py"]

Build and run it:

docker build -t snappy-giac-rur .
docker run --rm snappy-giac-rur

The container exits nonzero after reporting the triangulation size and Giac's
allocator failure:

tetrahedra: 6
variables: 16
...
before (last 100 chars): b'sage3:=gbasis(sage0,sage1,sage2):;\r\nmalloc(): unaligned tcache chunk detected\r\n'
...
Giac crashed executing sage3:=gbasis(sage0,sage1,sage2):;

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:

tetrahedra: 6
variables: 16
Traceback (most recent call last):
...
ValueError: The extended Ptolemy ideal for this 6-tetrahedron triangulation has 16 variables. Giac 1.9.0's RUR implementation crashes for more than 15 variables, so this Giac version limits this computation to triangulations with at most 5 tetrahedra.

Testing

  • Existing giac_rur.py doctests: 17 passed.
  • Giac 1.9: a 15-variable ideal completes; a 16-variable ideal raises the new
    exception before Giac is invoked.
  • Giac 1.9 with 6_3(1,1): the closed-Ptolemy path reports six tetrahedra,
    16 variables, and the five-tetrahedron limit.
  • Giac 2.0.0.19 with the unchanged 6_3(1,1) ideal: the 16-variable RUR
    completes with component degrees [6, 22] and multiplicities [1, 1].

@maresb
maresb marked this pull request as draft July 30, 2026 13:18
@maresb

maresb commented Jul 30, 2026

Copy link
Copy Markdown
Author

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.

@maresb
maresb force-pushed the fix-giac-rur-variable-limit branch from b88d364 to 9a39073 Compare July 30, 2026 17:04
@maresb maresb changed the title Work around Giac's RUR crash on ideals in 16 or more variables Guard against Giac RUR crashes on large ideals Jul 30, 2026
@maresb
maresb force-pushed the fix-giac-rur-variable-limit branch from 9a39073 to 8a24cef Compare July 30, 2026 18:39
@maresb

maresb commented Jul 30, 2026

Copy link
Copy Markdown
Author

Much more clean now, ready for review.

@maresb
maresb marked this pull request as ready for review July 30, 2026 18:44
@NathanDunfield

Copy link
Copy Markdown
Member

I could find no reference confirming this issue in giac. What is your source?

@maresb

maresb commented Aug 2, 2026

Copy link
Copy Markdown
Author

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 ChangeLog/NEWS in the tarball are 860- and 589-byte stubs — so the commit is the only citable artifact.

Reproducer without Sage or SnapPy, 4 lines of giac:

F:=[x0^2-2,x1^2-3,x2-x0*x1,x3-x0,x4-x0,x5-x0,x6-x0,x7-x0,
    x8-x0,x9-x0,x10-x0,x11-x0,x12-x0,x13-x0,x14-x0,x15-x0]:;
X:=[x0,x1,x2,x3,x4,x5,x6,x7,x8,x9,x10,x11,x12,x13,x14,x15]:;
G:=gbasis(F,X,rur):;
print(size(G));

On conda-forge giac=1.9.0.21 this dies with malloc(): unaligned tcache chunk detected (SIGSEGV, exit 139). Dropping x15 (15 variables) succeeds; keeping 16 variables but calling gbasis(F,X) without rur also succeeds; 17 and 20 variables crash. On giac=2.0.0.19 all cases succeed and the 16-variable RUR is correct.

Mechanism. _gbasis templates the Gröbner/RUR engine on the monomial type and switches at exactly 15 variables — if (v.front().dim<=15 …) picks tdeg_t15, else tdeg_t64. tdeg_t15 is a flat packed struct with no destructor; tdeg_t64 is a union that, in its >15-variable form, holds a refcounted malloc'd exponent buffer released by ~tdeg_t64 (--(*ui); if (*ui==0) free(ui);). rur_minpoly built the constant monomial 1 as T_unsigned<modint,tdeg_t>(1,0), which yields the inline representation while every other monomial in a >15-variable run is in the heap representation — and tdeg_t64::operator+ dispatches only on the left operand's tag, so inline_1 + heap_monomial takes the inline path and produces an object that looks heap-allocated but aliases the other operand's pointer without incrementing its refcount. Its destructor then frees a still-live buffer. Parisse's fix constructs the monomial properly (tdeg_t(index_m(dim),order)), hardens the destructor with && ui, and annotates the operator // FIXME: if x is 0 and y not 0 with different rep.

Version boundary. I extracted src/cocoa.cc from the release tarballs and grepped: giac 1.9.0-999 (2025-04-25, the last 1.9 release, one day before the fix) has all three unfixed sites and no destructor guard; giac 2.0.0-1 (2025-05-12) has the fix. So version < (2, 0) is exact rather than approximate — no released 1.9.x is patched, and the fix shipped in the very first 2.0.0. Still shipping unpatched today: conda-forge 1.9.0.21, Debian 13, FreeBSD, Gentoo, Fedora 43; patched: conda-forge 2.0.0.19, Arch, Nix, Guix.

I also checked the two other look-alike (1,0) sites Parisse left alone — both are unreachable (one behind an always-taken #if 1 … return, one defused by an early return in rur_linsolve), so there's nothing outstanding to report upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants