Re: [Valgrind-users] [RFH] git-difftool fails to export GIT_DIFF_TOOL when running t7800 with valgrind over ssh

[Julian S. <js...@ac...>]

That's interesting.  I remember rumours that there was a bug
in the env var handling once before, but I could not find it.
I wonder if it got filed as a bug.  Dan, one of yours maybe?

J

On Monday, June 14, 2010, Thomas Rast wrote:
> Thomas Rast wrote:
> >   0x7fffc45381fb GIT_DIFF_TOOM=bad-tool
> >   0x7fffc4538212 LD_PRELOAD=
> >   0x7fffc453821e
> >   0x7fffc453821f MORE=-sl
> >   0x7fffc4538228 GIT_DIFF_TOOK=bad-tool
> 
> Turns out that this is a bug in mash_colon_env().  Patch at the end.
> 
> The reasons it triggered in the git test suite (apart from the
> once-in-a-lifetime bad luck of hitting a crucial env var that makes
> the testcase fail):
> 
> * 'git difftool ...' is a perl script, but goes through the git
>   wrapper program and thus involves valgrind.  There is no LD_PRELOAD
>   variable at the beginning, but valgrind uses it for its purposes and
>   leaves it set-but-empty at exec() time.
> 
> * difftool invokes git-diff, which again goes through the wrapper.
> 
> * valgrind initially sets LD_PRELOAD again with the valgrind libs, but
>   this time it has a trailing : because it started out empty!
> 
> * mash_colon_env() removes the first components, but in the last
>   iteration here:
> 
> 	 if (match) {
> 	    output = entry_start;
> 	    varp++;			/* skip ':' after removed entry */  
//[1]
> 	 } else
> 	    entry_start = output+1;	/* entry starts after ':' */
>       }
> 
>       *output++ = *varp++;   //[2]
>    }
> 
>   varp is first stepped over the trailing : onto the \0 in line [1],
>   then over the \0 in line [2].  Bang, you're dead.
> 
> This can be fixed by guarding [2] so it only steps (and copies)
> anything if we're not at the end yet.
> 
> Then I believe there's another bug; admittedly I haven't looked into
> how string_match works, but for the last entry, I believe you need to
> \0-terminate the string starting from entry_start, in the same way
> that you do for all other matches.
> 
> Thanks for a great tool!
> 
> - Thomas
> 
> Index: coregrind/m_libcproc.c
> ===================================================================
> --- coregrind/m_libcproc.c	(revision 11175)
> +++ coregrind/m_libcproc.c	(working copy)
> @@ -182,9 +182,14 @@
>  	    entry_start = output+1;	/* entry starts after ':' */
>        }
> 
> -      *output++ = *varp++;
> +      if (*varp)
> +         *output++ = *varp++;
>     }
> 
> +   /* Again match against the copied entry; this time we do not have
> +      to save the old value of *output as we will zero it anyway */
> +   *output = '\0';
> +
>     /* match against the last entry */
>     if (VG_(string_match)(remove_pattern, entry_start)) {
>        output = entry_start;