Re: [PATCH] Avoid crippled getpass function on Solaris

Jeff King [off-list ref] 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, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

On Tue, Aug 07, 2012 at 01:05:45AM +0200, Andreas Schwab wrote:

quoted

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

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);
 	}

On Tue, Aug 7, 2012 at 8:35 AM, Jeff King [off-list ref] wrote:

This works. Ben, does this work for you too?

-- 
Cheers,
Ray Chuan

Excerpts from Jeff King's message of Mon Aug 06 20:35:41 -0400 2012:

Acked-by: Ben Walton <redacted>

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

Now that git_terminal_prompt can cleanly interact with /dev/tty on
Solaris, enable HAVE_DEV_TTY so that this code path is used for
credential reading instead of relying on the crippled getpass().

Signed-off-by: Ben Walton <redacted>
---

This is a follow up to Jeff's patch that fixes git_terminal_prompt on
Solaris.  I don't have 5.6 or 5.7 for testing but I believe this
should be valid for both of those releases as well.


 Makefile |    1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 15d1319..6b0c961 100644
--- a/Makefile
+++ b/Makefile

@@ -1014,6 +1014,7 @@ ifeq ($(uname_S),SunOS)
 	NO_REGEX = YesPlease
 	NO_FNMATCH_CASEFOLD = YesPlease
 	NO_MSGFMT_EXTENDED_OPTIONS = YesPlease
+	HAVE_DEV_TTY = YesPlease
 	ifeq ($(uname_R),5.6)
 		SOCKLEN_T = int
 		NO_HSTRERROR = YesPlease

-- 
1.7.10.3