Matthew Vanecek <[EMAIL PROTECTED]> writes:
> > 2) You should never have to set data to NULL when you free it.
>
> You should *always* set the pointer to NULL when you free it. You know
> that. You can find it in any competent C reference. It prevents
> dangling pointers and situations like this.
Sorry, but if I'm doing:
free(foo->p1);
free(foo->p2);
free(foo);
I feel no need to set foo's internal to NULL, because _foo_ is
invalid! The caller of gnc_book_destroy() should certainly clear
out any references to foo, but that doesn't mean that free'd data
should be zeroized before it's freed.
> > 3) It sounds like the REAL problem is that the postgres backend is
> > referencing a deleted book -- why is it doing that? You should
> > try to fix that, rather than trying to fix the symbols of accessing
> > freed data.
> >
>
> The *real* problem may be a combination of a lack of functionality in
> the PG backend, plus dangling pointers in the engine not being set to
> NULL like they should. There is currently no way I can see to remove a
> book from the PG backend's booklist. The PG backend and engine and glib
> *do*, however, check for NULL before accessing data a pointer points to.
Well, the problem is that something is maintaining a reference to this
book after it's being destroyed. I have no idea who's maintaining
the pointer, or why.
> The PG backend retrieves the book from the session, or is told what the
> book is by the Engine or Session. The PG backend doesn't create books
> willy-nilly (not yet =P), so if a book in the PG backend's book list has
> dangling pointers, there's really nothing it can do about it at this
> point.
Is it caching the book? Who is destroying the book? The problem is
not dangling pointers _in_ the book, the problem is a dangling pointer
_TO_ the book. There is a major difference between these two issues.
If the problem were the former then I would agree that the pointers
should be zeroized. As the problem is the latter, I do not agree.
What you are, in essense, saying is that you believe this code is reasonable:
free(book->data);
free(book);
...
if (book->data)
use_data(book->data);
I disagree that this is reasonable code. What needs to happen is more
along the lines of:
free(book->data);
free(book);
book = NULL;
if (book && book->data)
use_data(book->data);
This requires finding the reference to book and zeroizing it after
gnc_book_destroy().
> To me, it is simpler to fix the dangling pointers, especially since the
> memory that the pointers point to has already been freed and is no
> longer accessible (thus causing the segmentation fault when accessed).
> Certainly issues exist in the existing backend; that's why it's being
> re-written. But the entire call chain *does* check for NULL before
> accessing the data. The data is not NULL, so a segmentation fault
> ensues.
Unfortunately checking for NULL in invalid memory just pushes the
problem off until later. What will happen is a heisenbug, where some
day someone will find a strange case where the memory has been
over-written by something else, and then you'll jump off into lala
land. By zeroizng before free you're just hiding the symptom, but not
fixing the actual problem, instead you're waiting for it to come up
again and bite us at some later date.
A much better solution is figuring out who is still referencing the
book after it's destroyed.
> I'll change the kvp_frame thing to use kvp_frame_delete and resubmit the
> patch...
I've already added the kvp_frame_delete() into the code (both in HEAD
and 1.8), so no need to do that. Thanks, tho ;)
-derek
--
Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
Member, MIT Student Information Processing Board (SIPB)
URL: http://web.mit.edu/warlord/ PP-ASEL-IA N1NWH
[EMAIL PROTECTED] PGP key available
_______________________________________________
gnucash-devel mailing list
[EMAIL PROTECTED]
http://www.gnucash.org/cgi-bin/mailman/listinfo/gnucash-devel
