|
From: Thomas R. <tr...@st...> - 2010-06-13 10:54:54
|
Hi all,
I'm also posting to the valgrind folks in case any of you have some
idea what's going on. (Please Cc, I'm not subscribed there.) I'm
still not sure who to blame, but at this point, any sort of input
would be *great*. I put some extra comments for non-gitters in
footnotes.
This bug has been puzzling me for about a day now, and if you judged
from the subject that I'm crazy... well, I think so too.
I can reliably reproduce it on two machines:
openSUSE 11.2, x86_64, 2.6.31.12-0.2-desktop, GCC 4.5.0, valgrind 3.5.0
RHEL Server 5.4, x86_64, 2.6.18-164.el5, GCC 4.1.2, valgrind 3.5.0
While writing this I also upgraded the first one (my laptop where I do
all my development) to valgrind 3.6.0SVN, r11171. I have also used
GCC 4.4.1 instead, which is what originally shipped with openSUSE
11.2; no luck either.
Reproducing it *requires* that I'm logged in over SSH. I still have
no idea what huge difference this makes, and any pointers would be
appreciated. 'ssh localhost' will do just fine.
For instant failure, try running[*] t7800 with valgrind, e.g.
cd t/
./t7800-difftool.sh --valgrind -i
Tests 3 and 4 reliably fail for me. I focused on test 3 for now, it
just reads
diff=$(git difftool --no-prompt --tool=bad-tool branch)
test "$?" = 1 &&
test "$diff" = ""
I have tried reproducing the valgrind environment; running with
--valgrind once will set up some of the necessary glue[2], and I can
then do (assuming I'm in t/)
export GIT_VALGRIND=$(pwd)/valgrind
export GIT_EXEC_PATH=$GIT_VALGRIND/bin
export PATH=$GIT_VALGRIND/bin:$PATH
Then you can manually run the offending command with
git difftool --no-prompt --tool=bad-tool branch
This now gives me
==9204== valgrind: failed to move logfile fd into safe range, using stderr
[...more of those...]
==9276== valgrind: failed to move logfile fd into safe range, using stderr
branch
which at least verifies that something is wrong. Through way too much
fiddling, I came up with the patch at the end of the email. After
applying it, you'll have to install the env-helper:
gcc -o $GIT_EXEC_PATH/env-helper env-helper.c
After that, running
GIT_TRACE=1 git difftool --no-prompt --tool=bad-tool branch
dumps the environment in three places. I won't paste the full output,
but here's the rub:
1. Immediately after git-difftool has set up the variables. You can
see that $GIT_DIFF_TOOL is set to bad-tool, as it should.
2. Immediately before execv_shell_cmd() called by git-diff runs execvp
to run the $GIT_EXTERNAL_DIFF command. The differences to (1) as
per diffing a sorted dump are:
+GIT_DIR=.git
-LD_PRELOAD=
+LD_PRELOAD=/home/thomas/.local/lib/valgrind/vgpreload_core-amd64-linux.so:/home/thomas/.local/lib/valgrind/vgpreload_memcheck-amd64-linux.so:
-PATH=/home/thomas/dev/git/t/valgrind/bin:/home/thomas/g/t/valgrind/bin:/home/thomas/g/t/valgrind/../..:/home/thomas/g/t/valgrind/bin:/home/thomas/.local/bin:/home/thomas/.local/bin:/usr/lib64/mpi/gcc/openmpi/bin:/usr/local/bin:/usr/bin:/bin:/usr/bin/X11:/usr/X11R6/bin:/usr/games:/opt/kde3/bin:/usr/lib/mit/bin:/usr/lib/mit/sbin:/usr/sbin:/sbin:/usr/games:/usr/sbin:/sbin:/usr/games
+PATH=/home/thomas/g/t/valgrind/bin:/home/thomas/g/t/valgrind/../..:/home/thomas/dev/git/t/valgrind/bin:/home/thomas/g/t/valgrind/bin:/home/thomas/g/t/valgrind/../..:/home/thomas/g/t/valgrind/bin:/home/thomas/.local/bin:/home/thomas/.local/bin:/usr/lib64/mpi/gcc/openmpi/bin:/usr/local/bin:/usr/bin:/bin:/usr/bin/X11:/usr/X11R6/bin:/usr/games:/opt/kde3/bin:/usr/lib/mit/bin:/usr/lib/mit/sbin:/usr/sbin:/sbin:/usr/games:/usr/sbin:/sbin:/usr/games
(This is already with valgrind-3.6.0SVN installed for testing, but
the result with 3.5.0 is basically the same.)
3. Immediately after that execvp(), inside the env-helper (which is a
bogus diff tool serving only for debugging). The differences to
(2) are
+
-GIT_DIFF_TOOL=bad-tool
-LD_PRELOAD=/home/thomas/.local/lib/valgrind/vgpreload_core-amd64-linux.so:/home/thomas/.local/lib/valgrind/vgpreload_memcheck-amd64-linux.so:
+LD_PRELOAD=
At (3), $GIT_DIFF_TOOL is gone! Its "I'm doubting my sanity" friends
$GIT_DIFF_TOO[KM] aren't though. Also, that is *not* a stray +, there
really is an empty entry in the environment! In the output of
env-helper, it looks like:
0x7fffc45381fb GIT_DIFF_TOOM=bad-tool
0x7fffc4538212 LD_PRELOAD=
0x7fffc453821e
0x7fffc453821f MORE=-sl
0x7fffc4538228 GIT_DIFF_TOOK=bad-tool
Eh? Is this even allowed? Probably not, right?
BTW, the empty LD_PRELOAD before step (1) is already valgrind's; I do
not have it set in the shell.
That's about as far as I got. There's a bug report that seems to run
into a similar problem at
http://bugs.kde.org/show_bug.cgi?id=215914
but it ends up being blamed on the shell. AFAICS there is no shell
involved in our case between (2) and (3) since git only uses one if
the command has characters that "require a shell".
Any pointers would be welcome...
[1] You'll first need git sources, try
git clone git://git.kernel.org/pub/scm/git/git.git
cd git
make
though if you actually want to install it, you should first read
the top of the Makefile and at least set 'prefix'.
[2] The --valgrind support sets up trampolines that execute git
commands written in C under valgrind in t/valgrind/bin/.
--- 8< ---
diff --git i/git-difftool.perl w/git-difftool.perl
index adc42de..ddf0330 100755
--- i/git-difftool.perl
+++ w/git-difftool.perl
@@ -34,7 +34,7 @@ sub setup_environment
{
$ENV{PATH} = "$DIR:$ENV{PATH}";
$ENV{GIT_PAGER} = '';
- $ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
+ $ENV{GIT_EXTERNAL_DIFF} = 'env-helper';
}
sub exe
@@ -107,10 +107,16 @@ sub generate_command
setup_environment();
+my @cmd = generate_command();
+$ENV{GIT_DIFF_TOOM} = $ENV{GIT_DIFF_TOOK} = $ENV{GIT_DIFF_TOOL};
+foreach my $k (keys %ENV) {
+ print STDERR "$k=$ENV{$k}\n"
+}
+
# ActiveState Perl for Win32 does not implement POSIX semantics of
# exec* system call. It just spawns the given executable and finishes
# the starting program, exiting with code 0.
# system will at least catch the errors returned by git diff,
# allowing the caller of git difftool better handling of failures.
-my $rc = system(generate_command());
+my $rc = system(@cmd);
exit($rc | ($rc >> 8));
diff --git i/run-command.c w/run-command.c
index 2a1041e..5cb0475 100644
--- i/run-command.c
+++ w/run-command.c
@@ -54,6 +54,11 @@ static const char **prepare_shell_cmd(const char **argv)
static int execv_shell_cmd(const char **argv)
{
const char **nargv = prepare_shell_cmd(argv);
+ if (1) {
+ char **p;
+ for (p = environ; *p; p++)
+ fprintf(stderr, "%p %s\n", *p, *p);
+ }
trace_argv_printf(nargv, "trace: exec:");
execvp(nargv[0], (char **)nargv);
free(nargv);
diff --git i/t/env-helper.c w/t/env-helper.c
index e69de29..3fe6053 100644
--- i/t/env-helper.c
+++ w/t/env-helper.c
@@ -0,0 +1,8 @@
+#include <stdio.h>
+
+int main (int argc, char *argv[], char **environ)
+{
+ char **p;
+ for (p = environ; *p; p++)
+ fprintf(stderr, "%p %s\n", *p, *p);
+}
diff --git i/t/valgrind/valgrind.sh w/t/valgrind/valgrind.sh
index 582b4dc..d5b9874 100755
--- i/t/valgrind/valgrind.sh
+++ w/t/valgrind/valgrind.sh
@@ -16,7 +16,5 @@ exec valgrind -q --error-exitcode=126 \
--suppressions="$GIT_VALGRIND/default.supp" \
--gen-suppressions=all \
$TRACK_ORIGINS \
- --log-fd=4 \
- --input-fd=4 \
$GIT_VALGRIND_OPTIONS \
"$GIT_VALGRIND"/../../"$base" "$@"
--
Thomas Rast
trast@{inf,student}.ethz.ch
|
|
From: Thomas R. <tr...@st...> - 2010-06-14 15:45:27
|
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;
--
Thomas Rast
trast@{inf,student}.ethz.ch
|
|
From: Julian S. <js...@ac...> - 2010-06-14 16:06:10
|
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;
|
|
From: Thomas R. <tr...@st...> - 2010-06-14 16:13:53
|
Julian Seward wrote: > > 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 Sorry, I should have quoted the link, I had it in the original post: http://bugs.kde.org/show_bug.cgi?id=215914 The symptomps match so I think it's this bug. -- Thomas Rast trast@{inf,student}.ethz.ch |
|
From: Julian S. <js...@ac...> - 2010-06-15 11:44:15
|
On Monday, June 14, 2010, Thomas Rast wrote: > Sorry, I should have quoted the link, I had it in the original post: > > http://bugs.kde.org/show_bug.cgi?id=215914 > > The symptomps match so I think it's this bug. Please could you add your analysis and patch to the above URL. Bug reports and fixes that are only reported by email and are not in the bug database tend to become lost or forgotten; the bug database is a much better place for them. J |