Bug 215914 - Valgrind inserts bogus empty environment variable
Summary: Valgrind inserts bogus empty environment variable
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-24 00:48 UTC by Dan Kegel
Modified: 2010-07-21 16:13 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
patch that does debug printing in the env mangler (1.74 KB, patch)
2010-02-09 20:28 UTC, Julian Seward
Details
Proposed fix for string overrun. (647 bytes, text/plain)
2010-06-15 13:57 UTC, Thomas Rast
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Kegel 2009-11-24 00:48:14 UTC
Version:            (using Devel)
OS:                Linux
Installed from:    Compiled sources

The command
valgrind --trace-children=yes /bin/sh -c "/bin/sh -c env"
yields incorrect output on my system (r10990).

It seems to be lacking the LANG environment variable,
but it has an extra blank line.

I first ran into this running wine under valgrind,
where the symptom was missing environment variables.
I managed to track it down a bit; in
coregrind/m_initimg/initimg-linux.c, on entry to
setup_client_env(), on about the fourth time through,
there was an empty environment variable (no name,
no =, no value) inserted after LD_PRELOAD=, i.e.
     "LD_PRELOAD="
     ""
     "XAUTHORITY=/home/dank/.Xauthority"
and wine (instead of printing a blank line as env does)
truncated the environment at that point.
Could the LD_PRELOAD munging in that file have
misbehaved somehow?
Comment 1 Dan Kegel 2010-01-31 16:56:38 UTC
Wine users can apply the crude workaround which I posted at
  http://www.winehq.org/pipermail/wine-patches/2009-November/081801.html
but it's not a fix.
Comment 2 Julian Seward 2010-02-09 20:20:08 UTC
I can reproduce this, but it doesn't seem to me to be a bug in
Valgrind.  Or at least not in the env mangler in m_initimg.  In the
test case I'm working with, every time the output mangled environment
contains a blank line, the input environment contained a blank line
too.  So the mangler itself isn't creating them.

If Wine truncates the environment at a blank line, isn't that a bug in
Wine?  It really ought to regard the environment as being terminated
by a (char*)NULL, not a blank line.

I note in passing that I can reproduce this on Ubuntu 9.10 but not on
openSUSE 11.0.  So perhaps the presence of the blank line is somehow
to do with the dash-vs-bash difference?

Am leaning towards closing this as invalid.
Comment 3 Julian Seward 2010-02-09 20:28:57 UTC
Created attachment 40622 [details]
patch that does debug printing in the env mangler

Using this patch, it's possible to see that (at least in Dan's
test case) whenever a blank line exists in the final environment
("FINAL:  ") then it also exists in the incoming environment
("BEFORE: ").
Comment 4 Dan Kegel 2010-02-09 20:43:42 UTC
I'm stuck somewhere with only a Windows box at the moment,
but the next step seems to be to try to reproduce without
valgrind, and file a bug with the shell?
Comment 5 Julian Seward 2010-02-15 10:56:33 UTC
I still think I should WFM this; nevertheless .. the debug printing
got committed as r11041.
Comment 6 Thomas Rast 2010-06-15 13:54:32 UTC
This is a bug in mash_colon_env(), as explained in
  http://sourceforge.net/mailarchive/forum.php?thread_name=201006141813.45168.trast%40student.ethz.ch&forum_name=valgrind-users

I'm quoting the resolution part here; the original story of how I hit it is in the thread starter. I'll attach the patch that I'm referring to.

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.
Comment 7 Thomas Rast 2010-06-15 13:57:52 UTC
Created attachment 48031 [details]
Proposed fix for string overrun.
Comment 8 Tom Hughes 2010-06-15 14:49:03 UTC
The patch is clearly correct, so I've committed it as r11178.

I can't actually reproduce the bug here however so if somebody else could confirm that it is now fixed that would be great.
Comment 9 Thomas Rast 2010-06-15 20:21:15 UTC
Thanks a lot for the quick response times in discussing and committing this!

Reproducing the symptom itself is not so hard: Call the following program exec-env-helper.c and compile it.

--- 8< ---
#include <unistd.h>

extern char **environ;

int main (int argc, char *argv[])
{
	char *new_argv[] = { "env", NULL };

	return execvp(new_argv[0], new_argv);
}
--- >8 ---

Then run

  export LD_PRELOAD=  # this simulates the effect of one layer of valgrind
  env > expected
  valgrind ./exec-env-helper > actual
  diff -u expected actual

The diff will show that a random variable has been deleted. On the other hand, the fixed version merely shuffles things around a bit and updates the bash-magic $_ variable.
Comment 10 Julian Seward 2010-07-21 12:45:42 UTC
Thomas, could you please verify this bug is fixed in the trunk,
so that we can then close the report?
Comment 11 Thomas Rast 2010-07-21 16:04:42 UTC
Oh, my apologies for not making this explicit in my last comment.  I of course ran the testcase I provided there on all versions to verify that the committed fix works. I have just run it again and it also passes in r11217. Thanks!
Comment 12 Julian Seward 2010-07-21 16:13:38 UTC
Thanks.  Closing.