Re: [PATCH] Avoid crippled getpass function on Solaris
On Sun, Aug 05, 2012 at 10:35:06PM -0400, Ben Walton wrote: Excerpts from Junio C Hamano's message of Sun Aug 05 21:59:48 -0400 2012: Wouldn't #if solaris #define getpass getpassphrase #endif without anything else be more than sufficient? Yes, it would, but I was hoping to make it more explicit that the function getpass may be substituted with something else. I don't think that's important. Either the thing is a drop-in replica of getpass, or it is not. In the former case, it's OK for it to be transparent that it has been replaced. In the latter case, it should not be a #define replacement at all, but should be its own alternative in compat/terminal.c (just like HAVE_DEV_TTY is). From my reading of getpassphrase, it does seem to be a drop-in replacement. So I'm OK conceptually with the patch if we can't do any better. But getpass still sucks. It doesn't handle echoing, and it may or may not fall back to reading from stdin if the tty isn't available (which is disastrous for remote-curl, whose stdin is speaking the remote-helper protocol to git). So I'd really prefer to make HAVE_DEV_TTY work with Solaris if we can. I'm happy to spend a few cycles on it. I don't have access to any real Solaris boxes these days, but I imagine I can get OpenSolaris running under VirtualBox without too much trouble... -Peff PS If we do go the getpassphrase route, does it make sense to introduce HAVE_GETPASSPHRASE? We usually try to provide one layer of indirection by naming our #defines after features, and then connecting systems to the feature defines via the Makefile. But maybe Solaris is the only system that has getpassphrase. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Avoid crippled getpass function on Solaris
On Mon, Aug 06, 2012 at 05:31:00PM -0400, Ben Walton wrote: I'm happy to spend a few cycles on it. I don't have access to any real Solaris boxes these days, but I imagine I can get OpenSolaris running under VirtualBox without too much trouble... I'm also happy to do this work and have ready access to Solaris 8-11. I'm currently in pkgutil hell trying to remember how to get a working gcc on Solaris. Bleh. What kind of OS doesn't ship with a compiler by default? :) Would it be reasonable to support using getpassphrase() if !HAVE_DEV_TTY in addition to making the HAVE_DEV_TTY code function on Solaris too? That would provide a harm reduction for Solaris users that (for some reason) disabled the nicer interface...but maybe it's too much? If you think there is a reason that somebody on Solaris might not want to use HAVE_DEV_TTY, then I guess it makes sense. But I can't really think of one, so I'd be just as happy to leave it until somebody does. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Avoid crippled getpass function on Solaris
Excerpts from Jeff King's message of Mon Aug 06 17:34:04 -0400 2012: On Mon, Aug 06, 2012 at 05:31:00PM -0400, Ben Walton wrote: I'm happy to spend a few cycles on it. I don't have access to any real Solaris boxes these days, but I imagine I can get OpenSolaris running under VirtualBox without too much trouble... I'm also happy to do this work and have ready access to Solaris 8-11. I'm currently in pkgutil hell trying to remember how to get a working gcc on Solaris. Bleh. What kind of OS doesn't ship with a compiler by default? :) One that's losing mindshare and isn't gaining traction? *g* Feel free to message me offline if you need a hand with that. Would it be reasonable to support using getpassphrase() if !HAVE_DEV_TTY in addition to making the HAVE_DEV_TTY code function on Solaris too? That would provide a harm reduction for Solaris users that (for some reason) disabled the nicer interface...but maybe it's too much? If you think there is a reason that somebody on Solaris might not want to use HAVE_DEV_TTY, then I guess it makes sense. But I can't really think of one, so I'd be just as happy to leave it until somebody does. No, I can't think of a _good_ reason to do that either. Thanks -Ben -- Ben Walton Systems Programmer - CHASS University of Toronto C:416.407.5610 | W:416.978.4302 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Avoid crippled getpass function on Solaris
On Mon, Aug 06, 2012 at 06:09:08PM -0400, Ben Walton wrote:
I'm currently in pkgutil hell trying to remember how to get a working
gcc on Solaris. Bleh. What kind of OS doesn't ship with a compiler by
default? :)
One that's losing mindshare and isn't gaining traction? *g* Feel free
to message me offline if you need a hand with that.
Thanks, I figured it out (I installed opencsw's gcc, but I had no
standard include files. Turns out that I needed the system/header
package from upstream).
The stdio behavior on Solaris is weird. If I run this sample program:
#include stdio.h
int main(void)
{
FILE *fh = fopen(/dev/tty, w+);
char buf[32] = {0};
fgets(buf, sizeof(buf), fh);
fprintf(fh, got %s\n, buf);
return 0;
}
on Linux, I get:
$ ./a.out
foo-- me typing
got foo-- program output
On Solaris, I get:
$ ./a.out
foo-- me typing
foo-- ???
got foo-- program output
If you step it through the debugger, the mystery output comes as part of
the fprintf, as if it is in the buffer waiting to be flushed. And
indeed, if you dump the FILE handle via gdb, there is only a single
buffer, and it contains foo\n. Whereas with glibc, there are separate
read/write buffers.
I suspect the single buffer is enough to handle normal files, but not
bidirectional pipes where the input and output content are unrelated. I
don't think Solaris is _buggy_ per se, as we have probably stepped
outside the realm of what the standard promises, but it's certainly a
quality-of-implementation issue.
So I think it could be solved by opening /dev/tty twice (or fdopen()ing
the same descriptor twice). Or by just doing away with stdio entirely.
Looking over the code, I recall now why I used stdio: strbuf_getline
requires it. These days we have strbuf_getwholeline_fd. It's slightly
less efficient (it has to read() a character at a time), but since we
are talking about a few characters of keyboard input, it should be OK.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Avoid crippled getpass function on Solaris
Excerpts from Jeff King's message of Mon Aug 06 18:31:13 -0400 2012: Looking over the code, I recall now why I used stdio: strbuf_getline requires it. These days we have strbuf_getwholeline_fd. It's slightly less efficient (it has to read() a character at a time), but since we are talking about a few characters of keyboard input, it should be OK. I noticed the same requirement. I'm currently building/testing a patch that switches from FILE - fd and also to strbuf_getwholeline_fd. Personally, I like this approach more than calling an open function multiple times. Thanks -Ben -- Ben Walton Systems Programmer - CHASS University of Toronto C:416.407.5610 | W:416.978.4302 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Avoid crippled getpass function on Solaris
Jeff King p...@peff.net writes:
The stdio behavior on Solaris is weird. If I run this sample program:
#include stdio.h
int main(void)
{
FILE *fh = fopen(/dev/tty, w+);
char buf[32] = {0};
fgets(buf, sizeof(buf), fh);
fprintf(fh, got %s\n, buf);
return 0;
}
on Linux, I get:
$ ./a.out
foo-- me typing
got foo-- program output
On Solaris, I get:
$ ./a.out
foo-- me typing
foo-- ???
got foo-- program output
That's not a bug, you need to flush or seek when you want to switch
between read to write.
Andreas.
--
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
And now for something completely different.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Avoid crippled getpass function on Solaris
Excerpts from Jeff King's message of Mon Aug 06 18:42:22 -0400 2012: +if (strbuf_getwholeline(sb, fd, term)) Shouldn't this be strbuf_getwholeline_fd though? Otherwise, I think this is a good addition. Thanks -Ben -- Ben Walton Systems Programmer - CHASS University of Toronto C:416.407.5610 | W:416.978.4302 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Avoid crippled getpass function on Solaris
On Mon, Aug 06, 2012 at 07:31:30PM -0400, Ben Walton wrote: Excerpts from Jeff King's message of Mon Aug 06 18:42:22 -0400 2012: +if (strbuf_getwholeline(sb, fd, term)) Shouldn't this be strbuf_getwholeline_fd though? Whoops, yes. When I got your email, I had just finished the patch but had not yet written the follow-on patch that would actually exercise it. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Avoid crippled getpass function on Solaris
On Tue, Aug 07, 2012 at 01:05:45AM +0200, Andreas Schwab wrote:
The stdio behavior on Solaris is weird. If I run this sample program:
#include stdio.h
int main(void)
{
FILE *fh = fopen(/dev/tty, w+);
char buf[32] = {0};
fgets(buf, sizeof(buf), fh);
fprintf(fh, got %s\n, buf);
return 0;
}
on Linux, I get:
$ ./a.out
foo-- me typing
got foo-- program output
On Solaris, I get:
$ ./a.out
foo-- me typing
foo-- ???
got foo-- program output
That's not a bug, you need to flush or seek when you want to switch
between read to write.
Thanks. Inserting an fflush() before the fprintf does fix it, but I
think that a flush is disallowed by the standard in this case. From C99,
7.19.5.2 (fflush):
If stream points to an output stream or an update stream in which the
most recent operation was not input, the fflush function causes any
unwritten data for that stream to be delivered to the host environment
to be written to the file; otherwise, the behavior is undefined.
And later, from 7.19.5.3 (fopen):
When a file is opened with update mode ('+' as the second or third
character in the above list of mode argument values), both input and
output may be performed on the associated stream. However, output
shall not be directly followed by input without an intervening call to
the fflush function or to a file positioning function (fseek, fsetpos,
or rewind), and input shall not be directly followed by output without
an intervening call to a file positioning function, unless the input
operation encounters end-of-file.
I don't know if any implementation actually cares in practice, of
course, but probably the sane thing would be to call
fseek(fh, SEEK_CUR, 0) before the fprintf.
This is all moot if we end up ripping stdio out of this code for other
reasons, but it does give us another option for a fix.
Thanks for the pointer.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Avoid crippled getpass function on Solaris
On Mon, Aug 06, 2012 at 08:23:18PM -0400, Jeff King wrote:
This is all moot if we end up ripping stdio out of this code for other
reasons, but it does give us another option for a fix.
And here is what that patch would look like:
-- 8 --
Subject: [PATCH] terminal: seek when switching between reading and writing
When a stdio stream is opened in update mode (e.g., w+),
the C standard forbids switching between reading or writing
without an intervening positioning function. Many
implementations are lenient about this, but Solaris libc
will flush the recently-read contents to the output buffer.
In this instance, that meant writing the non-echoed password
that the user just typed to the terminal.
Fix it by inserting a no-op fseek between the read and
write.
The opposite direction (writing immediately followed by
reading) is also disallowed, but our fflush immediately
after printing the prompt is sufficient to satisfy the
standard.
---
compat/terminal.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/compat/terminal.c b/compat/terminal.c
index 6d16c8f..bbb038d 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -59,6 +59,7 @@ char *git_terminal_prompt(const char *prompt, int echo)
r = strbuf_getline(buf, fh, '\n');
if (!echo) {
+ fseek(fh, SEEK_CUR, 0);
putc('\n', fh);
fflush(fh);
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Avoid crippled getpass function on Solaris
On Tue, Aug 7, 2012 at 8:35 AM, Jeff King p...@peff.net wrote:
Subject: [PATCH] terminal: seek when switching between reading and writing
When a stdio stream is opened in update mode (e.g., w+),
the C standard forbids switching between reading or writing
without an intervening positioning function. Many
implementations are lenient about this, but Solaris libc
will flush the recently-read contents to the output buffer.
In this instance, that meant writing the non-echoed password
that the user just typed to the terminal.
Fix it by inserting a no-op fseek between the read and
write.
The opposite direction (writing immediately followed by
reading) is also disallowed, but our fflush immediately
after printing the prompt is sufficient to satisfy the
standard.
---
compat/terminal.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/compat/terminal.c b/compat/terminal.c
index 6d16c8f..bbb038d 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -59,6 +59,7 @@ char *git_terminal_prompt(const char *prompt, int echo)
r = strbuf_getline(buf, fh, '\n');
if (!echo) {
+ fseek(fh, SEEK_CUR, 0);
putc('\n', fh);
fflush(fh);
}
This works. Ben, does this work for you too?
--
Cheers,
Ray Chuan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Avoid crippled getpass function on Solaris
Excerpts from Jeff King's message of Mon Aug 06 20:35:41 -0400 2012:
---
compat/terminal.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/compat/terminal.c b/compat/terminal.c
index 6d16c8f..bbb038d 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -59,6 +59,7 @@ char *git_terminal_prompt(const char *prompt, int echo)
r = strbuf_getline(buf, fh, '\n');
if (!echo) {
+fseek(fh, SEEK_CUR, 0);
putc('\n', fh);
fflush(fh);
}
Acked-by: Ben Walton bwal...@artsci.utoronto.ca
That looks good to me. I'm able to clone a password protected https
repository and the prompting works as I'd expect.
Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Avoid crippled getpass function on Solaris
On Mon, Aug 6, 2012 at 7:17 AM, Ben Walton bwal...@artsci.utoronto.ca wrote:
I've also briefly dabbled with getting Solaris to simply use the
HAVE_DEV_TTY code path but the terminal echo stuff hasn't worked
nicely for me just yet. (It reads the password with nothing echoed
but then displays the string after reading the newline.) This might
still be a better approach in the future, but for now, having long
password reading capability will still be a benefit to users on this
platform.
Replacing
if (!echo) {
putc('\n', fh);
fflush(fh);
}
with
if (!echo)
write(term_fd, \n, 1);
fixed that. Using fd's instead of FILE* was mentioned at [1]. Perhaps
that is the direction to go in.
[1] http://mid.gmane.org/7vsjc12j5o@alter.siamese.dyndns.org
--
Cheers,
Ray Chuan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Avoid crippled getpass function on Solaris
Ben Walton bwal...@artsci.utoronto.ca writes: diff --git a/compat/terminal.h b/compat/terminal.h index 97db7cd..8d7b3f9 100644 --- a/compat/terminal.h +++ b/compat/terminal.h @@ -3,4 +3,13 @@ char *git_terminal_prompt(const char *prompt, int echo); +/* getpass() returns at most 8 characters on solaris so use + getpassphrase() which returns up to 256. */ +# if defined (__SVR4) defined (__sun) /* solaris */ +#define GETPASS getpassphrase +#else +#define GETPASS getpass +#endif + + #endif /* COMPAT_TERMINAL_H */ Wouldn't #if solaris #define getpass getpassphrase #endif without anything else be more than sufficient? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Avoid crippled getpass function on Solaris
Excerpts from Junio C Hamano's message of Sun Aug 05 21:59:48 -0400 2012: Wouldn't #if solaris #define getpass getpassphrase #endif without anything else be more than sufficient? Yes, it would, but I was hoping to make it more explicit that the function getpass may be substituted with something else. Thanks -Ben -- Ben Walton Systems Programmer - CHASS University of Toronto C:416.407.5610 | W:416.978.4302 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
