{"thread":{"id":"50191","subject":"[PATCH] diff: ensure correct lifetime of external_diff_cmd","startedAt":"2019-01-09T22:19:03Z","lastAt":"2019-01-12T02:44:10Z","messageCount":6,"participants":["Kim Gybels","Eric Sunshine","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"366453","messageId":"20190109221007.21624-1-kgybels@infogroep.be","threadId":"50191","inReplyTo":null,"subject":"[PATCH] diff: ensure correct lifetime of external_diff_cmd","fromName":"Kim Gybels","fromEmail":"kgybels@infogroep.be","sentAt":"2019-01-09T22:10:07Z","receivedAt":"2019-01-09T22:19:03Z","isPatch":true,"sender":{"key":"kgybels@infogroep.be","avatar":"https://avatars.githubusercontent.com/u/2051188?v=4"},"body":"According to getenv(3)'s notes:\n\n The implementation of getenv() is not required to be reentrant. The\n string pointed to by the return value of getenv() may be statically\n allocated, and can be modified by a subsequent call to getenv(),\n putenv(3), setenv(3), or unsetenv(3).\n\nSince strings returned by getenv() are allowed to change on subsequent\ncalls to getenv(), make sure to duplicate when caching external_diff_cmd\nfrom environment.\n\nThis problem becomes apparent on Git for Windows since fe21c6b285df\n(mingw: reencode environment variables on the fly (UTF-16 <-> UTF-8)),\nwhen the getenv() implementation provided in compat/mingw.c was changed\nto keep a certain amount of alloc'ed strings and freeing them on\nsubsequent calls.\n\nThis fixes https://github.com/git-for-windows/git/issues/2007:\n\n $ yes n | git -c difftool.prompt=yes difftool fe21c6b285df fe21c6b285df~100\n\n Viewing (1/404): '.gitignore'\n Launch 'bc3' [Y/n]?\n Viewing (2/404): 'Documentation/.gitignore'\n Launch 'bc3' [Y/n]?\n Viewing (3/404): 'Documentation/Makefile'\n Launch 'bc3' [Y/n]?\n Viewing (4/404): 'Documentation/RelNotes/2.14.5.txt'\n Launch 'bc3' [Y/n]?\n Viewing (5/404): 'Documentation/RelNotes/2.15.3.txt'\n Launch 'bc3' [Y/n]?\n Viewing (6/404): 'Documentation/RelNotes/2.16.5.txt'\n Launch 'bc3' [Y/n]?\n Viewing (7/404): 'Documentation/RelNotes/2.17.2.txt'\n Launch 'bc3' [Y/n]?\n Viewing (8/404): 'Documentation/RelNotes/2.18.1.txt'\n Launch 'bc3' [Y/n]?\n Viewing (9/404): 'Documentation/RelNotes/2.19.0.txt'\n Launch 'bc3' [Y/n]? error: cannot spawn ¦?: No such file or directory\n fatal: external diff died, stopping at Documentation/RelNotes/2.19.1.txt\n\nSigned-off-by: Kim Gybels \n---\n diff.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex dc9965e836..f69687e288 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -492,6 +492,9 @@ static const char *external_diff(void)\n \texternal_diff_cmd = getenv(\"GIT_EXTERNAL_DIFF\");\n \tif (!external_diff_cmd)\n \t\texternal_diff_cmd = external_diff_cmd_cfg;\n+\telse\n+\t\texternal_diff_cmd = xstrdup(external_diff_cmd);\n+\n \tdone_preparing = 1;\n \treturn external_diff_cmd;\n }\n-- \n2.20.1.windows.1\n\n"},{"id":"366460","messageId":"CAPig+cQKnEWb+co_NJ0UyZbXZrvx2KsbS_ZugdyjjYZcz8tjvw@mail.gmail.com","threadId":"50191","inReplyTo":"20190109221007.21624-1-kgybels@infogroep.be","subject":"Re: [PATCH] diff: ensure correct lifetime of external_diff_cmd","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-01-09T23:10:58Z","receivedAt":"2019-01-09T23:11:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 9, 2019 at 5:19 PM Kim Gybels wrote:\n> According to getenv(3)'s notes:\n> [...]\n> Since strings returned by getenv() are allowed to change on subsequent\n> calls to getenv(), make sure to duplicate when caching external_diff_cmd\n> from environment.\n> [...]\n> Signed-off-by: Kim Gybels \n> ---\n> diff --git a/diff.c b/diff.c\n> @@ -492,6 +492,9 @@ static const char *external_diff(void)\n> external_diff_cmd = getenv(\"GIT_EXTERNAL_DIFF\");\n> if (!external_diff_cmd)\n> external_diff_cmd = external_diff_cmd_cfg;\n> + else\n> + external_diff_cmd = xstrdup(external_diff_cmd);\n\nMake sense.\n\nNot shown in the context is that 'external_diff_cmd' is static, so\nthis is not (in the traditional sense) leaking the dup'd string.\n\nI do find that the logic is obscured by doing the xstrdup() in the\n'else' arm; it would be easier to grok if the condition was reversed\nand xstrdup() done in the 'then' arm.\n\nHowever, you might also consider using xstrdup_or_null(), like this:\n\n external_diff_cmd = xstrdup_or_null(getenv(...));\n if (!external_diff_cmd)\n ...as before...\n\n> done_preparing = 1;\n> return external_diff_cmd;\n> }\n"},{"id":"366505","messageId":"nycvar.QRO.7.76.6.1901101646050.41@tvgsbejvaqbjf.bet","threadId":"50191","inReplyTo":"CAPig+cQKnEWb+co_NJ0UyZbXZrvx2KsbS_ZugdyjjYZcz8tjvw@mail.gmail.com","subject":"Re: [PATCH] diff: ensure correct lifetime of external_diff_cmd","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-10T15:47:02Z","receivedAt":"2019-01-10T15:47:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 9 Jan 2019, Eric Sunshine wrote:\n\n> On Wed, Jan 9, 2019 at 5:19 PM Kim Gybels wrote:\n> > According to getenv(3)'s notes:\n> > [...]\n> > Since strings returned by getenv() are allowed to change on subsequent\n> > calls to getenv(), make sure to duplicate when caching external_diff_cmd\n> > from environment.\n> > [...]\n> > Signed-off-by: Kim Gybels \n> > ---\n> > diff --git a/diff.c b/diff.c\n> > @@ -492,6 +492,9 @@ static const char *external_diff(void)\n> > external_diff_cmd = getenv(\"GIT_EXTERNAL_DIFF\");\n> > if (!external_diff_cmd)\n> > external_diff_cmd = external_diff_cmd_cfg;\n> > + else\n> > + external_diff_cmd = xstrdup(external_diff_cmd);\n> \n> Make sense.\n> \n> Not shown in the context is that 'external_diff_cmd' is static, so\n> this is not (in the traditional sense) leaking the dup'd string.\n\nAh! And that also explains why we do not need to take care of releasing\nthe memory via `free()` (which is what I was wondering about).\n\n> I do find that the logic is obscured by doing the xstrdup() in the\n> 'else' arm; it would be easier to grok if the condition was reversed and\n> xstrdup() done in the 'then' arm.\n> \n> However, you might also consider using xstrdup_or_null(), like this:\n> \n> external_diff_cmd = xstrdup_or_null(getenv(...));\n> if (!external_diff_cmd)\n> ...as before...\n> \n> > done_preparing = 1;\n> > return external_diff_cmd;\n> > }\n\nI like this version slightly better, too.\n\nThanks for diagnosing and fixing this annoying bug!\nDscho\n"},{"id":"366514","messageId":"xmqqh8eg2wsc.fsf@gitster-ct.c.googlers.com","threadId":"50191","inReplyTo":"CAPig+cQKnEWb+co_NJ0UyZbXZrvx2KsbS_ZugdyjjYZcz8tjvw@mail.gmail.com","subject":"Re: [PATCH] diff: ensure correct lifetime of external_diff_cmd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-10T18:27:15Z","receivedAt":"2019-01-10T18:27:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> However, you might also consider using xstrdup_or_null(), like this:\n>\n> external_diff_cmd = xstrdup_or_null(getenv(...));\n> if (!external_diff_cmd)\n> ...as before...\n>\n>> done_preparing = 1;\n>> return external_diff_cmd;\n>> }\n\nLooks good.\n"},{"id":"366574","messageId":"20190111202608.10576-1-kgybels@infogroep.be","threadId":"50191","inReplyTo":"20190109221007.21624-1-kgybels@infogroep.be","subject":"[PATCH v2] diff: ensure correct lifetime of external_diff_cmd","fromName":"Kim Gybels","fromEmail":"kgybels@infogroep.be","sentAt":"2019-01-11T20:26:08Z","receivedAt":"2019-01-11T20:27:19Z","isPatch":true,"sender":{"key":"kgybels@infogroep.be","avatar":"https://avatars.githubusercontent.com/u/2051188?v=4"},"body":"According to getenv(3)'s notes:\n\n The implementation of getenv() is not required to be reentrant. The\n string pointed to by the return value of getenv() may be statically\n allocated, and can be modified by a subsequent call to getenv(),\n putenv(3), setenv(3), or unsetenv(3).\n\nSince strings returned by getenv() are allowed to change on subsequent\ncalls to getenv(), make sure to duplicate when caching external_diff_cmd\nfrom environment.\n\nThis problem becomes apparent on Git for Windows since fe21c6b285df\n(mingw: reencode environment variables on the fly (UTF-16 <-> UTF-8)),\nwhen the getenv() implementation provided in compat/mingw.c was changed\nto keep a certain amount of alloc'ed strings and freeing them on\nsubsequent calls.\n\nThis fixes https://github.com/git-for-windows/git/issues/2007:\n\n $ yes n | git -c difftool.prompt=yes difftool fe21c6b285df fe21c6b285df~100\n\n Viewing (1/404): '.gitignore'\n Launch 'bc3' [Y/n]?\n Viewing (2/404): 'Documentation/.gitignore'\n Launch 'bc3' [Y/n]?\n Viewing (3/404): 'Documentation/Makefile'\n Launch 'bc3' [Y/n]?\n Viewing (4/404): 'Documentation/RelNotes/2.14.5.txt'\n Launch 'bc3' [Y/n]?\n Viewing (5/404): 'Documentation/RelNotes/2.15.3.txt'\n Launch 'bc3' [Y/n]?\n Viewing (6/404): 'Documentation/RelNotes/2.16.5.txt'\n Launch 'bc3' [Y/n]?\n Viewing (7/404): 'Documentation/RelNotes/2.17.2.txt'\n Launch 'bc3' [Y/n]?\n Viewing (8/404): 'Documentation/RelNotes/2.18.1.txt'\n Launch 'bc3' [Y/n]?\n Viewing (9/404): 'Documentation/RelNotes/2.19.0.txt'\n Launch 'bc3' [Y/n]? error: cannot spawn ¦?: No such file or directory\n fatal: external diff died, stopping at Documentation/RelNotes/2.19.1.txt\n\nSigned-off-by: Kim Gybels \n---\n\nUses xstrdup_or_null as suggested by Eric Sunshine.\n\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex dc9965e836..5634992bbc 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -487,11 +487,11 @@ static const char *external_diff(void)\n \tstatic const char *external_diff_cmd = NULL;\n \tstatic int done_preparing = 0;\n \n \tif (done_preparing)\n \t\treturn external_diff_cmd;\n-\texternal_diff_cmd = getenv(\"GIT_EXTERNAL_DIFF\");\n+\texternal_diff_cmd = xstrdup_or_null(getenv(\"GIT_EXTERNAL_DIFF\"));\n \tif (!external_diff_cmd)\n \t\texternal_diff_cmd = external_diff_cmd_cfg;\n \tdone_preparing = 1;\n \treturn external_diff_cmd;\n }\n-- \n2.20.1.windows.1\n\n"},{"id":"366609","messageId":"xmqq1s5iy4qy.fsf@gitster-ct.c.googlers.com","threadId":"50191","inReplyTo":"20190111202608.10576-1-kgybels@infogroep.be","subject":"Re: [PATCH v2] diff: ensure correct lifetime of external_diff_cmd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-12T02:44:05Z","receivedAt":"2019-01-12T02:44:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kim Gybels writes:\n\n> According to getenv(3)'s notes:\n>\n> The implementation of getenv() is not required to be reentrant. The\n> string pointed to by the return value of getenv() may be statically\n> allocated, and can be modified by a subsequent call to getenv(),\n> putenv(3), setenv(3), or unsetenv(3).\n> ...\n> Signed-off-by: Kim Gybels \n> ---\n\nThanks, looking good.\n\nWill queue.\n\n>\n> Uses xstrdup_or_null as suggested by Eric Sunshine.\n>\n> diff.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/diff.c b/diff.c\n> index dc9965e836..5634992bbc 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -487,11 +487,11 @@ static const char *external_diff(void)\n> \tstatic const char *external_diff_cmd = NULL;\n> \tstatic int done_preparing = 0;\n> \n> \tif (done_preparing)\n> \t\treturn external_diff_cmd;\n> -\texternal_diff_cmd = getenv(\"GIT_EXTERNAL_DIFF\");\n> +\texternal_diff_cmd = xstrdup_or_null(getenv(\"GIT_EXTERNAL_DIFF\"));\n> \tif (!external_diff_cmd)\n> \t\texternal_diff_cmd = external_diff_cmd_cfg;\n> \tdone_preparing = 1;\n> \treturn external_diff_cmd;\n> }\n"}]}