Valgrind warning cleanup...

[Dan Kegel]

I was trying to use valgrind-3.1.1 to track down
an OpenOffice crash, and I noticed that current
wine shows errors like this even running notepad:

==8986== Invalid read of size 4
==8986==    at 0x20010C21: (within /lib/ld-2.3.6.so)
...
==8986==    by 0x20189D10: dlopen (in /lib/tls/i686/cmov/libdl-2.3.6.so)
==8986==    by 0x20023CB8: wine_dlopen (loader.c:663)
==8986==    by 0x200248B7: wine_init (loader.c:625)
==8986==    by 0x7BF00F9F: main (main.c:58)
==8986==  Address 0x2018D3A8 is 56 bytes inside a block of size 57 alloc'd
==8986==    at 0x2001C422: malloc (vg_replace_malloc.c:149)
==8986==    by 0x2002463A: first_dll_path (loader.c:201)
==8986==    by 0x20024895: wine_init (loader.c:623)
==8986==    by 0x7BF00F9F: main (main.c:58)

I looked at first_dll_path, but I couldn't see any problem.
On the theory that glibc is reading a bit past the end of the string,
I tried changing the line that does the allocation in question.
Adding one byte cut down the number of those warnings;
adding two bytes didn't help more than adding one;
adding three bytes got rid of the warnings.  Here's the change:

--- libs/wine/loader.c  17 Mar 2006 12:23:52 -0000      1.36
+++ libs/wine/loader.c  13 May 2006 19:56:52 -0000
@@ -199,7 +199,7 @@
     char *p;
     int namelen = strlen( name );

-    context->buffer = malloc( dll_path_maxlen + 2 * namelen +
strlen(ext) + 3 );
+    context->buffer = malloc( dll_path_maxlen + 2 * namelen +
strlen(ext) + 6 );
     context->index = build_dir ? 0 : 3;  /* if no build dir skip all
the build dir magic cases */
     context->name = context->buffer + dll_path_maxlen + namelen + 1;
     context->namelen = namelen + 1;

What do folks think - should we just make the allocation a little bigger to make
valgrind happy?

I'd also like to understand what's causing them, too, though.
- Dan

On Sat, May 13, 2006 at 01:01:34PM -0700, Dan Kegel wrote:
> What do folks think - should we just make the allocation a little
> bigger to make valgrind happy?

I know this kind of errors from working on GNU Parted with Valgrind, and
my policy is to leave the code alone when I can't spot why it should be
wrong.

I strongly advise against doing otherwise.

Leslie

[Dan Kegel]

On 5/13/06, [e-mail]  wrote:
> I know this kind of errors from working on GNU Parted with Valgrind, and
> my policy is to leave the code alone when I can't spot why it should be
> wrong.
>
> I strongly advise against doing otherwise.

I'll buy that.  That's why I sent the note to wine-devel instead
of wine-patches.

There's another valgrind warning that is much more understandable,
and I'm sending an obvious fix for it to wine-patches.
(My first patch missed one instance, so I'm resending.)
I'd appreciate it if you could check that one, too.
- Dan

[Vassilis Virvilis]

Dan Kegel wrote:
> On 5/13/06, [e-mail]  wrote:
> > I know this kind of errors from working on GNU Parted with Valgrind, and
> > my policy is to leave the code alone when I can't spot why it should be
> > wrong.
> >
> > I strongly advise against doing otherwise.
>
> I'll buy that.  That's why I sent the note to wine-devel instead
> of wine-patches.
>
> There's another valgrind warning that is much more understandable,
> and I'm sending an obvious fix for it to wine-patches.
> (My first patch missed one instance, so I'm resending.)
> I'd appreciate it if you could check that one, too.
> - Dan

For errors like these valgrind has a feature that records them (the errors) and
allows you to put them somewhere so they get excluded from future runs. I have
never done it though so I don't know if the feature works as advertised but for
sure I have been tempted enough by the various ld/dlopen issues to start me
reading the man page.

.bill

On Sat, May 13, 2006 at 01:36:54PM -0700, Dan Kegel wrote:
> There's another valgrind warning that is much more understandable, and
> I'm sending an obvious fix for it to wine-patches. (My first patch
> missed one instance, so I'm resending.) I'd appreciate it if you could
> check that one, too. - Dan

Sorry for taking a bit of time.  Here goes:
> - memset( ldtent, 0, sizeof(*ldtent) );       /* Keep valgrind happy */

I'd never do anything like that (i.e. doing something where you have to
put a comment besides that it's there to please lint, valgrind or what
else code checkers there are (compilers excluded for obvious reasons).
> -
>    /* memset above already cleared *ldtent */

Isn't this obvious?
> - *entry = null_entry;    /* Keep valgrind happy */

This one is an exception.  It replaces the two lines removed in context
and makes the code more readable -- the comment does therefore not
describe the benefits accurately and should IMO be removed.

All the best,

Leslie

[Dan Kegel]

On 5/14/06, [e-mail]  wrote:
> > - memset( ldtent, 0, sizeof(*ldtent) );       /* Keep valgrind happy */
>
> I'd never do anything like that (i.e. doing something where you have to
> put a comment besides that it's there to please lint, valgrind or what
> else code checkers there are (compilers excluded for obvious reasons).

The comment is there to mark it as something that should be gotten
rid of once we figure out the right thing to do.  I agree with you.
> > -
> >     /* memset above already cleared *ldtent */
>
> Isn't this obvious?

I suppose.
> > - *entry = null_entry;    /* Keep valgrind happy */
>
> This one is an exception.  It replaces the two lines removed in context
> and makes the code more readable -- the comment does therefore not
> describe the benefits accurately and should IMO be removed.

Thanks for the review!  It's moot, I think, since Alexandre considers
this a valgrind bug.  (I haven't looked at it in detail, so I can't say.)
- Dan