"Alex Riesen" [off-list ref] wrote:
On 6/27/07, Jim Meyering [off-list ref] wrote:
quoted
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 <redacted>
---
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) {On 6/27/07, Jim Meyering [off-list ref] 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).
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