#14501: Fix memory allocation problems in data_structures_pyx.pxi
---------------------------------+------------------------------------------
Reporter: dcoudert | Owner: joyner
Type: defect | Status: needs_review
Priority: major | Milestone: sage-5.10
Component: group theory | Resolution:
Keywords: | Work issues:
Report Upstream: N/A | Reviewers:
Authors: David Coudert | Merged in:
Dependencies: | Stopgaps:
---------------------------------+------------------------------------------
Comment (by dcoudert):
Nathann,
again, pointers are not set to NULL by default, and a {{{sage_free}}} call
on an non initialized pointer gives a segfault (or should give a
segfault).
Replying to [comment:8 ncohen]:
> Your patch adds
> {{{
> sage_free(int_array)
> sage_free(int_ptrs)
> }}}
>
> three lines before `SC_dealloc(SC)`. In `SC_dealloc` :
> {{{
> sage_free(SC.generators) # frees int_ptrs
> sage_free(SC.orbit_sizes) # frees int_array
> }}}
Yes and I'm right to do it since at that point of the program the
{{{int_array}}} variable has not been assigned to SC.generators. Therefore
SC.generators points to something we don't know (neither {{{int_array}}},
nor NULL) and a call to {{{sage_free}}} could results in a segfault (at
least it is the case on my computer). The same for {{{int_ptrs}}}.
> You also set to NULL the members of a structure that is deallocated two
lines afterwards.
Yes, since the {{{SC_dealloc}}} methods tests if some members of the
structure are not NULL. If they have never been initialized, they could be
different from NULL and so we can have a segfault!
> The `and SC.gen_inverses is not NULL` also seems to be unnecessary, for
`SC.gen` and `SC.gen_inverses` are always allocated (and checked)
simultaneously, so that both are not NULL or both are NULL.
No, they are allocated using 2 distinct malloc. So the first one could
succeed while the second one fails. In case the second fails, then it is
set to NULL.
> And "formally", that is if you believe that one can be NULL while the
other is not (which does not happen) you introduce a memory leak with this
additional test, for you will only deallocate both vectors if BOTH are
NULL, and do nothing when only one of them is NULL (which again, does not
happen).
Yes, that's right. So I'm splitting these instruction again.
I'm not adding these instructions for fun. I'm adding them because if you
follow properly the sequence of instructions, you realize that some tests
are unsafe and could result in segfault. More precisely, removing any of
these instructions results in a segfault on my computer.
David.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14501#comment:9>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
