{"thread":{"id":"56806","subject":"[Bug] wrapper.c uses unportable unsetenv","startedAt":"2021-10-29T20:14:27Z","lastAt":"2021-10-29T21:57:58Z","messageCount":7,"participants":["rsbecker@nexbridge.com","Junio C Hamano","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"440057","messageId":"012301d7cd01$90428960$b0c79c20$@nexbridge.com","threadId":"56806","inReplyTo":null,"subject":"[Bug] wrapper.c uses unportable unsetenv","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-10-29T20:14:17Z","receivedAt":"2021-10-29T20:14:27Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"The unsetenv()/setenv(overwrite) calls are not 100% portable - as in not on\nall POSIX implementations. It breaks the build on some of the NonStop\nplatforms. This will change in a year or two but I really don't want to fall\nbehind on git releases.\n\nThis was introduced at 3540c71 but I was on vacation when it happened so did\nnot catch it during reviews - my apologies for that.\n\nIs it critical that this be called or can we #ifdef it away if it isn't\nsupported for a build? The #if is exactly this:\n\nwrapper.c@150\n+ #if (_TANDEM_ARCH_ > 3 || (_TANDEM_ARCH_ == 3 && __L_Series_RVU >= 2010))\n\tif (setenv(name, value, overwrite))\n\t\tdie_errno(_(\"could not setenv '%s'\"), name ? name :\n\"(null)\");\n+ #endif\n\nwrapper.c@154\n+ #if (_TANDEM_ARCH_ > 3 || (_TANDEM_ARCH_ == 3 && __L_Series_RVU >= 2010))\n\tif (!unsetenv(name))\n\t\tdie_errno(_(\"could not unsetenv '%s'\"), name ? name :\n\"(null)\");\n+ #endif\n\n-Randall\n\n"},{"id":"440061","messageId":"xmqqsfwjk1s3.fsf@gitster.g","threadId":"56806","inReplyTo":"012301d7cd01$90428960$b0c79c20$@nexbridge.com","subject":"Re: [Bug] wrapper.c uses unportable unsetenv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T20:35:08Z","receivedAt":"2021-10-29T20:35:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":" writes:\n\n> The unsetenv()/setenv(overwrite) calls are not 100% portable - as in not on\n> all POSIX implementations. It breaks the build on some of the NonStop\n> platforms. This will change in a year or two but I really don't want to fall\n> behind on git releases.\n>\n> This was introduced at 3540c71 but I was on vacation when it happened so did\n> not catch it during reviews - my apologies for that.\n\nI do not quite understand. xsetenv() does use the three-arg\nsetenv(), but that is not anything new. It merely replaced a call\nto the same three-arg setenv() in environment.c that have already\nbeen there, introduced by d7ac12b2 (Add set_git_dir() function,\n2007-08-01).\n\nYou may argue that 3540c71 has done a shoddy job by introducing\nxunsetenv() without adding any caller, and to this day, we do not\nhave a single caller to the wrapper, but we already have a few calls\nto unsetenv() that is compiled unconditionally.\n\nSo if you built any version of Git, you must have had these somehow\n\"available\" in your build (e.g. your system headers may have made\nthem a no-op), and I'd expect you'd keep doing the same to locally\nwork it around on the platform side, without ...\n\n> Is it critical that this be called or can we #ifdef it away if it isn't\n> supported for a build? The #if is exactly this:\n\n... doing something like this in the generic part of the code.\nPlease don't do this.\n\n> wrapper.c@150\n> + #if (_TANDEM_ARCH_ > 3 || (_TANDEM_ARCH_ == 3 && __L_Series_RVU >= 2010))\n> \tif (setenv(name, value, overwrite))\n> \t\tdie_errno(_(\"could not setenv '%s'\"), name ? name :\n> \"(null)\");\n> + #endif\n>\n> wrapper.c@154\n> + #if (_TANDEM_ARCH_ > 3 || (_TANDEM_ARCH_ == 3 && __L_Series_RVU >= 2010))\n> \tif (!unsetenv(name))\n> \t\tdie_errno(_(\"could not unsetenv '%s'\"), name ? name :\n> \"(null)\");\n> + #endif\n\n\nThere are compat/setenv.c and compat/unsetenv.c to be used when\nNO_SETENV and NO_UNSETENV are defined. Is that how you built your\nGit earlier since 2007, perhaps?\n\n\n"},{"id":"440065","messageId":"013501d7cd06$8c8281e0$a58785a0$@nexbridge.com","threadId":"56806","inReplyTo":"xmqqsfwjk1s3.fsf@gitster.g","subject":"RE: [Bug] wrapper.c uses unportable unsetenv","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-10-29T20:49:58Z","receivedAt":"2021-10-29T20:50:10Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On October 29, 2021 4:35 PM, Junio C Hamano:\n> writes:\n> \n> > The unsetenv()/setenv(overwrite) calls are not 100% portable - as in\n> > not on all POSIX implementations. It breaks the build on some of the\n> > NonStop platforms. This will change in a year or two but I really\n> > don't want to fall behind on git releases.\n> >\n> > This was introduced at 3540c71 but I was on vacation when it happened\n> > so did not catch it during reviews - my apologies for that.\n> \n> I do not quite understand. xsetenv() does use the three-arg setenv(), but\nthat is\n> not anything new. It merely replaced a call to the same three-arg\nsetenv() in\n> environment.c that have already been there, introduced by d7ac12b2 (Add\n> set_git_dir() function, 2007-08-01).\n> \n> You may argue that 3540c71 has done a shoddy job by introducing\n> xunsetenv() without adding any caller, and to this day, we do not have a\nsingle\n> caller to the wrapper, but we already have a few calls to unsetenv() that\nis\n> compiled unconditionally.\n> \n> So if you built any version of Git, you must have had these somehow\n\"available\"\n> in your build (e.g. your system headers may have made them a no-op), and\nI'd\n> expect you'd keep doing the same to locally work it around on the platform\nside,\n> without ...\n> \n> > Is it critical that this be called or can we #ifdef it away if it\n> > isn't supported for a build? The #if is exactly this:\n> \n> ... doing something like this in the generic part of the code.\n> Please don't do this.\n> \n> > wrapper.c@150\n> > + #if (_TANDEM_ARCH_ > 3 || (_TANDEM_ARCH_ == 3 && __L_Series_RVU >=\n> > + 2010))\n> > \tif (setenv(name, value, overwrite))\n> > \t\tdie_errno(_(\"could not setenv '%s'\"), name ? name :\n> > \"(null)\");\n> > + #endif\n> >\n> > wrapper.c@154\n> > + #if (_TANDEM_ARCH_ > 3 || (_TANDEM_ARCH_ == 3 && __L_Series_RVU >=\n> > + 2010))\n> > \tif (!unsetenv(name))\n> > \t\tdie_errno(_(\"could not unsetenv '%s'\"), name ? name :\n> > \"(null)\");\n> > + #endif\n> \n> \n> There are compat/setenv.c and compat/unsetenv.c to be used when\n> NO_SETENV and NO_UNSETENV are defined. Is that how you built your Git\n> earlier since 2007, perhaps?\n\nWe have defined NO_SETENV and NO_UNSETENV since I have been maintaining it,\nso I don't get how we are getting into this situation. I was planning on\nremoving NO_SETENV when the OS caught up for that on our minimum support\nbuilds next year. NO_UNSETENV needs to stick around for bit longer. The\nsetenv() code is actually fine. It is unsetenv() that is causing problems.\nShould not git-compat-util.h be included in wrapper.c so that we reference\ngitunsetenv?\n\n\n\n"},{"id":"440071","messageId":"013a01d7cd09$72d91cb0$588b5610$@nexbridge.com","threadId":"56806","inReplyTo":"013501d7cd06$8c8281e0$a58785a0$@nexbridge.com","subject":"RE: [Bug] wrapper.c uses unportable unsetenv","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-10-29T21:10:44Z","receivedAt":"2021-10-29T21:11:04Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On October 29, 2021 4:50 PM, I wrote:\n> To: 'Junio C Hamano' \n> Cc: git@vger.kernel.org\n> Subject: RE: [Bug] wrapper.c uses unportable unsetenv\n> \n> On October 29, 2021 4:35 PM, Junio C Hamano:\n> > writes:\n> >\n\n> > There are compat/setenv.c and compat/unsetenv.c to be used when\n> > NO_SETENV and NO_UNSETENV are defined. Is that how you built your Git\n> > earlier since 2007, perhaps?\n> \n> We have defined NO_SETENV and NO_UNSETENV since I have been maintaining\n> it, so I don't get how we are getting into this situation. I was planning\non\n> removing NO_SETENV when the OS caught up for that on our minimum support\n> builds next year. NO_UNSETENV needs to stick around for bit longer. The\n> setenv() code is actually fine. It is unsetenv() that is causing problems.\n> Should not git-compat-util.h be included in wrapper.c so that we reference\n> gitunsetenv?\n\nThe actual issue is this:\n\n if (!unsetenv(name))\n ^\n\"/home/ituglib/randall/git/wrapper.c\", line 156: error(134): expression\nmust have arithmetic or pointer type\n\nThis is with NO_UNSETENV = YesPlease. It makes no sense to me why this isn't\ncompiling.\n\n"},{"id":"440085","messageId":"xmqqwnlvik3v.fsf@gitster.g","threadId":"56806","inReplyTo":"013a01d7cd09$72d91cb0$588b5610$@nexbridge.com","subject":"Re: [Bug] wrapper.c uses unportable unsetenv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T21:42:12Z","receivedAt":"2021-10-29T21:42:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":" writes:\n\n> The actual issue is this:\n>\n> if (!unsetenv(name))\n> ^\n> \"/home/ituglib/randall/git/wrapper.c\", line 156: error(134): expression\n> must have arithmetic or pointer type\n>\n> This is with NO_UNSETENV = YesPlease. It makes no sense to me why this isn't\n> compiling.\n\nAh, compat/unsetenv.c is broken. gitunsetenv() should return int,\nnot void.\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: unsetenv(3) returns int, not void\n\nThis compatilibity implementation has been returning a wrong type,\never since 731043fd (Add compat/unsetenv.c ., 2006-01-25) added to\nthe system, yet nobody noticed it in the past 16 years, presumably\nbecause no code checks failures in their unsetenv() calls. Sigh.\n\nFor now, make it always succeed.\n\nSigned-off-by: Junio C Hamano \n---\n compat/unsetenv.c | 2 +-\n git-compat-util.h | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git c/compat/unsetenv.c w/compat/unsetenv.c\nindex bf5fd7063b..46d49c2c5e 100644\n--- c/compat/unsetenv.c\n+++ w/compat/unsetenv.c\n@@ -1,6 +1,6 @@\n #include \"../git-compat-util.h\"\n \n-void gitunsetenv (const char *name)\n+int gitunsetenv(const char *name)\n {\n #if !defined(__MINGW32__)\n extern char **environ;\ndiff --git c/git-compat-util.h w/git-compat-util.h\nindex b46605300a..0f7e369a5d 100644\n--- c/git-compat-util.h\n+++ w/git-compat-util.h\n@@ -726,7 +726,7 @@ char *gitmkdtemp(char *);\n \n #ifdef NO_UNSETENV\n #define unsetenv gitunsetenv\n-void gitunsetenv(const char *);\n+int gitunsetenv(const char *);\n #endif\n \n #ifdef NO_STRCASESTR\n\n\n"},{"id":"440091","messageId":"nycvar.QRO.7.76.6.2110292348530.56@tvgsbejvaqbjf.bet","threadId":"56806","inReplyTo":"xmqqwnlvik3v.fsf@gitster.g","subject":"Re: [Bug] wrapper.c uses unportable unsetenv","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmail.com","sentAt":"2021-10-29T21:50:38Z","receivedAt":"2021-10-29T21:50:46Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmail.com","avatar":null},"body":"Hi Junio,\n\nOn Fri, 29 Oct 2021, Junio C Hamano wrote:\n\n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\n> Subject: unsetenv(3) returns int, not void\n> \n> This compatilibity implementation has been returning a wrong type,\n> ever since 731043fd (Add compat/unsetenv.c ., 2006-01-25) added to\n> the system, yet nobody noticed it in the past 16 years, presumably\n> because no code checks failures in their unsetenv() calls. Sigh.\n> \n> For now, make it always succeed.\n\nMakes sense, but...\n\n> Signed-off-by: Junio C Hamano \n> ---\n> compat/unsetenv.c | 2 +-\n> git-compat-util.h | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git c/compat/unsetenv.c w/compat/unsetenv.c\n> index bf5fd7063b..46d49c2c5e 100644\n> --- c/compat/unsetenv.c\n> +++ w/compat/unsetenv.c\n> @@ -1,6 +1,6 @@\n> #include \"../git-compat-util.h\"\n> \n> -void gitunsetenv (const char *name)\n> +int gitunsetenv(const char *name)\n> {\n> #if !defined(__MINGW32__)\n> extern char **environ;\n\n... but we are missing this:\n\n@@ -24,4 +24,5 @@ void gitunsetenv (const char *name)\n \t ++dst;\n }\n environ[dst] = NULL;\n+ return 0;\n }\n\nThanks,\nDscho\n\n> diff --git c/git-compat-util.h w/git-compat-util.h\n> index b46605300a..0f7e369a5d 100644\n> --- c/git-compat-util.h\n> +++ w/git-compat-util.h\n> @@ -726,7 +726,7 @@ char *gitmkdtemp(char *);\n> \n> #ifdef NO_UNSETENV\n> #define unsetenv gitunsetenv\n> -void gitunsetenv(const char *);\n> +int gitunsetenv(const char *);\n> #endif\n> \n> #ifdef NO_STRCASESTR\n> \n> \n> \n> \n"},{"id":"440096","messageId":"xmqqfssjijdr.fsf@gitster.g","threadId":"56806","inReplyTo":"nycvar.QRO.7.76.6.2110292348530.56@tvgsbejvaqbjf.bet","subject":"Re: [Bug] wrapper.c uses unportable unsetenv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T21:57:52Z","receivedAt":"2021-10-29T21:57:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> @@ -24,4 +24,5 @@ void gitunsetenv (const char *name)\n> \t ++dst;\n> }\n> environ[dst] = NULL;\n> + return 0;\n> }\n\nYup, that hunk (with a blank before the \"return\") is already in my\n\"real\" version. Thanks for spotting.\n\n"}]}