Re: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS

From: Johannes Sixt <j.sixt@viscovery.net>
Date: Feb 26, 2010, 07:34 UTC
Message-ID: <4B87797D.7030905@viscovery.net>
In-Reply-To: <1267143154-5020-1-git-send-email-lznuaa@gmail.com>

Frank Li schrieb:

>  connect.c   |   40 ++++++++++++++++++++++++++++++++++++++++
>  http.c      |    4 ++--
>  imap-send.c |    2 +-

I don't see any header file changes. Don't you get warnings about an undeclared function git_getpass() at the call sites?

> +char *git_getpass(char *prompt)

char *git_getpass(const char *prompt)

> +	askpass = getenv("GIT_ASKPASS");
> +	if (askpass && strlen(askpass) != 0) {
> +		args[0] = getenv("GIT_ASKPASS");

	if (askpass && *askpass) {
		args[0] = askpass;

BTW, to save a level of indentation, you could handle the "trivial" case early like this:

	if (!askpass || !*askpass)
		return get_pass(prompt);

and continue without an 'else' branch.

Show 8 quoted lines

> +		args[1]	= prompt;
> +		args[2] = NULL;
> +
> +		memset(&pass, 0, sizeof(pass));
> +		pass.argv = args;
> +		pass.out = -1;
> +		pass.no_stdin = 1;
> +		pass.no_stderr = 1;

Is it such a good idea to redirect stdin and stderr to /dev/null? What if my password prompt program depends on them? I think it should not matter for your use-case, where a GUI is invoked, to just inherit all channels.

OTOH, it may be worthwhile to set

		pass.use_shell = 1;

to allow commands that are not just a single plain word. But perhaps this has security implications - I don't know.

> +
> +		if (start_command(&pass)) {
> +			error("could not run %s\n", askpass);
> +			return getpass(prompt);

I don't think this is a good idea. The user instructed to use GIT_ASKPASS, and you fall back to asking a password from the terminal. I think the most sensible thing to do here is to 'exit(1)' (start_command has already printed an error message that included the command), because there are callers that do not expect NULL.

Show 10 quoted lines

> +		}
> +
> +		strbuf_read(&buffer, pass.out, 20);
> +		close(pass.out);
> +		for (i = 0; i < buffer.len; i++)
> +			if (buffer.buf[i] == '\n' || buffer.buf[i] == '\r') {
> +				buffer.buf[i] = '\0';
> +				buffer.len = i;
> +		}
> +		return strbuf_detach(&buffer, NULL);

You don't call finish_command() anywhere. Call it after the close() call.

> +
> +	} else {
> +		return getpass(prompt);

You handle the return value in different ways. getpass() returns a pointer to a static buffer, but in the 'then' branch you return an allocated buffer. Not that it matters a lot, though. You could add a comment that you are aware that the memory is leaked.

> +	}
> +	return NULL;

What is this good for?

-- Hannes