[PATCH] detect dup failure

[PATCH] detect dup failure

[Jim Meyering]

Without this, if you ever run out of file descriptors, dup will
fail (silently), fdopen will return NULL, and fprintf will
try to dereference NULL (i.e., usually segfault).

Signed-off-by: Jim Meyering <jim@meyering.net>
---
 builtin-log.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 073a2a1..ca54387 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -588,8 +588,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (ignore_if_in_upstream)
 		get_patch_ids(&rev, &ids, prefix);

-	if (!use_stdout)
-		realstdout = fdopen(dup(1), "w");
+	if (!use_stdout) {
+		int fd = dup(1);
+		if (fd < 0)
+			die("failed to duplicate standard output");
+		realstdout = fdopen(fd, "w");
+	}

 	prepare_revision_walk(&rev);
 	while ((commit = get_revision(&rev)) != NULL) {

Re: [PATCH] detect dup failure

[Alex Riesen]

On 6/27/07, Jim Meyering <jim@meyering.net> wrote:
> Without this, if you ever run out of file descriptors, dup will
> fail (silently), fdopen will return NULL, and fprintf will
> try to dereference NULL (i.e., usually segfault).

But if you check the result of fdopen for NULL instead
you'll cover the dup failure _and_ out-of-memory in one
go. You'll loose the errno (probably), but you don't seem
to use it here anyway.

[PATCH] git-log: detect dup and fdopen failure

[Jim Meyering]

"Alex Riesen" <raa.lkml@gmail.com> wrote:
> On 6/27/07, Jim Meyering <jim@meyering.net> wrote:
>> Without this, if you ever run out of file descriptors, dup will
>> fail (silently), fdopen will return NULL, and fprintf will
>> try to dereference NULL (i.e., usually segfault).
>
> But if you check the result of fdopen for NULL instead
> you'll cover the dup failure _and_ out-of-memory in one
> go. You'll loose the errno (probably), but you don't seem
> to use it here anyway.

Good catch.  Thanks!
I didn't see that fdopen could fail with ENOMEM.
That'll teach me to trust the man page.  I see POSIX does mention it.

Here's a better patch:

Signed-off-by: Jim Meyering <jim@meyering.net>
---
 builtin-log.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 073a2a1..7b0d6f4 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -588,8 +588,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (ignore_if_in_upstream)
 		get_patch_ids(&rev, &ids, prefix);

-	if (!use_stdout)
-		realstdout = fdopen(dup(1), "w");
+	if (!use_stdout) {
+		int fd = dup(1);
+		if (fd < 0 || (realstdout = fdopen(fd, "w")) == NULL)
+			die("failed to duplicate standard output: %s",
+			    strerror(errno));
+	}

 	prepare_revision_walk(&rev);
 	while ((commit = get_revision(&rev)) != NULL) {

Re: [PATCH] git-log: detect dup and fdopen failure

[Alex Riesen]

On 6/27/07, Jim Meyering <jim@meyering.net> wrote:
> I didn't see that fdopen could fail with ENOMEM.
> That'll teach me to trust the man page.  I see POSIX does mention it.

I wouldn't trust Linux man pages nor POSIX, if I were you.
Check if this works in some exotic but common
environments (like MacOSX, Cygwin or HP-UX).

(And yes, they probably are broken, and no, you can't fix them,
and no, people are not going to stop using them).

> +       if (!use_stdout) {
> +               int fd = dup(1);
> +               if (fd < 0 || (realstdout = fdopen(fd, "w")) == NULL)
> +                       die("failed to duplicate standard output: %s",
> +                           strerror(errno));
> +       }

Kinda stuffed in here. What's wrong with plain

  realstdout = fdopen(dup(1), "w");
  if (!realstdout)
    die("%s", strerror(errno));

(Yes, I do think that "duplicate standard output" is useless,
except for debugging. Exactly as strerror is, but that is shorter).

Re: [PATCH] git-log: detect dup and fdopen failure

[Jim Meyering]

"Alex Riesen" <raa.lkml@gmail.com> wrote:

> On 6/27/07, Jim Meyering <jim@meyering.net> wrote:
>> I didn't see that fdopen could fail with ENOMEM.
>> That'll teach me to trust the man page.  I see POSIX does mention it.

s/trust/use/ :)

> I wouldn't trust Linux man pages nor POSIX, if I were you.

Ha.  Normally I don't rely solely on the man pages.

> Check if this works in some exotic but common
> environments (like MacOSX, Cygwin or HP-UX).

What do you mean?
I know that dup and fdopen work in those environments.

> (And yes, they probably are broken, and no, you can't fix them,
> and no, people are not going to stop using them).
>
>> +       if (!use_stdout) {
>> +               int fd = dup(1);
>> +               if (fd < 0 || (realstdout = fdopen(fd, "w")) == NULL)
>> +                       die("failed to duplicate standard output: %s",
>> +                           strerror(errno));
>> +       }
>
> Kinda stuffed in here. What's wrong with plain

It's ok, but for the fact that when dup fails, all you get
is the uninformative EINVAL from fdopen.

>  realstdout = fdopen(dup(1), "w");
>  if (!realstdout)
>    die("%s", strerror(errno));
>
> (Yes, I do think that "duplicate standard output" is useless,
> except for debugging. Exactly as strerror is, but that is shorter).

I like to include *something* in the diagnostic so that when someone
sees it and reports it developers have an easier time finding where it
comes from in the code.  Especially with something as unlikely (and hard
to reproduce) as this, that might be useful.

Re: [PATCH] git-log: detect dup and fdopen failure

[Geert Bosch]

On Jun 27, 2007, at 09:02, Jim Meyering wrote:
> -	if (!use_stdout)
> -		realstdout = fdopen(dup(1), "w");
> +	if (!use_stdout) {
> +		int fd = dup(1);
> +		if (fd < 0 || (realstdout = fdopen(fd, "w")) == NULL)
> +			die("failed to duplicate standard output: %s",
> +			    strerror(errno));
> +	}

This makes the code unreadable! A great way to ruin
perfectly fine code is to add tons of error checking.
The error checking is likely wrong (detects non-errors,
or fails to detect real ones), and for sure makes code
untestable  and unreadable.

If we really case about catching such errors, write
the code as:
	if (!use_stdout)
		realstdout = xfdopen(dup(1), "w");
where xfdopen is a wrapper around fdopen that dies in
case of an error. This follows a practice we use elsewhere,
and only adds one character to the code and only affects
readability very slightly.

> Without this, if you ever run out of file descriptors, dup will
> fail (silently), fdopen will return NULL, and fprintf will
> try to dereference NULL (i.e., usually segfault).

As it is unlikely the failure mode will ever occur in practice,
any way of aborting is fine. Even SIGSEGV would do: it would be
trivial to find that we were leaking file descriptors or are out
of memory. Oh, wait, that means we don't need any checking code
at all...

   -Geert

Re: [PATCH] git-log: detect dup and fdopen failure

[Alex Riesen]

On 6/27/07, Jim Meyering <jim@meyering.net> wrote:
> "Alex Riesen" <raa.lkml@gmail.com> wrote:
> > Check if this works in some exotic but common
> > environments (like MacOSX, Cygwin or HP-UX).
>
> What do you mean?
> I know that dup and fdopen work in those environments.

Exactly as you described? Are you sure fdopen on Cygwin
sets ENOMEM?

> >
> > Kinda stuffed in here. What's wrong with plain
>
> It's ok, but for the fact that when dup fails, all you get
> is the uninformative EINVAL from fdopen.

EBADF on Cygwin, 0 on Mingw32. Can't even imagine what msvc
(which actually isn't a developers tool, but very common) will return.

> > (Yes, I do think that "duplicate standard output" is useless,
> > except for debugging. Exactly as strerror is, but that is shorter).
>
> I like to include *something* in the diagnostic so that when someone
> sees it and reports it developers have an easier time finding where it
> comes from in the code.  Especially with something as unlikely (and hard
> to reproduce) as this, that might be useful.

Then put file name and the line number in!

Re: [PATCH] git-log: detect dup and fdopen failure

[Jim Meyering]

Geert Bosch <bosch@adacore.com> wrote:

> On Jun 27, 2007, at 09:02, Jim Meyering wrote:
>> -	if (!use_stdout)
>> -		realstdout = fdopen(dup(1), "w");
>> +	if (!use_stdout) {
>> +		int fd = dup(1);
>> +		if (fd < 0 || (realstdout = fdopen(fd, "w")) == NULL)
>> +			die("failed to duplicate standard output: %s",
>> +			    strerror(errno));
>> +	}
>
> This makes the code unreadable! A great way to ruin
> perfectly fine code is to add tons of error checking.
> The error checking is likely wrong (detects non-errors,
> or fails to detect real ones), and for sure makes code
> untestable  and unreadable.
>
> If we really case about catching such errors, write
> the code as:
> 	if (!use_stdout)
> 		realstdout = xfdopen(dup(1), "w");

Of course.  That is much more readable.
Though, perhaps you meant this?

> 		realstdout = xfdopen(xdup(1), "w");

If so, I'll be happy to prepare a patch to do that instead.
IMHO, we should never ignore syscall errors, and when preparing a
patch for a project like git (to which I haven't contributed much yet),
I prefer to keep the initial patch small and localized.

> where xfdopen is a wrapper around fdopen that dies in
> case of an error. This follows a practice we use elsewhere,
> and only adds one character to the code and only affects
> readability very slightly.
>
>> Without this, if you ever run out of file descriptors, dup will
>> fail (silently), fdopen will return NULL, and fprintf will
>> try to dereference NULL (i.e., usually segfault).
>
> As it is unlikely the failure mode will ever occur in practice,
> any way of aborting is fine. Even SIGSEGV would do: it would be
> trivial to find that we were leaking file descriptors or are out
> of memory. Oh, wait, that means we don't need any checking code
> at all...

Aren't you presuming the problem is easily reproducible
(and encountered by someone capable of investigating/reproducing),
or maybe that the abort left a usable core dump behind?

In my experience, the hardest bugs to track down are those
that are very rare and hard to reproduce.

Re: [PATCH] git-log: detect dup and fdopen failure

[Jim Meyering]

"Alex Riesen" <raa.lkml@gmail.com> wrote:

> On 6/27/07, Jim Meyering <jim@meyering.net> wrote:
>> "Alex Riesen" <raa.lkml@gmail.com> wrote:
>> > Check if this works in some exotic but common
>> > environments (like MacOSX, Cygwin or HP-UX).
>>
>> What do you mean?
>> I know that dup and fdopen work in those environments.
>
> Exactly as you described?

No, but why does that matter?

> Are you sure fdopen on Cygwin
> sets ENOMEM?

Are you suggesting not to use errno because
it won't contain useful information on Mingw32?

>> It's ok, but for the fact that when dup fails, all you get
>> is the uninformative EINVAL from fdopen.
>
> EBADF on Cygwin, 0 on Mingw32. Can't even imagine what msvc

EBADF is fine.  Setting errno to 0 upon error is a bug.
Don't cater to buggy systems.

> (which actually isn't a developers tool, but very common) will return.
>
>> > (Yes, I do think that "duplicate standard output" is useless,
>> > except for debugging. Exactly as strerror is, but that is shorter).
>>
>> I like to include *something* in the diagnostic so that when someone
>> sees it and reports it developers have an easier time finding where it
>> comes from in the code.  Especially with something as unlikely (and hard
>> to reproduce) as this, that might be useful.
>
> Then put file name and the line number in!

Surely you're playing devil's advocate, now...
If you go that route, you might as well use assert() for both conditions,
and we all know *that* is silly.

Re: [PATCH] git-log: detect dup and fdopen failure

[Jim Meyering]

Geert Bosch <bosch@adacore.com> wrote:
> If we really case about catching such errors, write
> the code as:
> 	if (!use_stdout)
> 		realstdout = xfdopen(dup(1), "w");

Thanks for the suggestion.
Here's a patch doing just that (and same for dup/xdup):

Subject: [PATCH] git-log: detect dup and fdopen failure
 git-compat-util.h (xdup, xfdopen): Define functions.


Signed-off-by: Jim Meyering <jim@meyering.net>
---
 builtin-log.c     |    2 +-
 git-compat-util.h |   16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 073a2a1..a4186ea 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -589,7 +589,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		get_patch_ids(&rev, &ids, prefix);

 	if (!use_stdout)
-		realstdout = fdopen(dup(1), "w");
+		realstdout = xfdopen(xdup(1), "w");

 	prepare_revision_walk(&rev);
 	while ((commit = get_revision(&rev)) != NULL) {
diff --git a/git-compat-util.h b/git-compat-util.h
index b2ab3f8..362e040 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -287,6 +287,22 @@ static inline ssize_t xwrite(int fd, const void *buf, size_t len)
 	}
 }

+static inline int xdup(int fd)
+{
+	int ret = dup(fd);
+	if (ret < 0)
+		die("dup failed: %s", strerror(errno));
+	return ret;
+}
+
+static inline FILE *xfdopen(int fd, const char *mode)
+{
+	FILE *stream = fdopen(fd, mode);
+	if (stream == NULL)
+		die("Out of memory? fdopen failed: %s", strerror(errno));
+	return stream;
+}
+
 static inline size_t xsize_t(off_t len)
 {
 	return (size_t)len;

Re: [PATCH] git-log: detect dup and fdopen failure

[Alex Riesen]

On 6/27/07, Jim Meyering <jim@meyering.net> wrote:
> "Alex Riesen" <raa.lkml@gmail.com> wrote:
>
> > On 6/27/07, Jim Meyering <jim@meyering.net> wrote:
> >> "Alex Riesen" <raa.lkml@gmail.com> wrote:
> >> > Check if this works in some exotic but common
> >> > environments (like MacOSX, Cygwin or HP-UX).
> >>
> >> What do you mean?
> >> I know that dup and fdopen work in those environments.
> >
> > Exactly as you described?
>
> No, but why does that matter?
>

Your code will need additional tweaking on this
systems.

> > Are you sure fdopen on Cygwin
> > sets ENOMEM?
>
> Are you suggesting not to use errno because
> it won't contain useful information on Mingw32?

Yes. Mingw32 is just one example.

> >> It's ok, but for the fact that when dup fails, all you get
> >> is the uninformative EINVAL from fdopen.
> >
> > EBADF on Cygwin, 0 on Mingw32. Can't even imagine what msvc
>
> EBADF is fine.  Setting errno to 0 upon error is a bug.
> Don't cater to buggy systems.

It is my system at work. What should I do, throw it out of window?

> >
> > Then put file name and the line number in!
>
> Surely you're playing devil's advocate, now...

No, I'm just thinking aloud about whether I should start preparing
a reverting commit for your changes just to be able to continue
using Git on that mismanaged, stupid, slow and very real
system I have to work with. It is a Win2k laptop, in a Novell
network and with Cygwin on (actually 2 cygwins at least, which
cygwin does not support), and without a chance to _EVER_ use
anything else. And I happen to depend on Git, because the
alternative is, as someone noted, "badly mismanaged" Perforce.
And I believe I am not that bad off, there are other systems on
which you didn't test either. "Silly", as you say.

Re: [PATCH] git-log: detect dup and fdopen failure

[Jim Meyering]

"Alex Riesen" <raa.lkml@gmail.com> wrote:
> On 6/27/07, Jim Meyering <jim@meyering.net> wrote:
>> "Alex Riesen" <raa.lkml@gmail.com> wrote:
>>
>> > On 6/27/07, Jim Meyering <jim@meyering.net> wrote:
>> >> "Alex Riesen" <raa.lkml@gmail.com> wrote:
>> >> > Check if this works in some exotic but common
>> >> > environments (like MacOSX, Cygwin or HP-UX).
>> >>
>> >> What do you mean?
>> >> I know that dup and fdopen work in those environments.
>> >
>> > Exactly as you described?
>>
>> No, but why does that matter?
>
> Your code will need additional tweaking on this
> systems.

Why bother?  The only undesirable side effect you'd see on a buggy
system would be a meaningless strerror(errno) in that very unusual
error situation: when git-format-patch (without --stdout)
runs out of file descriptors or memory.

>> > Are you sure fdopen on Cygwin
>> > sets ENOMEM?
>>
>> Are you suggesting not to use errno because
>> it won't contain useful information on Mingw32?
>
> Yes. Mingw32 is just one example.
>
>> >> It's ok, but for the fact that when dup fails, all you get
>> >> is the uninformative EINVAL from fdopen.
>> >
>> > EBADF on Cygwin, 0 on Mingw32. Can't even imagine what msvc
>>
>> EBADF is fine.  Setting errno to 0 upon error is a bug.
>> Don't cater to buggy systems.
>
> It is my system at work. What should I do, throw it out of window?

Perhaps you misunderstood?

My saying "don't cater to buggy systems", means don't pollute or dumb-down
application code to accommodate them.  Instead, code to some best-case
standard (Linux, POSIX, whatever) and if necessary, use wrappers that
work around the deficiencies of less-well-endowed systems.  For example,
coreutils assumes that functions like fchdir and openat work properly.
Even though some really crufty systems have no fchdir syscall, and many
relatively modern systems lack openat, fstatat, etc., all of the coreutils
programs work fine in spite of that: they rely on gnulib's emulation
of those underlying functions.  This lets applications keep free of
the dirty underlying portability details and concentrate on algorithms.
Then there are the all of the wrappers for functions like stat, lstat,
rename, etc. that misbehave on certain losing (but still in use) systems.
I've been "accommodating" deficient systems for a very long time, so
know from experience that it can be done cleanly.

If this little corner of portability matters to you, it wouldn't be
hard to write dup and/or fdopen wrappers that do the right thing on
whatever buggy system you find you have to use.  Incidentally, one nice
(essential) thing about the gnulib framework is that full-featured systems
usually incur *absolutely no* overhead, because they don't even use the
wrapper functions.

>> > Then put file name and the line number in!
>>
>> Surely you're playing devil's advocate, now...
>
> No, I'm just thinking aloud about whether I should start preparing
> a reverting commit for your changes just to be able to continue
> using Git on that mismanaged, stupid, slow and very real

No need to go to such an extreme...
Unless that particular diagnostic really matters to you.

> system I have to work with. It is a Win2k laptop, in a Novell
> network and with Cygwin on (actually 2 cygwins at least, which
> cygwin does not support), and without a chance to _EVER_ use
> anything else.

You have my sympathy :-/

Re: [PATCH] git-log: detect dup and fdopen failure

[Linus Torvalds]

On Wed, 27 Jun 2007, Jim Meyering wrote:
> 
> Subject: [PATCH] git-log: detect dup and fdopen failure
>  git-compat-util.h (xdup, xfdopen): Define functions.
> 
> Signed-off-by: Jim Meyering <jim@meyering.net>

Acked-by: Linus Torvalds <torvalds@linux-foundation.org>

That said, the whole "prepend 'x' to the function name" thing is obviously 
how git does things, but maybe it would be more readable in the long term 
to make the prefix be 'safe_' instead of 'x', or something like that?

The 'x' thing is the fairly traditional thing to do for malloc(), and 
that's where it comes from, of course. In git (and other places: google 
code shows that other projects have ended up with the same kind of things, 
with busybox being one example I found), it then got extended to 
xread/xwrite, but now it's getting extended so much that I'd worry a bit 
that in the long run the easy-to-miss 'x' part really would be better off 
written out a bit more.

No really strong opinion, just throwing it out for comment.

It was brought on by the fact that I mentally parsed "xfdopen()" as "xf" + 
"dopen" for some reason (but maybe that's just me).

			Linus

Re: [PATCH] git-log: detect dup and fdopen failure

[Josh Triplett]

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

Linus Torvalds wrote:
> On Wed, 27 Jun 2007, Jim Meyering wrote:
>> Subject: [PATCH] git-log: detect dup and fdopen failure
>>  git-compat-util.h (xdup, xfdopen): Define functions.
>>
>> Signed-off-by: Jim Meyering <jim@meyering.net>
> 
> Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> That said, the whole "prepend 'x' to the function name" thing is obviously 
> how git does things, but maybe it would be more readable in the long term 
> to make the prefix be 'safe_' instead of 'x', or something like that?

If you want a more verbose name than xfoo, I personally like the foo_or_die
convention, which reminds you explicitly that the function might kill the
program.  safe_ might convey exactly the opposite impression.

- Josh Triplett


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [PATCH] git-log: detect dup and fdopen failure

[Shawn O. Pearce]

Josh Triplett <josh@freedesktop.org> wrote:
> Linus Torvalds wrote:
> > That said, the whole "prepend 'x' to the function name" thing is obviously 
> > how git does things, but maybe it would be more readable in the long term 
> > to make the prefix be 'safe_' instead of 'x', or something like that?
> 
> If you want a more verbose name than xfoo, I personally like the foo_or_die
> convention, which reminds you explicitly that the function might kill the
> program.  safe_ might convey exactly the opposite impression.

I think the _gently suffix is already preferred for the
non-die()'ing variant of a function within Git.  For example see
setup_git_directory_gently() or lookup_commit_reference_gently().

Of course pkt-line.c goes and defines safe_read/safe_write.
And lets not forget about safe_create_dir() and
safe_create_leading_directories().

I guess we're about half-way in both directions.  Next function
declared with safe_ prefix or _gently suffix will tip the scale in
one direction or the other.  ;-)

-- 
Shawn.