Opened 9 years ago

Last modified 6 months ago

#12858 positive_review defect

Bug in sympow

Reported by: stephen Owned by: rlm
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: memleak Keywords: sympow FreeBSD
Cc: mjo Merged in:
Authors: Stephen Montgomery-Smith Reviewers: Dima Pasechnik
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

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';}

Change History (21)

comment:1 Changed 9 years ago by kcrisman

  • Authors set to Stephen Montgomery-Smith

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?

comment:2 Changed 8 years ago by kcrisman

  • Component changed from memleak to porting: BSD
  • Owner changed from rlm to pjeremy

comment:3 Changed 8 years ago by stephen

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"?

comment:4 Changed 8 years ago by kcrisman

  • Component changed from porting: BSD to memleak
  • Owner changed from pjeremy to rlm

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?

comment:5 follow-up: Changed 8 years ago by stephen

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.

comment:6 in reply to: ↑ 5 ; follow-ups: Changed 8 years ago by kcrisman

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...

comment:7 in reply to: ↑ 6 Changed 8 years ago by fbissey

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?

comment:8 follow-up: Changed 8 years ago by stephen

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

comment:9 in reply to: ↑ 8 Changed 8 years ago by fbissey

Replying to 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.

comment:10 in reply to: ↑ 6 Changed 8 years ago by kcrisman

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.

comment:11 Changed 8 years ago by kcrisman

  • Keywords FreeBSD added

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

comment:12 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:13 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:14 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:15 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:16 Changed 6 months ago by dimpase

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)

comment:17 Changed 6 months ago by mkoeppe

  • Cc mjo added
  • Milestone changed from sage-6.4 to sage-9.3
  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.

comment:19 Changed 6 months ago by dimpase

  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Fixed upstream, in a later stable release.
  • Reviewers set to Dima Pasechnik
  • Status changed from new to needs_review

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.

comment:20 Changed 6 months ago by dimpase

  • Milestone changed from sage-9.3 to sage-duplicate/invalid/wontfix
  • Status changed from needs_review to positive_review

comment:21 Changed 6 months ago by dimpase

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

Note: See TracTickets for help on using tickets.