git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* GIT_ASKPASS absolute path detection bug on Windows
@ 2020-03-21 11:42 András Kucsma
  2020-03-22  7:31 ` Torsten Bögershausen
  0 siblings, 1 reply; 9+ messages in thread
From: András Kucsma @ 2020-03-21 11:42 UTC (permalink / raw)
  To: git

Hi All,

I believe to have found an issue regarding properly executing the
GIT_ASKPASS binary. I'm using Windows Server 2019, with git 2.21.0
installed using cygwin.

## To reproduce:

Assume you have the askpass binary at C:\askpass.bat. In CMD the
following commands reproduce the issue:

C:\> set GIT_ASKPASS=C:\askpass.bat
C:\> git clone https://<private_repository>.git
Cloning into '<private_repository>'...
error: cannot run C:\askpass.bat: No such file or directory
[... proceeds to interactively ask for username and password ...]

On the other hand, if we change the GIT_ASKPASS environment variable
slightly, so that there is a forward slash (/) instead of a backslash
(\), things work as expected:

C:\> set GIT_ASKPASS=C:/askpass.bat
C:\> git clone https://<private_repository>.git
Cloning into '<private_repository>'...
[... success ...]

## Some context:

The source of the problem, is that if git doesn't find a forward slash
anywhere in the path, it assumes it is not a real path and has to look
for the binary using the PATH environment variable. See in
prepare_cmd():
https://github.com/git/git/blob/98cedd0233e/run-command.c#L429-L439

You can see that the "cannot run" error message is printed here, just
after prepare_cmd() returned -1:
https://github.com/git/git/blob/98cedd0233e/run-command.c#L749-L753

I believe this was introduced in late 2018 around git v2.19.2,
although I did not actually bisect the issue:
https://github.com/git/git/commit/321fd823897#diff-7577a5178f8cdc0f719e580577889f04R401-R415


I hope I'm sharing this bug at the right forum. Please direct me to
the proper place if not.

Thank you,
Andras

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: GIT_ASKPASS absolute path detection bug on Windows
  2020-03-21 11:42 GIT_ASKPASS absolute path detection bug on Windows András Kucsma
@ 2020-03-22  7:31 ` Torsten Bögershausen
  2020-03-22 11:44   ` András Kucsma
  0 siblings, 1 reply; 9+ messages in thread
From: Torsten Bögershausen @ 2020-03-22  7:31 UTC (permalink / raw)
  To: András Kucsma; +Cc: git

On Sat, Mar 21, 2020 at 12:42:50PM +0100, András Kucsma wrote:
> Hi All,
>
> I believe to have found an issue regarding properly executing the
> GIT_ASKPASS binary. I'm using Windows Server 2019, with git 2.21.0
> installed using cygwin.
>
> ## To reproduce:
>
> Assume you have the askpass binary at C:\askpass.bat. In CMD the
> following commands reproduce the issue:
>
> C:\> set GIT_ASKPASS=C:\askpass.bat
> C:\> git clone https://<private_repository>.git
> Cloning into '<private_repository>'...
> error: cannot run C:\askpass.bat: No such file or directory
> [... proceeds to interactively ask for username and password ...]
>
> On the other hand, if we change the GIT_ASKPASS environment variable
> slightly, so that there is a forward slash (/) instead of a backslash
> (\), things work as expected:
>
> C:\> set GIT_ASKPASS=C:/askpass.bat
> C:\> git clone https://<private_repository>.git
> Cloning into '<private_repository>'...
> [... success ...]
>
> ## Some context:
>
> The source of the problem, is that if git doesn't find a forward slash
> anywhere in the path, it assumes it is not a real path and has to look
> for the binary using the PATH environment variable. See in
> prepare_cmd():
> https://github.com/git/git/blob/98cedd0233e/run-command.c#L429-L439
>
> You can see that the "cannot run" error message is printed here, just
> after prepare_cmd() returned -1:
> https://github.com/git/git/blob/98cedd0233e/run-command.c#L749-L753
>
> I believe this was introduced in late 2018 around git v2.19.2,
> although I did not actually bisect the issue:
> https://github.com/git/git/commit/321fd823897#diff-7577a5178f8cdc0f719e580577889f04R401-R415
>
>
> I hope I'm sharing this bug at the right forum. Please direct me to
> the proper place if not.

Yes, you came to the rigth place.
Thanks for the report and the detailed analysis.

A quick fix, and a begin of a patch, could be to use
has_dos_drive_prefix() which will look for C: and will therefore even work
with C:\

	/*
	 * If there are no '/' characters in the command then perform a path
	 * lookup and use the resolved path as the command to exec.  If there
	 * are '/' characters, we have exec attempt to invoke the command
	 * directly.
	 */
	if ((!strchr(out->argv[1], '/')) ||
	    (has_dos_drive_prefix(out->argv[1]))) {
		char *program = locate_in_PATH(out->argv[1]);
[]

If you want to play around with the code a little bit, and send us a "git diff",
we can convert that into a patch.

Wellcome to the Git community.

>
> Thank you,
> Andras

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: GIT_ASKPASS absolute path detection bug on Windows
  2020-03-22  7:31 ` Torsten Bögershausen
@ 2020-03-22 11:44   ` András Kucsma
  2020-03-22 16:59     ` brian m. carlson
  2020-03-23 16:58     ` Torsten Bögershausen
  0 siblings, 2 replies; 9+ messages in thread
From: András Kucsma @ 2020-03-22 11:44 UTC (permalink / raw)
  To: Torsten Bögershausen; +Cc: git

On Sun, Mar 22, 2020 at 8:31 AM Torsten Bögershausen <tboegi@web.de> wrote:
>
> On Sat, Mar 21, 2020 at 12:42:50PM +0100, András Kucsma wrote:
> > Hi All,
> >
> > I believe to have found an issue regarding properly executing the
> > GIT_ASKPASS binary. I'm using Windows Server 2019, with git 2.21.0
> > installed using cygwin.
> >
> > ## To reproduce:
> >
> > Assume you have the askpass binary at C:\askpass.bat. In CMD the
> > following commands reproduce the issue:
> >
> > C:\> set GIT_ASKPASS=C:\askpass.bat
> > C:\> git clone https://<private_repository>.git
> > Cloning into '<private_repository>'...
> > error: cannot run C:\askpass.bat: No such file or directory
> > [... proceeds to interactively ask for username and password ...]
> >
> > On the other hand, if we change the GIT_ASKPASS environment variable
> > slightly, so that there is a forward slash (/) instead of a backslash
> > (\), things work as expected:
> >
> > C:\> set GIT_ASKPASS=C:/askpass.bat
> > C:\> git clone https://<private_repository>.git
> > Cloning into '<private_repository>'...
> > [... success ...]
> >
> > ## Some context:
> >
> > The source of the problem, is that if git doesn't find a forward slash
> > anywhere in the path, it assumes it is not a real path and has to look
> > for the binary using the PATH environment variable. See in
> > prepare_cmd():
> > https://github.com/git/git/blob/98cedd0233e/run-command.c#L429-L439
> >
> > You can see that the "cannot run" error message is printed here, just
> > after prepare_cmd() returned -1:
> > https://github.com/git/git/blob/98cedd0233e/run-command.c#L749-L753
> >
> > I believe this was introduced in late 2018 around git v2.19.2,
> > although I did not actually bisect the issue:
> > https://github.com/git/git/commit/321fd823897#diff-7577a5178f8cdc0f719e580577889f04R401-R415
> >
> >
> > I hope I'm sharing this bug at the right forum. Please direct me to
> > the proper place if not.
>
> Yes, you came to the rigth place.
> Thanks for the report and the detailed analysis.
>
> A quick fix, and a begin of a patch, could be to use
> has_dos_drive_prefix() which will look for C: and will therefore even work
> with C:\
>
>         /*
>          * If there are no '/' characters in the command then perform a path
>          * lookup and use the resolved path as the command to exec.  If there
>          * are '/' characters, we have exec attempt to invoke the command
>          * directly.
>          */
>         if ((!strchr(out->argv[1], '/')) ||
>             (has_dos_drive_prefix(out->argv[1]))) {
>                 char *program = locate_in_PATH(out->argv[1]);
> []
>
> If you want to play around with the code a little bit, and send us a "git diff",
> we can convert that into a patch.
>
> Wellcome to the Git community.
>
> >
> > Thank you,
> > Andras

Thanks Torsten!

I believe it is not enough to test only for the drive specifier, as
GIT_ASKPASS has to work with relative paths as well:
C:\SomeDirectory> set GIT_ASKPASS=.\SomeOtherDirectory\askpass.bat
C:\SomeDirectory> git clone https://<some_private_repository>.git

My proposal patch is to take advantage of find_last_dir_sep function's
OS specific directory separator knowledge.
I posted the diff below, which is also available on github here:
https://github.com/git/git/compare/maint...r0mai:fix-prepare_cmd-windows-maint

diff --git a/run-command.c b/run-command.c
index f5e1149f9b..9fcc12ebf9 100644
--- a/run-command.c
+++ b/run-command.c
@@ -421,12 +421,12 @@ static int prepare_cmd(struct argv_array *out,
const struct child_process *cmd)
     }

     /*
-     * If there are no '/' characters in the command then perform a path
-     * lookup and use the resolved path as the command to exec.  If there
-     * are '/' characters, we have exec attempt to invoke the command
-     * directly.
+     * If there are no dir separator characters in the command then perform
+     * a path lookup and use the resolved path as the command to exec. If
+     * there are dir separator characters, we have exec attempt to invoke
+     * the command directly.
      */
-    if (!strchr(out->argv[1], '/')) {
+    if (find_last_dir_sep(out->argv[1]) == NULL) {
         char *program = locate_in_PATH(out->argv[1]);
         if (program) {
             free((char *)out->argv[1]);

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: GIT_ASKPASS absolute path detection bug on Windows
  2020-03-22 11:44   ` András Kucsma
@ 2020-03-22 16:59     ` brian m. carlson
  2020-03-22 18:07       ` Torsten Bögershausen
  2020-03-23 16:58     ` Torsten Bögershausen
  1 sibling, 1 reply; 9+ messages in thread
From: brian m. carlson @ 2020-03-22 16:59 UTC (permalink / raw)
  To: András Kucsma; +Cc: Torsten Bögershausen, git

[-- Attachment #1: Type: text/plain, Size: 2025 bytes --]

On 2020-03-22 at 11:44:33, András Kucsma wrote:
> My proposal patch is to take advantage of find_last_dir_sep function's
> OS specific directory separator knowledge.
> I posted the diff below, which is also available on github here:
> https://github.com/git/git/compare/maint...r0mai:fix-prepare_cmd-windows-maint
> 
> diff --git a/run-command.c b/run-command.c
> index f5e1149f9b..9fcc12ebf9 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -421,12 +421,12 @@ static int prepare_cmd(struct argv_array *out,
> const struct child_process *cmd)
>      }
> 
>      /*
> -     * If there are no '/' characters in the command then perform a path
> -     * lookup and use the resolved path as the command to exec.  If there
> -     * are '/' characters, we have exec attempt to invoke the command
> -     * directly.
> +     * If there are no dir separator characters in the command then perform
> +     * a path lookup and use the resolved path as the command to exec. If
> +     * there are dir separator characters, we have exec attempt to invoke
> +     * the command directly.
>       */
> -    if (!strchr(out->argv[1], '/')) {
> +    if (find_last_dir_sep(out->argv[1]) == NULL) {
>          char *program = locate_in_PATH(out->argv[1]);

This function (locate_in_PATH) specifically says it is not to be used on
Windows because it doesn't work properly there due to file extensions.
I'm pretty sure a proper solution would involve touching that as well,
although your solution does indeed fix the issue you reported.  That
function also uses a colon-separated PATH, which I'm not sure will work
in all cases on Windows (although maybe it will).

From looking at this earlier, I think the problem here is that we're
trying to use the Unix codepaths (on Cygwin) and then expecting those to
handle Windows-style paths, which they aren't intended to do.  This is
likely one of many problems on Cygwin.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: GIT_ASKPASS absolute path detection bug on Windows
  2020-03-22 16:59     ` brian m. carlson
@ 2020-03-22 18:07       ` Torsten Bögershausen
  2020-03-22 18:33         ` András Kucsma
  2020-03-22 18:59         ` Achim Gratz
  0 siblings, 2 replies; 9+ messages in thread
From: Torsten Bögershausen @ 2020-03-22 18:07 UTC (permalink / raw)
  To: brian m. carlson, András Kucsma, git

On Sun, Mar 22, 2020 at 04:59:15PM +0000, brian m. carlson wrote:
> On 2020-03-22 at 11:44:33, András Kucsma wrote:
> > My proposal patch is to take advantage of find_last_dir_sep function's
> > OS specific directory separator knowledge.
> > I posted the diff below, which is also available on github here:
> > https://github.com/git/git/compare/maint...r0mai:fix-prepare_cmd-windows-maint
> >
> > diff --git a/run-command.c b/run-command.c
> > index f5e1149f9b..9fcc12ebf9 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -421,12 +421,12 @@ static int prepare_cmd(struct argv_array *out,
> > const struct child_process *cmd)
> >      }
> >
> >      /*
> > -     * If there are no '/' characters in the command then perform a path
> > -     * lookup and use the resolved path as the command to exec.  If there
> > -     * are '/' characters, we have exec attempt to invoke the command
> > -     * directly.
> > +     * If there are no dir separator characters in the command then perform
> > +     * a path lookup and use the resolved path as the command to exec. If
> > +     * there are dir separator characters, we have exec attempt to invoke
> > +     * the command directly.
> >       */
> > -    if (!strchr(out->argv[1], '/')) {
> > +    if (find_last_dir_sep(out->argv[1]) == NULL) {
> >          char *program = locate_in_PATH(out->argv[1]);
>
> This function (locate_in_PATH) specifically says it is not to be used on
> Windows because it doesn't work properly there due to file extensions.

My reading is, that it dows work if you specify "foo.exe", "foo.bat".
And when you specify "foo" it may use "foo.exe", but there may be
a shell script called "foo".
But I may be wrong.

> I'm pretty sure a proper solution would involve touching that as well,
> although your solution does indeed fix the issue you reported.  That
> function also uses a colon-separated PATH, which I'm not sure will work
> in all cases on Windows (although maybe it will).
>
> From looking at this earlier, I think the problem here is that we're
> trying to use the Unix codepaths (on Cygwin) and then expecting those to
> handle Windows-style paths, which they aren't intended to do.  This is
> likely one of many problems on Cygwin.

Yes and no.
C:/MyTool.BAT is a valid Windows file name even under Windows.
Cygwin preferres /cygdrive/cMyTool.BAT
Git under Cygwin should handle C:/MyTool.BAT correctly, and to my
understanding it does.

Some interesting reading can be found here:
https://docs.microsoft.com/en-us/dotnet/standard/io/file-path-formats#path-normalization

> --
> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: GIT_ASKPASS absolute path detection bug on Windows
  2020-03-22 18:07       ` Torsten Bögershausen
@ 2020-03-22 18:33         ` András Kucsma
  2020-03-22 18:59         ` Achim Gratz
  1 sibling, 0 replies; 9+ messages in thread
From: András Kucsma @ 2020-03-22 18:33 UTC (permalink / raw)
  To: Torsten Bögershausen; +Cc: brian m. carlson, git

On Sun, Mar 22, 2020 at 7:07 PM Torsten Bögershausen <tboegi@web.de> wrote:
> On Sun, Mar 22, 2020 at 04:59:15PM +0000, brian m. carlson wrote:
> > This function (locate_in_PATH) specifically says it is not to be used on
> > Windows because it doesn't work properly there due to file extensions.
>
> My reading is, that it dows work if you specify "foo.exe", "foo.bat".
> And when you specify "foo" it may use "foo.exe", but there may be
> a shell script called "foo".
> But I may be wrong.

Other than the extension appending, I believe the other difference is
that on native Windows the PATH separator is ';' (semi-colon). In
cygwin and mingw, PATH is translated to use ':' (colon).

>
> > I'm pretty sure a proper solution would involve touching that as well,
> > although your solution does indeed fix the issue you reported.  That
> > function also uses a colon-separated PATH, which I'm not sure will work
> > in all cases on Windows (although maybe it will).
> >
> > From looking at this earlier, I think the problem here is that we're
> > trying to use the Unix codepaths (on Cygwin) and then expecting those to
> > handle Windows-style paths, which they aren't intended to do.  This is
> > likely one of many problems on Cygwin.
>
> Yes and no.
> C:/MyTool.BAT is a valid Windows file name even under Windows.
> Cygwin preferres /cygdrive/cMyTool.BAT
> Git under Cygwin should handle C:/MyTool.BAT correctly, and to my
> understanding it does.
>

Indeed, the Cygwin shell tools recognize C:/Users style and even
C:\\Users style paths as well.


Looking at the code a bit more, I feel like git is only supposed to
work within a Unix compatibility layer (like Cygwin or Mingw shell) on
Windows. Beside the PATH and directory separator characters, the code
makes other Unix like assumptions, for example that /bin/sh (a.k.a.
SHELL_PATH), or /dev/null exists. If native Windows support is a goal,
I'm happy to help a bit to improve the situation, but I wouldn't want
to swim against the current. Note that other than this GIT_ASKPASS
related edge case, cygwin built git has been working in this
environment for me.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: GIT_ASKPASS absolute path detection bug on Windows
  2020-03-22 18:07       ` Torsten Bögershausen
  2020-03-22 18:33         ` András Kucsma
@ 2020-03-22 18:59         ` Achim Gratz
  1 sibling, 0 replies; 9+ messages in thread
From: Achim Gratz @ 2020-03-22 18:59 UTC (permalink / raw)
  To: git

Torsten Bögershausen writes:
> C:/MyTool.BAT is a valid Windows file name even under Windows.

Not all Windows programs handle that correctly, though.

> Cygwin preferres /cygdrive/cMyTool.BAT

The /cygdrive prefix can be changed, however.  If you want to be
independent of such vagaries, use /proc/cygdrive instead (use cygpath -U
instead of cygpath -u).

> Git under Cygwin should handle C:/MyTool.BAT correctly, and to my
> understanding it does.

Yes, but there is always the possiblity of some shell script somewhere
missing that boat.  You are best off normalizing to POSIX conventions as
early as possible and keep it that way until you hand off to a Windows
program.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#BlofeldUserWavetables


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: GIT_ASKPASS absolute path detection bug on Windows
  2020-03-22 11:44   ` András Kucsma
  2020-03-22 16:59     ` brian m. carlson
@ 2020-03-23 16:58     ` Torsten Bögershausen
  2020-03-23 18:13       ` András Kucsma
  1 sibling, 1 reply; 9+ messages in thread
From: Torsten Bögershausen @ 2020-03-23 16:58 UTC (permalink / raw)
  To: András Kucsma; +Cc: git

On Sun, Mar 22, 2020 at 12:44:33PM +0100, András Kucsma wrote:
> On Sun, Mar 22, 2020 at 8:31 AM Torsten Bögershausen <tboegi@web.de> wrote:
> >
> > On Sat, Mar 21, 2020 at 12:42:50PM +0100, András Kucsma wrote:
> > > Hi All,
> > >
> > > I believe to have found an issue regarding properly executing the
> > > GIT_ASKPASS binary. I'm using Windows Server 2019, with git 2.21.0
> > > installed using cygwin.
> > >
> > > ## To reproduce:
> > >
> > > Assume you have the askpass binary at C:\askpass.bat. In CMD the
> > > following commands reproduce the issue:
> > >
> > > C:\> set GIT_ASKPASS=C:\askpass.bat
> > > C:\> git clone https://<private_repository>.git
> > > Cloning into '<private_repository>'...
> > > error: cannot run C:\askpass.bat: No such file or directory
> > > [... proceeds to interactively ask for username and password ...]
> > >
> > > On the other hand, if we change the GIT_ASKPASS environment variable
> > > slightly, so that there is a forward slash (/) instead of a backslash
> > > (\), things work as expected:
> > >
> > > C:\> set GIT_ASKPASS=C:/askpass.bat
> > > C:\> git clone https://<private_repository>.git
> > > Cloning into '<private_repository>'...
> > > [... success ...]
> > >
> > > ## Some context:
> > >
> > > The source of the problem, is that if git doesn't find a forward slash
> > > anywhere in the path, it assumes it is not a real path and has to look
> > > for the binary using the PATH environment variable. See in
> > > prepare_cmd():
> > > https://github.com/git/git/blob/98cedd0233e/run-command.c#L429-L439
> > >
> > > You can see that the "cannot run" error message is printed here, just
> > > after prepare_cmd() returned -1:
> > > https://github.com/git/git/blob/98cedd0233e/run-command.c#L749-L753
> > >
> > > I believe this was introduced in late 2018 around git v2.19.2,
> > > although I did not actually bisect the issue:
> > > https://github.com/git/git/commit/321fd823897#diff-7577a5178f8cdc0f719e580577889f04R401-R415
> > >
> > >
> > > I hope I'm sharing this bug at the right forum. Please direct me to
> > > the proper place if not.
> >
> > Yes, you came to the rigth place.
> > Thanks for the report and the detailed analysis.
> >
> > A quick fix, and a begin of a patch, could be to use
> > has_dos_drive_prefix() which will look for C: and will therefore even work
> > with C:\
> >
> >         /*
> >          * If there are no '/' characters in the command then perform a path
> >          * lookup and use the resolved path as the command to exec.  If there
> >          * are '/' characters, we have exec attempt to invoke the command
> >          * directly.
> >          */
> >         if ((!strchr(out->argv[1], '/')) ||
> >             (has_dos_drive_prefix(out->argv[1]))) {
> >                 char *program = locate_in_PATH(out->argv[1]);
> > []
> >
> > If you want to play around with the code a little bit, and send us a "git diff",
> > we can convert that into a patch.
> >
> > Wellcome to the Git community.
> >
> > >
> > > Thank you,
> > > Andras
>
> Thanks Torsten!
>
> I believe it is not enough to test only for the drive specifier, as
> GIT_ASKPASS has to work with relative paths as well:
> C:\SomeDirectory> set GIT_ASKPASS=.\SomeOtherDirectory\askpass.bat
> C:\SomeDirectory> git clone https://<some_private_repository>.git
>
> My proposal patch is to take advantage of find_last_dir_sep function's
> OS specific directory separator knowledge.
> I posted the diff below, which is also available on github here:
> https://github.com/git/git/compare/maint...r0mai:fix-prepare_cmd-windows-maint
>
> diff --git a/run-command.c b/run-command.c
> index f5e1149f9b..9fcc12ebf9 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -421,12 +421,12 @@ static int prepare_cmd(struct argv_array *out,
> const struct child_process *cmd)
>      }
>
>      /*
> -     * If there are no '/' characters in the command then perform a path
> -     * lookup and use the resolved path as the command to exec.  If there
> -     * are '/' characters, we have exec attempt to invoke the command
> -     * directly.
> +     * If there are no dir separator characters in the command then perform
> +     * a path lookup and use the resolved path as the command to exec. If
> +     * there are dir separator characters, we have exec attempt to invoke
> +     * the command directly.
>       */
> -    if (!strchr(out->argv[1], '/')) {
> +    if (find_last_dir_sep(out->argv[1]) == NULL) {
>          char *program = locate_in_PATH(out->argv[1]);
>          if (program) {
>              free((char *)out->argv[1]);

András,
The patch looks good to me.
If you want, you can submit it here to the list,
either with git send-mail

or may be

https://gitgitgadget.github.io/

And please don't forget to sign-off the patch

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: GIT_ASKPASS absolute path detection bug on Windows
  2020-03-23 16:58     ` Torsten Bögershausen
@ 2020-03-23 18:13       ` András Kucsma
  0 siblings, 0 replies; 9+ messages in thread
From: András Kucsma @ 2020-03-23 18:13 UTC (permalink / raw)
  To: Torsten Bögershausen; +Cc: git

Thanks Torsten!
I submitted it as https://github.com/gitgitgadget/git/pull/587
I believe someone has to /allow me to be able to submit it.


On Mon, Mar 23, 2020 at 5:58 PM Torsten Bögershausen <tboegi@web.de> wrote:
>
> On Sun, Mar 22, 2020 at 12:44:33PM +0100, András Kucsma wrote:
> > On Sun, Mar 22, 2020 at 8:31 AM Torsten Bögershausen <tboegi@web.de> wrote:
> > >
> > > On Sat, Mar 21, 2020 at 12:42:50PM +0100, András Kucsma wrote:
> > > > Hi All,
> > > >
> > > > I believe to have found an issue regarding properly executing the
> > > > GIT_ASKPASS binary. I'm using Windows Server 2019, with git 2.21.0
> > > > installed using cygwin.
> > > >
> > > > ## To reproduce:
> > > >
> > > > Assume you have the askpass binary at C:\askpass.bat. In CMD the
> > > > following commands reproduce the issue:
> > > >
> > > > C:\> set GIT_ASKPASS=C:\askpass.bat
> > > > C:\> git clone https://<private_repository>.git
> > > > Cloning into '<private_repository>'...
> > > > error: cannot run C:\askpass.bat: No such file or directory
> > > > [... proceeds to interactively ask for username and password ...]
> > > >
> > > > On the other hand, if we change the GIT_ASKPASS environment variable
> > > > slightly, so that there is a forward slash (/) instead of a backslash
> > > > (\), things work as expected:
> > > >
> > > > C:\> set GIT_ASKPASS=C:/askpass.bat
> > > > C:\> git clone https://<private_repository>.git
> > > > Cloning into '<private_repository>'...
> > > > [... success ...]
> > > >
> > > > ## Some context:
> > > >
> > > > The source of the problem, is that if git doesn't find a forward slash
> > > > anywhere in the path, it assumes it is not a real path and has to look
> > > > for the binary using the PATH environment variable. See in
> > > > prepare_cmd():
> > > > https://github.com/git/git/blob/98cedd0233e/run-command.c#L429-L439
> > > >
> > > > You can see that the "cannot run" error message is printed here, just
> > > > after prepare_cmd() returned -1:
> > > > https://github.com/git/git/blob/98cedd0233e/run-command.c#L749-L753
> > > >
> > > > I believe this was introduced in late 2018 around git v2.19.2,
> > > > although I did not actually bisect the issue:
> > > > https://github.com/git/git/commit/321fd823897#diff-7577a5178f8cdc0f719e580577889f04R401-R415
> > > >
> > > >
> > > > I hope I'm sharing this bug at the right forum. Please direct me to
> > > > the proper place if not.
> > >
> > > Yes, you came to the rigth place.
> > > Thanks for the report and the detailed analysis.
> > >
> > > A quick fix, and a begin of a patch, could be to use
> > > has_dos_drive_prefix() which will look for C: and will therefore even work
> > > with C:\
> > >
> > >         /*
> > >          * If there are no '/' characters in the command then perform a path
> > >          * lookup and use the resolved path as the command to exec.  If there
> > >          * are '/' characters, we have exec attempt to invoke the command
> > >          * directly.
> > >          */
> > >         if ((!strchr(out->argv[1], '/')) ||
> > >             (has_dos_drive_prefix(out->argv[1]))) {
> > >                 char *program = locate_in_PATH(out->argv[1]);
> > > []
> > >
> > > If you want to play around with the code a little bit, and send us a "git diff",
> > > we can convert that into a patch.
> > >
> > > Wellcome to the Git community.
> > >
> > > >
> > > > Thank you,
> > > > Andras
> >
> > Thanks Torsten!
> >
> > I believe it is not enough to test only for the drive specifier, as
> > GIT_ASKPASS has to work with relative paths as well:
> > C:\SomeDirectory> set GIT_ASKPASS=.\SomeOtherDirectory\askpass.bat
> > C:\SomeDirectory> git clone https://<some_private_repository>.git
> >
> > My proposal patch is to take advantage of find_last_dir_sep function's
> > OS specific directory separator knowledge.
> > I posted the diff below, which is also available on github here:
> > https://github.com/git/git/compare/maint...r0mai:fix-prepare_cmd-windows-maint
> >
> > diff --git a/run-command.c b/run-command.c
> > index f5e1149f9b..9fcc12ebf9 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -421,12 +421,12 @@ static int prepare_cmd(struct argv_array *out,
> > const struct child_process *cmd)
> >      }
> >
> >      /*
> > -     * If there are no '/' characters in the command then perform a path
> > -     * lookup and use the resolved path as the command to exec.  If there
> > -     * are '/' characters, we have exec attempt to invoke the command
> > -     * directly.
> > +     * If there are no dir separator characters in the command then perform
> > +     * a path lookup and use the resolved path as the command to exec. If
> > +     * there are dir separator characters, we have exec attempt to invoke
> > +     * the command directly.
> >       */
> > -    if (!strchr(out->argv[1], '/')) {
> > +    if (find_last_dir_sep(out->argv[1]) == NULL) {
> >          char *program = locate_in_PATH(out->argv[1]);
> >          if (program) {
> >              free((char *)out->argv[1]);
>
> András,
> The patch looks good to me.
> If you want, you can submit it here to the list,
> either with git send-mail
>
> or may be
>
> https://gitgitgadget.github.io/
>
> And please don't forget to sign-off the patch

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-03-23 18:13 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-21 11:42 GIT_ASKPASS absolute path detection bug on Windows András Kucsma
2020-03-22  7:31 ` Torsten Bögershausen
2020-03-22 11:44   ` András Kucsma
2020-03-22 16:59     ` brian m. carlson
2020-03-22 18:07       ` Torsten Bögershausen
2020-03-22 18:33         ` András Kucsma
2020-03-22 18:59         ` Achim Gratz
2020-03-23 16:58     ` Torsten Bögershausen
2020-03-23 18:13       ` András Kucsma

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).