{"thread":{"id":"24095","subject":"[RFH] git-difftool fails to export GIT_DIFF_TOOL when running t7800 with valgrind over ssh","startedAt":"2010-06-13T10:51:10Z","lastAt":"2010-06-14T15:03:28Z","messageCount":2,"participants":["Thomas Rast"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"143605","messageId":"201006131251.11229.trast@student.ethz.ch","threadId":"24095","inReplyTo":null,"subject":"[RFH] git-difftool fails to export GIT_DIFF_TOOL when running t7800 with valgrind over ssh","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-06-13T10:51:10Z","receivedAt":"2010-06-13T10:51:10Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Hi all,\n\nI'm also posting to the valgrind folks in case any of you have some\nidea what's going on.  (Please Cc, I'm not subscribed there.)  I'm\nstill not sure who to blame, but at this point, any sort of input\nwould be *great*.  I put some extra comments for non-gitters in\nfootnotes.\n\nThis bug has been puzzling me for about a day now, and if you judged\nfrom the subject that I'm crazy... well, I think so too.\n\nI can reliably reproduce it on two machines:\n\n  openSUSE 11.2, x86_64, 2.6.31.12-0.2-desktop, GCC 4.5.0, valgrind 3.5.0\n  RHEL Server 5.4, x86_64, 2.6.18-164.el5, GCC 4.1.2, valgrind 3.5.0\n\nWhile writing this I also upgraded the first one (my laptop where I do\nall my development) to valgrind 3.6.0SVN, r11171.  I have also used\nGCC 4.4.1 instead, which is what originally shipped with openSUSE\n11.2; no luck either.\n\nReproducing it *requires* that I'm logged in over SSH.  I still have\nno idea what huge difference this makes, and any pointers would be\nappreciated.  'ssh localhost' will do just fine.\n\nFor instant failure, try running[*] t7800 with valgrind, e.g.\n\n  cd t/\n  ./t7800-difftool.sh --valgrind -i\n\nTests 3 and 4 reliably fail for me.  I focused on test 3 for now, it\njust reads\n\n\tdiff=$(git difftool --no-prompt --tool=bad-tool branch)\n\ttest \"$?\" = 1 &&\n\ttest \"$diff\" = \"\"\n\nI have tried reproducing the valgrind environment; running with\n--valgrind once will set up some of the necessary glue[2], and I can\nthen do (assuming I'm in t/)\n\n  export GIT_VALGRIND=$(pwd)/valgrind\n  export GIT_EXEC_PATH=$GIT_VALGRIND/bin\n  export PATH=$GIT_VALGRIND/bin:$PATH\n\nThen you can manually run the offending command with\n\n  git difftool --no-prompt --tool=bad-tool branch\n\nThis now gives me\n\n  ==9204== valgrind: failed to move logfile fd into safe range, using stderr\n  [...more of those...]\n  ==9276== valgrind: failed to move logfile fd into safe range, using stderr\n  branch\n\nwhich at least verifies that something is wrong.  Through way too much\nfiddling, I came up with the patch at the end of the email.  After\napplying it, you'll have to install the env-helper:\n\n  gcc -o $GIT_EXEC_PATH/env-helper env-helper.c\n\nAfter that, running\n\n  GIT_TRACE=1 git difftool --no-prompt --tool=bad-tool branch\n\ndumps the environment in three places.  I won't paste the full output,\nbut here's the rub:\n\n1. Immediately after git-difftool has set up the variables.  You can\n   see that $GIT_DIFF_TOOL is set to bad-tool, as it should.\n\n2. Immediately before execv_shell_cmd() called by git-diff runs execvp\n   to run the $GIT_EXTERNAL_DIFF command.  The differences to (1) as\n   per diffing a sorted dump are:\n\n     +GIT_DIR=.git\n\n     -LD_PRELOAD=\n     +LD_PRELOAD=/home/thomas/.local/lib/valgrind/vgpreload_core-amd64-linux.so:/home/thomas/.local/lib/valgrind/vgpreload_memcheck-amd64-linux.so:\n\n     -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\n     +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\n\n   (This is already with valgrind-3.6.0SVN installed for testing, but\n   the result with 3.5.0 is basically the same.)\n\n3. Immediately after that execvp(), inside the env-helper (which is a\n   bogus diff tool serving only for debugging).  The differences to\n   (2) are\n\n     +\n\n     -GIT_DIFF_TOOL=bad-tool\n\n     -LD_PRELOAD=/home/thomas/.local/lib/valgrind/vgpreload_core-amd64-linux.so:/home/thomas/.local/lib/valgrind/vgpreload_memcheck-amd64-linux.so:\n     +LD_PRELOAD=\n\n\nAt (3), $GIT_DIFF_TOOL is gone!  Its \"I'm doubting my sanity\" friends\n$GIT_DIFF_TOO[KM] aren't though.  Also, that is *not* a stray +, there\nreally is an empty entry in the environment!  In the output of\nenv-helper, it looks like:\n\n  0x7fffc45381fb GIT_DIFF_TOOM=bad-tool\n  0x7fffc4538212 LD_PRELOAD=\n  0x7fffc453821e \n  0x7fffc453821f MORE=-sl\n  0x7fffc4538228 GIT_DIFF_TOOK=bad-tool\n\nEh?  Is this even allowed?  Probably not, right?\n\nBTW, the empty LD_PRELOAD before step (1) is already valgrind's; I do\nnot have it set in the shell.\n\nThat's about as far as I got.  There's a bug report that seems to run\ninto a similar problem at\n\n  http://bugs.kde.org/show_bug.cgi?id=215914\n\nbut it ends up being blamed on the shell.  AFAICS there is no shell\ninvolved in our case between (2) and (3) since git only uses one if\nthe command has characters that \"require a shell\".\n\nAny pointers would be welcome...\n\n\n\n[1] You'll first need git sources, try\n      git clone git://git.kernel.org/pub/scm/git/git.git\n      cd git\n      make\n    though if you actually want to install it, you should first read\n    the top of the Makefile and at least set 'prefix'.\n\n[2] The --valgrind support sets up trampolines that execute git\n    commands written in C under valgrind in t/valgrind/bin/.  \n\n\n--- 8< ---\ndiff --git i/git-difftool.perl w/git-difftool.perl\nindex adc42de..ddf0330 100755\n--- i/git-difftool.perl\n+++ w/git-difftool.perl\n@@ -34,7 +34,7 @@ sub setup_environment\n {\n \t$ENV{PATH} = \"$DIR:$ENV{PATH}\";\n \t$ENV{GIT_PAGER} = '';\n-\t$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n+\t$ENV{GIT_EXTERNAL_DIFF} = 'env-helper';\n }\n \n sub exe\n@@ -107,10 +107,16 @@ sub generate_command\n \n setup_environment();\n \n+my @cmd = generate_command();\n+$ENV{GIT_DIFF_TOOM} = $ENV{GIT_DIFF_TOOK} = $ENV{GIT_DIFF_TOOL};\n+foreach my $k (keys %ENV) {\n+\tprint STDERR \"$k=$ENV{$k}\\n\"\n+}\n+\n # ActiveState Perl for Win32 does not implement POSIX semantics of\n # exec* system call. It just spawns the given executable and finishes\n # the starting program, exiting with code 0.\n # system will at least catch the errors returned by git diff,\n # allowing the caller of git difftool better handling of failures.\n-my $rc = system(generate_command());\n+my $rc = system(@cmd);\n exit($rc | ($rc >> 8));\ndiff --git i/run-command.c w/run-command.c\nindex 2a1041e..5cb0475 100644\n--- i/run-command.c\n+++ w/run-command.c\n@@ -54,6 +54,11 @@ static const char **prepare_shell_cmd(const char **argv)\n static int execv_shell_cmd(const char **argv)\n {\n \tconst char **nargv = prepare_shell_cmd(argv);\n+\tif (1) {\n+\t\tchar **p;\n+\t\tfor (p = environ; *p; p++)\n+\t\t\tfprintf(stderr, \"%p %s\\n\", *p, *p);\n+\t}\n \ttrace_argv_printf(nargv, \"trace: exec:\");\n \texecvp(nargv[0], (char **)nargv);\n \tfree(nargv);\ndiff --git i/t/env-helper.c w/t/env-helper.c\nindex e69de29..3fe6053 100644\n--- i/t/env-helper.c\n+++ w/t/env-helper.c\n@@ -0,0 +1,8 @@\n+#include <stdio.h>\n+\n+int main (int argc, char *argv[], char **environ)\n+{\n+\tchar **p;\n+\tfor (p = environ; *p; p++)\n+\t\tfprintf(stderr, \"%p %s\\n\", *p, *p);\n+}\ndiff --git i/t/valgrind/valgrind.sh w/t/valgrind/valgrind.sh\nindex 582b4dc..d5b9874 100755\n--- i/t/valgrind/valgrind.sh\n+++ w/t/valgrind/valgrind.sh\n@@ -16,7 +16,5 @@ exec valgrind -q --error-exitcode=126 \\\n \t--suppressions=\"$GIT_VALGRIND/default.supp\" \\\n \t--gen-suppressions=all \\\n \t$TRACK_ORIGINS \\\n-\t--log-fd=4 \\\n-\t--input-fd=4 \\\n \t$GIT_VALGRIND_OPTIONS \\\n \t\"$GIT_VALGRIND\"/../../\"$base\" \"$@\"\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"143659","messageId":"201006141703.28488.trast@student.ethz.ch","threadId":"24095","inReplyTo":"201006131251.11229.trast@student.ethz.ch","subject":"Re: [RFH] git-difftool fails to export GIT_DIFF_TOOL when running t7800 with valgrind over ssh","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-06-14T15:03:28Z","receivedAt":"2010-06-14T15:03:28Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Thomas Rast wrote:\n>   0x7fffc45381fb GIT_DIFF_TOOM=bad-tool\n>   0x7fffc4538212 LD_PRELOAD=\n>   0x7fffc453821e \n>   0x7fffc453821f MORE=-sl\n>   0x7fffc4538228 GIT_DIFF_TOOK=bad-tool\n\nIf you considered looking into it, don't; it is a subtle string\noverrun bug in valgrind's environment mangling.\n\n(Thanks anyway if you tried...  I really lost hope for a while there)\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"}]}