Opened 11 years ago
Closed 20 months ago
#12858 closed defect (invalid)
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: |
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 (22)
comment:1 Changed 11 years ago by
Authors: | → Stephen Montgomery-Smith |
---|
comment:2 Changed 10 years ago by
Component: | memleak → porting: BSD |
---|---|
Owner: | changed from rlm to pjeremy |
comment:3 Changed 10 years ago by
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 10 years ago by
Component: | porting: BSD → 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: 6 Changed 10 years ago by
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 follow-ups: 7 10 Changed 10 years ago by
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 Changed 10 years ago by
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: 9 Changed 10 years ago by
My understanding is that arrays in C are not automatically initialized.
comment:9 Changed 10 years ago by
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 Changed 10 years ago by
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 10 years ago by
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 9 years ago by
Milestone: | sage-5.11 → sage-5.12 |
---|
comment:13 Changed 9 years ago by
Milestone: | sage-6.1 → sage-6.2 |
---|
comment:14 Changed 9 years ago by
Milestone: | sage-6.2 → sage-6.3 |
---|
comment:15 Changed 8 years ago by
Milestone: | sage-6.3 → sage-6.4 |
---|
comment:16 Changed 2 years ago by
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 2 years ago by
Cc: | mjo added |
---|---|
Milestone: | sage-6.4 → sage-9.3 |
Report Upstream: | N/A → Not yet reported upstream; Will do shortly. |
comment:18 Changed 2 years ago by
I think this was fixed by removing the free()
instead:
https://gitlab.com/rezozer/forks/sympow/-/commit/f414d52ee601c54fab556ad2ecf79762ec7d5eef
comment:19 Changed 2 years ago by
Report Upstream: | Not yet reported upstream; Will do shortly. → Fixed upstream, in a later stable release. |
---|---|
Reviewers: | → Dima Pasechnik |
Status: | new → 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 2 years ago by
Milestone: | sage-9.3 → sage-duplicate/invalid/wontfix |
---|---|
Status: | needs_review → positive_review |
comment:21 Changed 2 years ago by
I've opened https://gitlab.com/rezozer/forks/sympow/-/issues/6 just so that the upstream is aware of this.
comment:22 Changed 20 months ago by
Resolution: | → invalid |
---|---|
Status: | positive_review → closed |
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?