Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP: Address getenv() issues #2019

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

WIP: Address getenv() issues #2019

wants to merge 13 commits into from

Commits on Jan 17, 2019

  1. fixup! tests: include detailed trace logs with --write-junit-xml upon… 

    … failure

The `cut -c` approach is *per line*, not *per file* as I had thought. So
it does not work...

Signed-off-by: Johannes Schindelin <[email protected]>
    @dscho
    dscho committed Jan 17, 2019
    Configuration menu
    Copy the full SHA
    cc7f43e View commit details
    Browse the repository at this point in the history

  2. DEBUG: invalidate getenv() calls when the next one is called 

    This is a crude, and incomplete, way to diagnose getenv() issues where
the return value is not used transiently.

Signed-off-by: Johannes Schindelin <[email protected]>
    @dscho
    dscho committed Jan 17, 2019
    Configuration menu
    Copy the full SHA
    acd1aae View commit details
    Browse the repository at this point in the history

  3. DEBUG2: help diagnosing issues 

    Signed-off-by: Johannes Schindelin <[email protected]>
    @dscho
    dscho committed Jan 17, 2019
    Configuration menu
    Copy the full SHA
    48a2b7d View commit details
    Browse the repository at this point in the history

  4. WORK-AROUND: force fmt_ident(...) to succeed 

    Signed-off-by: Johannes Schindelin <[email protected]>
    @dscho
    dscho committed Jan 17, 2019
    Configuration menu
    Copy the full SHA
    5a6c20a View commit details
    Browse the repository at this point in the history

  5. DEBUG: only test on Windows 

    Signed-off-by: Johannes Schindelin <[email protected]>
    @dscho
    dscho committed Jan 17, 2019
    Configuration menu
    Copy the full SHA
    e388cfa View commit details
    Browse the repository at this point in the history

  6. TODO: left-over bit from env conversion 

    Signed-off-by: Johannes Schindelin <[email protected]>
    @dscho
    dscho committed Jan 17, 2019
    Configuration menu
    Copy the full SHA
    ce83ddc View commit details
    Browse the repository at this point in the history

  7. get_super_prefix(): copy getenv() result 

    The return value of getenv() is not guaranteed to remain valid across
multiple calls (nor across calls to setenv()). Since this function
caches the result for the length of the program, we must make a copy to
ensure that it is still valid when we need it.

Reported-by: Yngve N. Pettersen <[email protected]>
Signed-off-by: Jeff King <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
    @peff @dscho
    peff authored and dscho committed Jan 17, 2019
    Configuration menu
    Copy the full SHA
    0bf3c96 View commit details
    Browse the repository at this point in the history

  8. commit: copy saved getenv() result 

    We save the result of $GIT_INDEX_FILE so that we can restore it after
setting it to a new value and running add--interactive. However, the
pointer returned by getenv() is not guaranteed to be valid after calling
setenv(). This _usually_ works fine, but can fail if libc needs to
reallocate the environment block during the setenv().

Let's just duplicate the string, so we know that it remains valid.

In the long run it may be more robust to teach interactive_add() to take
a set of environment variables to pass along to run-command when it
execs add--interactive. And then we would not have to do this
save/restore dance at all. But this is an easy fix in the meantime.

Signed-off-by: Jeff King <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
    @peff @dscho
    peff authored and dscho committed Jan 17, 2019
    Configuration menu
    Copy the full SHA
    ce2c4c2 View commit details
    Browse the repository at this point in the history

  9. config: make a copy of $GIT_CONFIG string 

    cmd_config() points our source filename pointer at the return value of
getenv(), but that value may be invalidated by further calls to
environment functions. Let's copy it to make sure it remains valid.

We don't need to bother freeing it, as it remains part of the
whole-process global state until we exit.

Signed-off-by: Jeff King <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
    @peff @dscho
    peff authored and dscho committed Jan 17, 2019
    Configuration menu
    Copy the full SHA
    43652dc View commit details
    Browse the repository at this point in the history

  10. init: make a copy of $GIT_DIR string 

    We pass the result of getenv("GIT_DIR") to init_db() and assume that the
string remains valid. But that's not guaranteed across calls to setenv()
or even getenv(), although it often works in practice. Let's make a copy
of the string so that we follow the rules.

Note that we need to mark it with UNLEAK(), since the value persists
until the end of program (but we have no opportunity to free it).

This patch also handles $GIT_WORK_TREE the same way. It actually doesn't
have as long a lifetime and is probably fine, but it's simpler to just
treat the two side-by-side variables the same.

Signed-off-by: Jeff King <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
    @peff @dscho
    peff authored and dscho committed Jan 17, 2019
    Configuration menu
    Copy the full SHA
    5bb5a8b View commit details
    Browse the repository at this point in the history

  11. merge-recursive: copy $GITHEAD strings 

    If $GITHEAD_1234abcd is set in the environment, we use its value as a
"better branch name" in generating conflict markers. However, we pick
these better names early in the process, and the return value from
getenv() is not guaranteed to stay valid.

Let's make a copy of the returned string. And to make memory management
easier, let's just always return an allocated string from
better_branch_name(), so we know that it must always be freed.

Signed-off-by: Jeff King <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
    @peff @dscho
    peff authored and dscho committed Jan 17, 2019
    Configuration menu
    Copy the full SHA
    69cbeaa View commit details
    Browse the repository at this point in the history

  12. builtin_diff(): read $GIT_DIFF_OPTS closer to use 

    The value returned by getenv() is not guaranteed to remain valid across
other environment function calls. But in between our call and using the
value, we run fill_textconv(), which may do quite a bit of work,
including spawning sub-processes.

We can make this safer by calling getenv() right before we actually look
at its value.

Signed-off-by: Jeff King <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
    @peff @dscho
    peff authored and dscho committed Jan 17, 2019
    Configuration menu
    Copy the full SHA
    cb62289 View commit details
    Browse the repository at this point in the history

  13. TO-SPLIT 

    Signed-off-by: Johannes Schindelin <[email protected]>
    @dscho
    dscho committed Jan 17, 2019
    Configuration menu
    Copy the full SHA
    162ee3f View commit details
    Browse the repository at this point in the history