Bug in sympow

[sagetrac-stephen]

In util.c there is a function free_data, which frees TACKS[0]. TACKS[0] is meant to be allocated in disk.c, but there are circumstances when TACKS[0] is not allocated.

The following patch seems to fix it. Notice that it makes use of the fact that free(NULL) should do nothing in the C programming language.

--- sympow-1.018.1.p11/src/disk.c-orig  2012-04-19 02:33:51.000000000 +0000
+++ sympow-1.018.1.p11/src/disk.c       2012-04-19 02:34:22.000000000 +0000
@@ -39,7 +39,7 @@
  else if (((sp&3)==0) && CM_CASE) {if (2*ep==sp) S[3]='l'; else S[3]='h';}
  else {if (2*ep==sp) S[3]='L'; else S[3]='H';}
  if (HECKE && dv) {TACKS[which]=malloc(dv*sizeof(QD)); TACKON[which]=dv;}
- else if (dv<=sp/2) TACKON[which]=0;
+ else if (dv<=sp/2) {TACKS[which]=NULL; TACKON[which]=0;}
  else {TACKS[which]=malloc((dv-sp/2)*sizeof(QD)); TACKON[which]=dv-sp/2;}
  S[4]=0; F=fopen("datafiles/param_data","r"); strcpy(U,S);
  if (ANAL_RANK) {if (dv>0) U[0]='A'; else U[0]='m';}

Upstream: Fixed upstream, in a later stable release.

CC: @orlitzky

Component: memleak

Keywords: sympow FreeBSD

Author: Stephen Montgomery-Smith

Reviewer: Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/12858

[kcrisman]

comment:1

Needs an spkg. I'm sorry that I do not know enough about allocations to review this.

Is there any way to doctest this, just out of curiosity?

[kcrisman]

Author: Stephen Montgomery-Smith

[sagetrac-stephen]

comment:3

I don't think this error only effects BSD. Rather I think it is an error that only happens very rarely, and I got unlucky.

Also, I seem to have missed the message posted 10 months ago. What do you mean by "doctest"?

[kcrisman]

comment:4

Sorry about that.

"doctest" means that whenever we have a regression that we fix, we add a test to the suite of tests to check that it stays fixed. Is there a command that failed or leaked, or did Sage simply fail to compile this spkg?

[sagetrac-stephen]

comment:5

OK, I did find this error with a doctest. I don't remember which doctest it was, but it was quite a few of them.

But I think it depends on the computer you are using, OS, probably amount of RAM, other processes, etc. In my case it failed an existing doctest on one computer, and passed on other computers.

But also, anyone who understands how malloc and free work should see instantly that the existing code has errors.

See man 3 free on any unix computer, e.g. on ubuntu:

       The free() function frees the memory space pointed  to  by  ptr,  which
       must  have  been  returned  by a previous call to malloc(), calloc() or
       realloc().  Otherwise, or if free(ptr) has already been called  before,
       undefined behavior occurs.  If ptr is NULL, no operation is performed.

[kcrisman]

comment:6

OK, I did find this error with a doctest. I don't remember which doctest it was, but it was quite a few of them.

But I think it depends on the computer you are using, OS, probably amount of RAM, other processes, etc. In my case it failed an existing doctest on one computer, and passed on other computers.

If you wouldn't mind terribly just undoing this patch and seeing which one it was, then we could add a nearly trivial patch adding a comment to that doctest saying also tests that :trac:`12858` is fixed or something like that.

But also, anyone who understands how malloc and free work should see instantly that the existing code has errors.

Since I don't really know C, I don't :) But presumably many other Sage developers do...

[kiwifb]

comment:7

Replying to @kcrisman:

Since I don't really know C, I don't :) But presumably many other Sage developers do...

The problem here is that sympow is not even legal C. We had that conversation a while ago with David Kirkby that sympow was probably by far the worst piece of code in sage and it couldn't even be entered in "the obfuscated C contest" since some bits are not even legal C.

The patch looks a bit weird but that's not any weirder than the surrounding code. And I actually understand the intent and by the look of it, it is the only case where TACKS[which] is not allocated. The other solution would be perform some check before deallocation.

But further, TACKS is an array of pointers, shouldn't the pointer be NULL if not allocated or is it just at the discretion of the compiler, or possibly a compilation option?

[sagetrac-stephen]

comment:8

My understanding is that arrays in C are not automatically initialized.

[kiwifb]

comment:9

Replying to @sagetrac-stephen:

My understanding is that arrays in C are not automatically initialized.

You are quite right. Wishful thinking of my part, although there must a gcc flag to do it.

[kcrisman]

comment:10

But I think it depends on the computer you are using, OS, probably amount of RAM, other processes, etc. In my case it failed an existing doctest on one computer, and passed on other computers.

If you wouldn't mind terribly just undoing this patch and seeing which one it was, then we could add a nearly trivial patch adding a comment to that doctest saying also tests that :trac:`12858` is fixed or something like that.

Ping - if you happen to have time to check that out, I realize it could be annoying to do.

[kcrisman]

comment:11

Adding keyword even though it may be a bug on all platforms, since it seems to have only been reported on FreeBSD.

[kcrisman]

Changed keywords from sympow to sympow FreeBSD

[dimpase]

comment:16

this appears still to be the case, see https://gitlab.com/rezozer/forks/sympow/-/blob/master/disk.c#L60
(this GitLab repo, I gather, is Debian's pseudo-upstream)

[mkoeppe]

Upstream: Not yet reported upstream; Will do shortly.

[orlitzky]

comment:18

I think this was fixed by removing the free() instead:

https://gitlab.com/rezozer/forks/sympow/-/commit/f414d52ee601c54fab556ad2ecf79762ec7d5eef

[dimpase]

Reviewer: Dima Pasechnik

[dimpase]

comment:19

this "fix" replaces a wrong attempt to free memory held (a bit wrongly, as some pointers that by right should be NULL are not always, as found on this ticket) in an array of pointers by no freeing at all.

This is of course fine if there are only created once during a run of the code, and this appears to be the case.

[dimpase]

Changed upstream from Not yet reported upstream; Will do shortly. to Fixed upstream, in a later stable release.

[dimpase]

comment:21

I've opened https://gitlab.com/rezozer/forks/sympow/-/issues/6 just so that the upstream is aware of this.