Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Segfault in tst/testinstall/syntaxtree.tst #3187

Closed
olexandr-konovalov opened this issue Jan 17, 2019 · 6 comments
Closed

Segfault in tst/testinstall/syntaxtree.tst #3187

olexandr-konovalov opened this issue Jan 17, 2019 · 6 comments

Comments

@olexandr-konovalov
Copy link
Member

This happens in the master branch when all packages are loaded. See e.g. on Travis here: https://travis-ci.org/gap-infra/gap-docker-master-testsuite/jobs/480427212. Started to happen two days ago.

@olexandr-konovalov
Copy link
Member Author

olexandr-konovalov commented Jan 17, 2019

Incidentally, on the same day Semigroups and Utils tests started to fail too:

for the same versions of these packages for which they used to work a day before.

@olexandr-konovalov
Copy link
Member Author

The error happening in Semigroups does not sound right at all:

######## > Diff in:
/home/gap/inst/gap-master/pkg/semigroups-3.0.20/tst/standard/congrms.tst:104
# Input is:
IsSubrelation(cong2, cong);
# Expected output:
Error, Semigroups: IsSubrelation: usage,
congruences must be defined over the same semigroup,
# But found:
Error, Z(3)^7Z(3)^7
########
#I  Errors detected while testing

@ChrisJefferson
Copy link
Contributor

So, here's the bug (I think).

src/syntaxtree.c:174 is: value = MakeObjInt(CONST_ADDR_EXPR(expr) + 1, size);. This is passing a pointer inside a bag to MakeObjInt, which internally allocates a new bag, so this pointer moves and awful things happen.

I think in this case we could replace this with EvalIntExpr(expr), which is how the evaluator normally turns expr ints into ints, but I'd prefer an integer expert (so @fingolfin :) ) to sanity-check :)

@ChrisJefferson
Copy link
Contributor

EDIT: No, that's old code.

@ChrisJefferson
Copy link
Contributor

ChrisJefferson commented Jan 17, 2019

Updated problem (I think)

GET_VALUE_FROM_CURRENT_BODY isn't wired up properly to the syntaxtree (I think), so getting constants isn't producing the right answers. This is used in evaluating Strings (for example).

fingolfin added a commit to fingolfin/gap that referenced this issue Jan 17, 2019
The NEW_PLIST call could trigger a GC, which then rendered the value of header
invalid, possibly leading to arbitrary memory corruption, and also the values
pointer of the function body was then not set correctly.

Fixes gap-system#3187
@fingolfin
Copy link
Member

GET_VALUE_FROM_CURRENT_BODY is fine, the problem is that I introduced a silly GC bug, by keeping a pointer into an object across an allocation. Should be fixed in PR #3191

fingolfin added a commit to fingolfin/gap that referenced this issue Jan 17, 2019
The NEW_PLIST call could trigger a GC, which then rendered the value of header
invalid, possibly leading to arbitrary memory corruption, and also the values
pointer of the function body was then not set correctly.

Fixes gap-system#3187
markuspf pushed a commit that referenced this issue Jan 18, 2019
The NEW_PLIST call could trigger a GC, which then rendered the value of header
invalid, possibly leading to arbitrary memory corruption, and also the values
pointer of the function body was then not set correctly.

Fixes #3187
ssiccha pushed a commit to ssiccha/gap that referenced this issue Mar 27, 2019
The NEW_PLIST call could trigger a GC, which then rendered the value of header
invalid, possibly leading to arbitrary memory corruption, and also the values
pointer of the function body was then not set correctly.

Fixes gap-system#3187
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

No branches or pull requests

3 participants