git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 0/6] getenv() timing fixes

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Jan 12, 2019, 11:31 UTC
Message-ID
<87va2u3yeu.fsf@evledraar.gmail.com>
In-Reply-To
<20190111221414.GA31335@sigill.intra.peff.net>
On Fri, Jan 11 2019, Jeff King wrote:
Show 51 quoted lines
> Similar to the recent:
>
>   https://public-inbox.org/git/20190109221007.21624-1-kgybels@infogroep.be/
>
> there are some other places where we do not follow the POSIX rule that
> getenv()'s return value may be invalidated by other calls to getenv() or
> setenv().
>
> For the most part we haven't noticed because:
>
>   - on many platforms, you can call getenv() as many times as you want.
>     This changed recently in our mingw_getenv() helper, which is why
>     people are noticing now.
>
>   - calling setenv() in between _often_ works, but it depends on whether
>     libc feels like it needs to reallocate memory. Which is itself
>     platform specific, and even on a single platform may depend on
>     things like how many environment variables you have set.
>
> The first patch here is a problem somebody actually found in the wild.
> That led me to start looking through the results of:
>
>   git grep '= getenv('
>
> There are a ton of hits. I poked at the first 20 or so. A lot of them
> are fine, as they do something like this:
>
>   rla = getenv("GIT_REFLOG_ACTION");
>   strbuf_addstr("blah blah %s", rla);
>
> That's not _strictly_ correct, because strbuf_addstr() may actually look
> at the environment. But it works for our mingw_getenv() case, because
> there we use a rotating series of buffers. So as long as it doesn't look at
> 30 environment variables, we're fine. And many calls fall into that
> bucket (a more complicated one is get_ssh_command(), which runs a fair
> bit of code while holding the pointer, but ultimately probably has a
> small fixed number of opportunities to call getenv(). What is more
> worrisome is code that holds a pointer across an arbitrary number of
> calls (like once per diff'd file, or once per submodule, etc).
>
> Of course it's possible for some platform libc to use a single buffer.
> But in that case, I'd argue that the path of least resistance is
> wrapping getenv, like I described in:
>
>   https://public-inbox.org/git/20181025062037.GC11460@sigill.intra.peff.net/
>
> So anyway. Here are a handful of what seem like pretty low-hanging
> fruit. Beyond the first one, I'm not sure if they're triggerable, but
> they're easy to fix. There are 100+ grep matches that I _didn't_ audit,
> so this is by no means a complete fix. I was mostly trying to get a
> sense of how painful these fixes would be.

I wonder, and not as "you should do this" feedback on this series, just on future development, whether we shouldn't just make our own getenv() wrapper for the majority of the GIT_* variables. The semantics would be fetch value X, and if it's ever requested again return the value we found the first time.

For some things we rely on getenv(X) -> setenv(X) -> getenv(X) returning different values of X, e.g. in passing along config, but for e.g. GIT_TEST_* variables we just want to check them once, and have our own ad-hoc caches (via static variables) in a couple of places.

Maybe such an API would just loop over "environ" on startup, looking for any GIT_* variables, i.e. called from the setup.c code.

Previous: Jeff KingNext: Stefan Beller
Message 18 of 26 in “getenv() timing fixes”
  1. 0/6 getenv() timing fixesJeff King, Jan 11, 2019
  2. 1/6 get_super_prefix(): copy getenv() resultJeff King, Jan 11, 2019
  3. Junio C HamanoJan 12, 2019
  4. 2/6 commit: copy saved getenv() resultJeff King, Jan 11, 2019
  5. Junio C HamanoJan 12, 2019
  6. Jeff KingJan 12, 2019
  7. Johannes SchindelinJan 15, 2019
  8. Jeff KingJan 15, 2019
  9. Stefan BellerJan 15, 2019
  10. Jeff KingJan 15, 2019
  11. Johannes SchindelinJan 16, 2019
  12. 3/6 config: make a copy of $GIT_CONFIG stringJeff King, Jan 11, 2019
  13. 4/6 init: make a copy of $GIT_DIR stringJeff King, Jan 11, 2019
  14. Junio C HamanoJan 12, 2019
  15. 5/6 merge-recursive: copy $GITHEAD stringsJeff King, Jan 11, 2019
  16. Junio C HamanoJan 12, 2019
  17. 6/6 builtin_diff(): read $GIT_DIFF_OPTS closer to useJeff King, Jan 11, 2019
  18. Ævar Arnfjörð BjarmasonJan 12, 2019
  19. Stefan BellerJan 12, 2019
  20. Jeff KingJan 15, 2019
  21. Junio C HamanoJan 15, 2019
  22. Stefan BellerJan 15, 2019
  23. Jeff KingJan 15, 2019
  24. Jeff KingJan 15, 2019
  25. Junio C HamanoJan 15, 2019
  26. Jeff KingJan 15, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.