G2 and valgrind

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

G2 and valgrind

Phil Longstaff-2
I've been playing around with G2 and valgrind 3.1.0 and have seen the
following issues on SuSE 9.3:

1) There are lots of messages about decisions based on uninitialized variables
and accessing past the end of malloc'ed mem in the code dealing with guids.  
It seems as though readdir() does not completely initialize the dirent struct
that it returns.  There were also messages that md5_process_bytes() was
accessing bytes from the dirent that were off the end of memory malloc'ed by
opendir().  Finally, there were messages that guid_equal() was returning a
result based on uninitialized values because the comparison was done
including the __align_me field, not just the guid data.

The attached patch replaces the const 16 with GUID_DATA_SIZE, only compares
the guid data, not the other field in the struct, and removes the use of the
dirent structure in md5 calculation.

2) After g2 is up, I open an account, switch to ledger view, put my cursor
into a split and start pressing delete-tab-delete-tab...  I then get the
listing shown in attachment gnucash.valgrind.  I added printf statements to
show as splits were allocated and freed.  Most serious is that a split
continues to be accessed after it is freed.

Phil

patch (2K) Download Attachment
gnucash.valgrind (57K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: G2 and valgrind

Derek Atkins
Quoting Phil Longstaff <[hidden email]>:

> I've been playing around with G2 and valgrind 3.1.0 and have seen the
> following issues on SuSE 9.3:
>
> 1) There are lots of messages about decisions based on uninitialized
> variables
> and accessing past the end of malloc'ed mem in the code dealing with guids.
> It seems as though readdir() does not completely initialize the dirent struct
> that it returns.  There were also messages that md5_process_bytes() was
> accessing bytes from the dirent that were off the end of memory malloc'ed by
> opendir().

These are all perfectly fine.  It's just trying to get random data, so
the fact that the data is uninitialized is okay.  If it bothers you,
you should add a valgrind supression.

>   Finally, there were messages that guid_equal() was returning a
> result based on uninitialized values because the comparison was done
> including the __align_me field, not just the guid data.

This should get fixed.

> The attached patch replaces the const 16 with GUID_DATA_SIZE, only compares
> the guid data, not the other field in the struct, and removes the use of the
> dirent structure in md5 calculation.

The first part of the patch is okay in theory (I haven't looked at the code).
The second part of the patch is not okay.

> 2) After g2 is up, I open an account, switch to ledger view, put my cursor
> into a split and start pressing delete-tab-delete-tab...  I then get the
> listing shown in attachment gnucash.valgrind.  I added printf statements to
> show as splits were allocated and freed.  Most serious is that a split
> continues to be accessed after it is freed.

This is certainly something that needs to get fixed!  Thank you.  Hopefully
someone could track down the reference?

> Phil

-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
       [hidden email]                        PGP key available

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: G2 and valgrind

Josh Sled
On Sun, 2006-02-19 at 17:57 -0500, Derek Atkins wrote:
> > 2) After g2 is up, I open an account, switch to ledger view, put my cursor
> > into a split and start pressing delete-tab-delete-tab...  I then get the
> > listing shown in attachment gnucash.valgrind.  I added printf statements to
> > show as splits were allocated and freed.  Most serious is that a split
> > continues to be accessed after it is freed.
>
> This is certainly something that needs to get fixed!  Thank you.  Hopefully
> someone could track down the reference?

See http://bugzilla.gnome.org/show_bug.cgi?id=108347 and
http://bugzilla.gnome.org/show_bug.cgi?id=141287 and
http://bugzilla.gnome.org/show_bug.cgi?id=125480 ... they all seem very
likely to be related, and maybe or symptoms of the same underlying
problem(s?).

--
...jsled
http://asynchronous.org/ - `a=jsled; b=asynchronous.org; echo ${a}@${b}`
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: G2 and valgrind

Phil Longstaff-2
On February 19, 2006 06:09 pm, Josh Sled wrote:

> On Sun, 2006-02-19 at 17:57 -0500, Derek Atkins wrote:
> > > 2) After g2 is up, I open an account, switch to ledger view, put my
> > > cursor into a split and start pressing delete-tab-delete-tab...  I then
> > > get the listing shown in attachment gnucash.valgrind.  I added printf
> > > statements to show as splits were allocated and freed.  Most serious is
> > > that a split continues to be accessed after it is freed.
> >
> > This is certainly something that needs to get fixed!  Thank you.
> > Hopefully someone could track down the reference?
>
> See http://bugzilla.gnome.org/show_bug.cgi?id=108347 and
> http://bugzilla.gnome.org/show_bug.cgi?id=141287 and
> http://bugzilla.gnome.org/show_bug.cgi?id=125480 ... they all seem very
> likely to be related, and maybe or symptoms of the same underlying
> problem(s?).

I think so.  I've been poking around these.

Phil
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: G2 and valgrind

Derek Atkins
In reply to this post by Phil Longstaff-2
Phil Longstaff <[hidden email]> writes:

> The attached patch replaces the const 16 with GUID_DATA_SIZE, only compares
> the guid data, not the other field in the struct, and removes the use of the
> dirent structure in md5 calculation.

I've applied everything but the part to remove the dirent/md5 call.
We shouldn't reduce our entropy.  We should just add a valgrind
suppression for that particular issue.

-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
       [hidden email]                        PGP key available
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: G2 and valgrind

Phil Longstaff-2
On February 19, 2006 06:43 pm, Derek Atkins wrote:

> Phil Longstaff <[hidden email]> writes:
> > The attached patch replaces the const 16 with GUID_DATA_SIZE, only
> > compares the guid data, not the other field in the struct, and removes
> > the use of the dirent structure in md5 calculation.
>
> I've applied everything but the part to remove the dirent/md5 call.
> We shouldn't reduce our entropy.  We should just add a valgrind
> suppression for that particular issue.
>
> -derek

No problem.  I wasn't happy with it anyway.  I'm more concerned about the read
after the end of a malloc'ed area than about the uninitialized memory.  I'll
figure out a patch for the valgrind suppressions and send in a patch.

Phil
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: G2 and valgrind

Chris Shoemaker
On Sun, Feb 19, 2006 at 02:01:08PM -0500, Phil Longstaff wrote:

> On February 19, 2006 06:43 pm, Derek Atkins wrote:
> > Phil Longstaff <[hidden email]> writes:
> > > The attached patch replaces the const 16 with GUID_DATA_SIZE, only
> > > compares the guid data, not the other field in the struct, and removes
> > > the use of the dirent structure in md5 calculation.
> >
> > I've applied everything but the part to remove the dirent/md5 call.
> > We shouldn't reduce our entropy.  We should just add a valgrind
> > suppression for that particular issue.
> >
> > -derek
>
> No problem.  I wasn't happy with it anyway.  I'm more concerned about the read
> after the end of a malloc'ed area than about the uninitialized memory.  I'll
> figure out a patch for the valgrind suppressions and send in a patch.

Been there, done that.  The problem with using valgrind suppressions
for this is that you'd need a supression for every codepath that jumps
on a guid read, like every instance of a ghashtable that uses guid
keys and every place we store a guid and later compare with it.  

I think it's better just to use the valid portion of the dirent.
Something like:

md5_process_bytes(de, strlen(de->d_name), &guid_context);
total += strlen(de->d_name);

-chris
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: G2 and valgrind

Derek Atkins
Chris Shoemaker <[hidden email]> writes:

> Been there, done that.  The problem with using valgrind suppressions
> for this is that you'd need a supression for every codepath that jumps
> on a guid read, like every instance of a ghashtable that uses guid
> keys and every place we store a guid and later compare with it.  
>
> I think it's better just to use the valid portion of the dirent.
> Something like:
>
> md5_process_bytes(de, strlen(de->d_name), &guid_context);
> total += strlen(de->d_name);

I still can't believe that there's no way to tell valgrind
"this memory object is initialized, even if it may be 'tained'
by uninitialized memory"..   That would seem to be a core
suppression method, the "I know this data object is clean".

It's sounding like valgrind is making (in this case incorrect)
assumptions that if you process uninitialzed data and then use
the result of that processing, the result is 'tainted'.  In
random number generation, that's a false assumption..

On the other hand, we DO initialize from /dev/urandom....  So the lack
of extra entropy might not be a huge problem.
*grumps*

> -chris

-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
       [hidden email]                        PGP key available
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: G2 and valgrind

Phil Longstaff-2
On February 19, 2006 08:41 pm, Derek Atkins wrote:
> I still can't believe that there's no way to tell valgrind
> "this memory object is initialized, even if it may be 'tained'
> by uninitialized memory"..   That would seem to be a core
> suppression method, the "I know this data object is clean".

There is a mechanism if you don't mind adding a compile-time dependence on
valgrind.

Phil
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: G2 and valgrind

Derek Atkins
Quoting Phil Longstaff <[hidden email]>:

> On February 19, 2006 08:41 pm, Derek Atkins wrote:
>> I still can't believe that there's no way to tell valgrind
>> "this memory object is initialized, even if it may be 'tained'
>> by uninitialized memory"..   That would seem to be a core
>> suppression method, the "I know this data object is clean".
>
> There is a mechanism if you don't mind adding a compile-time dependence on
> valgrind.

We already do..   It's optional.  If we find the valgrind dev environment
then we use it in a few places.

> Phil

-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
       [hidden email]                        PGP key available

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: G2 and valgrind

Josh Sled
In reply to this post by Phil Longstaff-2
On Sun, 2006-02-19 at 12:21 -0500, Phil Longstaff wrote:
> 2) After g2 is up, I open an account, switch to ledger view, put my cursor
> into a split and start pressing delete-tab-delete-tab...  I then get the
> listing shown in attachment gnucash.valgrind.  I added printf statements to
> show as splits were allocated and freed.  Most serious is that a split
> continues to be accessed after it is freed.

Was this readily reproducible?
Is it better after r13343?

--
...jsled
http://asynchronous.org/ - `a=jsled; b=asynchronous.org; echo ${a}@${b}`
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: G2 and valgrind

Phil Longstaff-2
On February 21, 2006 03:16 pm, Josh Sled wrote:
> On Sun, 2006-02-19 at 12:21 -0500, Phil Longstaff wrote:
> > 2) After g2 is up, I open an account, switch to ledger view, put my
> > cursor into a split and start pressing delete-tab-delete-tab...  I then
> > get the listing shown in attachment gnucash.valgrind.  I added printf
> > statements to show as splits were allocated and freed.  Most serious is
> > that a split continues to be accessed after it is freed.
>
> Was this readily reproducible?
> Is it better after r13343?

It was readily reproducible with r13337.  It was not reproducible with r13343.  
There were a few oddities.  I originally had about 20 transactions with the
same date.  When I modified the first one, the transactions were reordered so
that the one I was modifying was shifted down to just before the blank
transaction.  It's not really an issue because I don't see that the order of
transactions with the same date matters.

Phil
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: G2 and valgrind

David Hampton-2
On Sun, 2006-02-26 at 12:32 -0500, Phil Longstaff wrote:
> There were a few oddities.  I originally had about 20 transactions with the
> same date.  When I modified the first one, the transactions were reordered so
> that the one I was modifying was shifted down to just before the blank
> transaction.  It's not really an issue because I don't see that the order of
> transactions with the same date matters.

That behavior has been around a long time.  The sorting order is
something like "Date of transaction", DEBIT/CREDIT, "Date of Entry",
other things.  You've changed the date of entry which is why the
transaction moves to the bottom.

David


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel