Opened 7 years ago

Closed 7 years ago

#13875 closed enhancement (fixed)

Test memory allocation in distances_all_pairs

Reported by: dcoudert Owned by: jason, ncohen, rlm
Priority: major Milestone: sage-5.6
Component: graph theory Keywords:
Cc: ncohen Merged in: sage-5.6.rc0
Authors: David Coudert Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13808 Stopgaps:

Description

This patch adds tests on memory allocation in the distances_all_pairs module. This is to prevent returning NULL pointers when the memory space is insufficient to allocate data structure.

These tests are not doctested since the behavior depends on the computer (ram).

Attachments (1)

trac_13875.patch (4.8 KB) - added by dcoudert 7 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 7 years ago by dcoudert

  • Status changed from new to needs_review

comment:2 follow-up: Changed 7 years ago by ncohen

  • Dependencies set to #13808
  • Status changed from needs_review to needs_work

Hellooooooooooooooo !!!

You cannot do things like if distances==NULL or predecessor==NULL: and raise an exception in that case. If distances has been allocated and predecessor hasn't, then an exception is raised before distances is freed.

Besides, I expect this patch to be incompatible with your hyperbolicity patch. This ticket should depend on #13808 !

Oh. And I am not sure that it is wise to return as a message "Not enough space". Sometimes there is sufficient memory available, but not a contiguous segment as long as you need it to be. In such cases there is sufficient memory, just not allocated as it should. Something like raise MemoryError() is ok I guess.

Nathann

comment:3 in reply to: ↑ 2 Changed 7 years ago by dcoudert

  • Status changed from needs_work to needs_review

You cannot do things like if distances==NULL or predecessor==NULL: and raise an exception in that case. If distances has been allocated and predecessor hasn't, then an exception is raised before distances is freed.

done.

Besides, I expect this patch to be incompatible with your hyperbolicity patch. This ticket should depend on #13808 !

No, this ticket is hopefully independent on the hyperbolicity patch. Furthermore, it don't change the behavior of the hyperbolicity patch. I have tested all possible combinations and I have no conflicts.

Oh. And I am not sure that it is wise to return as a message "Not enough space". Sometimes there is sufficient memory available, but not a contiguous segment as long as you need it to be. In such cases there is sufficient memory, just not allocated as it should. Something like raise MemoryError() is ok I guess.

done.

comment:4 Changed 7 years ago by ncohen

?.. BUt in the hyperbolicity patch you test that the pointers you are given by these methods are not null, don't you ? ANd after your patch they cannot be NULL anymore ?...

Nathann

comment:5 Changed 7 years ago by dcoudert

  • Dependencies #13808 deleted

If we have to indicate dependencies, it's in the other way. We may change patch #13808 according the behavior of this one, but this patch don't depend on patch #13808. I have therefore removed the dependency.

Furthermore, it is true that with this patch some tests in patch #13808 become useless. However, I think it is better to be on the safe side and to let them in case something else happen. The cost of these tests is clearly negligible.

comment:6 Changed 7 years ago by ncohen

Yoooooooooo !!

Well, the point of not having #13808 depend on this very patch is that #13808 is complicated enough *AND* already reviewed. And what's the point of testing whether the pointers are not null when the function cannot return NULL pointers ? It is checked, the function just can't return NULL pointers O_o The cost is negligible but what's the point as what it does is useless ? The code is not that easy to read, so let's remove the maximum amount of useless stuff ! And if we happen to copy/paste code from this file to another one, we won't copy useless checks at the same time ! O_o

Nathann

comment:7 Changed 7 years ago by dcoudert

  • Dependencies set to #13808

OK, you win ;-) I have removed the test in hyperbolicity.pyx and so added the dependency.

comment:8 Changed 7 years ago by ncohen

  • Status changed from needs_review to needs_work

^^;

Thanks !

As before, though, there are memory leaks if t_prec is allocated but not prec.

 t_prec = <unsigned short *> sage_malloc(n*n*sizeof(short)) 
 if t_prec==NULL: 
     raise MemoryError() 
 prec = <unsigned short **> sage_malloc(n*sizeof(short *)) 
 if prec==NULL: 
     raise MemoryError() 

You should add a sage_free(t_prec) before the second raise MemoryError(), and it hppens several timees in the file.

Nathann

Changed 7 years ago by dcoudert

comment:9 Changed 7 years ago by dcoudert

  • Status changed from needs_work to needs_review

Fixed.

comment:10 Changed 7 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

Looks good :-)

(and I love my hggett script :-P)

Nathann

comment:11 Changed 7 years ago by dcoudert

Thanks for the review and for the script ;-)

comment:12 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.6.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.