Message ID | 20190109221007.21624-1-kgybels@infogroep.be (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | diff: ensure correct lifetime of external_diff_cmd | expand |
On Wed, Jan 9, 2019 at 5:19 PM Kim Gybels <kgybels@infogroep.be> wrote: > According to getenv(3)'s notes: > [...] > Since strings returned by getenv() are allowed to change on subsequent > calls to getenv(), make sure to duplicate when caching external_diff_cmd > from environment. > [...] > Signed-off-by: Kim Gybels <kgybels@infogroep.be> > --- > diff --git a/diff.c b/diff.c > @@ -492,6 +492,9 @@ static const char *external_diff(void) > external_diff_cmd = getenv("GIT_EXTERNAL_DIFF"); > if (!external_diff_cmd) > external_diff_cmd = external_diff_cmd_cfg; > + else > + external_diff_cmd = xstrdup(external_diff_cmd); Make sense. Not shown in the context is that 'external_diff_cmd' is static, so this is not (in the traditional sense) leaking the dup'd string. I do find that the logic is obscured by doing the xstrdup() in the 'else' arm; it would be easier to grok if the condition was reversed and xstrdup() done in the 'then' arm. However, you might also consider using xstrdup_or_null(), like this: external_diff_cmd = xstrdup_or_null(getenv(...)); if (!external_diff_cmd) ...as before... > done_preparing = 1; > return external_diff_cmd; > }
Hi, On Wed, 9 Jan 2019, Eric Sunshine wrote: > On Wed, Jan 9, 2019 at 5:19 PM Kim Gybels <kgybels@infogroep.be> wrote: > > According to getenv(3)'s notes: > > [...] > > Since strings returned by getenv() are allowed to change on subsequent > > calls to getenv(), make sure to duplicate when caching external_diff_cmd > > from environment. > > [...] > > Signed-off-by: Kim Gybels <kgybels@infogroep.be> > > --- > > diff --git a/diff.c b/diff.c > > @@ -492,6 +492,9 @@ static const char *external_diff(void) > > external_diff_cmd = getenv("GIT_EXTERNAL_DIFF"); > > if (!external_diff_cmd) > > external_diff_cmd = external_diff_cmd_cfg; > > + else > > + external_diff_cmd = xstrdup(external_diff_cmd); > > Make sense. > > Not shown in the context is that 'external_diff_cmd' is static, so > this is not (in the traditional sense) leaking the dup'd string. Ah! And that also explains why we do not need to take care of releasing the memory via `free()` (which is what I was wondering about). > I do find that the logic is obscured by doing the xstrdup() in the > 'else' arm; it would be easier to grok if the condition was reversed and > xstrdup() done in the 'then' arm. > > However, you might also consider using xstrdup_or_null(), like this: > > external_diff_cmd = xstrdup_or_null(getenv(...)); > if (!external_diff_cmd) > ...as before... > > > done_preparing = 1; > > return external_diff_cmd; > > } I like this version slightly better, too. Thanks for diagnosing and fixing this annoying bug! Dscho
Eric Sunshine <sunshine@sunshineco.com> writes: > However, you might also consider using xstrdup_or_null(), like this: > > external_diff_cmd = xstrdup_or_null(getenv(...)); > if (!external_diff_cmd) > ...as before... > >> done_preparing = 1; >> return external_diff_cmd; >> } Looks good.
diff --git a/diff.c b/diff.c index dc9965e836..f69687e288 100644 --- a/diff.c +++ b/diff.c @@ -492,6 +492,9 @@ static const char *external_diff(void) external_diff_cmd = getenv("GIT_EXTERNAL_DIFF"); if (!external_diff_cmd) external_diff_cmd = external_diff_cmd_cfg; + else + external_diff_cmd = xstrdup(external_diff_cmd); + done_preparing = 1; return external_diff_cmd; }
According to getenv(3)'s notes: The implementation of getenv() is not required to be reentrant. The string pointed to by the return value of getenv() may be statically allocated, and can be modified by a subsequent call to getenv(), putenv(3), setenv(3), or unsetenv(3). Since strings returned by getenv() are allowed to change on subsequent calls to getenv(), make sure to duplicate when caching external_diff_cmd from environment. This problem becomes apparent on Git for Windows since fe21c6b285df (mingw: reencode environment variables on the fly (UTF-16 <-> UTF-8)), when the getenv() implementation provided in compat/mingw.c was changed to keep a certain amount of alloc'ed strings and freeing them on subsequent calls. This fixes https://github.com/git-for-windows/git/issues/2007: $ yes n | git -c difftool.prompt=yes difftool fe21c6b285df fe21c6b285df~100 Viewing (1/404): '.gitignore' Launch 'bc3' [Y/n]? Viewing (2/404): 'Documentation/.gitignore' Launch 'bc3' [Y/n]? Viewing (3/404): 'Documentation/Makefile' Launch 'bc3' [Y/n]? Viewing (4/404): 'Documentation/RelNotes/2.14.5.txt' Launch 'bc3' [Y/n]? Viewing (5/404): 'Documentation/RelNotes/2.15.3.txt' Launch 'bc3' [Y/n]? Viewing (6/404): 'Documentation/RelNotes/2.16.5.txt' Launch 'bc3' [Y/n]? Viewing (7/404): 'Documentation/RelNotes/2.17.2.txt' Launch 'bc3' [Y/n]? Viewing (8/404): 'Documentation/RelNotes/2.18.1.txt' Launch 'bc3' [Y/n]? Viewing (9/404): 'Documentation/RelNotes/2.19.0.txt' Launch 'bc3' [Y/n]? error: cannot spawn ¦?: No such file or directory fatal: external diff died, stopping at Documentation/RelNotes/2.19.1.txt Signed-off-by: Kim Gybels <kgybels@infogroep.be> --- diff.c | 3 +++ 1 file changed, 3 insertions(+)