[2/2] e2fsck: remove unnecessary reserve_stdio_fds()

Message ID 20180809083546.17419-2-lczerner@redhat.com
State Rejected
Headers show
Series
  • [1/2] e2fsprogs: remove unused datarootdir
Related show

Commit Message

Lukas Czerner Aug. 9, 2018, 8:35 a.m.
Standard stream are always open, we do not need to "check" it
specifically. Remove reserve_stdio_fds().

This also fixes the file descriptor leak.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 e2fsck/unix.c | 20 --------------------
 1 file changed, 20 deletions(-)

Comments

Theodore Y. Ts'o Aug. 12, 2018, 12:47 a.m. | #1
On Thu, Aug 09, 2018 at 10:35:46AM +0200, Lukas Czerner wrote:
> Standard stream are always open, we do not need to "check" it
> specifically. Remove reserve_stdio_fds().

I don't remember the exact circumstances, since it was twenty years
ago, but that's not *always* true.  Remember that e2fsck is run by
init or some other early boot sequence, and not all such programs
are.... sane.  In fact, this was added because some buggy init called
e2fsck with fd 2 closed, so the file system was opened using fd 2.
And then an error message ended up corrupting the file system....

This function was added in response to that failure, because
sometimes, the world *is* out to get you.  It is fair to fix the file
descriptor leak, so how about this?

					- Ted


commit 352701d9e2258299fe3bffb1d112566c0e4a7cdf
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Sat Aug 11 20:47:08 2018 -0400

    e2fsck: fix fd leak in reserve_stdio_fds
    
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index 90065b395..2df22b171 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -617,9 +617,10 @@ static void reserve_stdio_fds(void)
 			fprintf(stderr, _("ERROR: Couldn't open "
 				"/dev/null (%s)\n"),
 				strerror(errno));
-			break;
+			return;
 		}
 	}
+	(void) close(fd);
 }
 
 #ifdef HAVE_SIGNAL_H
Lukas Czerner Aug. 13, 2018, 8:11 a.m. | #2
On Sat, Aug 11, 2018 at 08:47:43PM -0400, Theodore Y. Ts'o wrote:
> On Thu, Aug 09, 2018 at 10:35:46AM +0200, Lukas Czerner wrote:
> > Standard stream are always open, we do not need to "check" it
> > specifically. Remove reserve_stdio_fds().
> 
> I don't remember the exact circumstances, since it was twenty years
> ago, but that's not *always* true.  Remember that e2fsck is run by
> init or some other early boot sequence, and not all such programs
> are.... sane.  In fact, this was added because some buggy init called
> e2fsck with fd 2 closed, so the file system was opened using fd 2.
> And then an error message ended up corrupting the file system....

Uff, even though we do not really want to optimize for a broken code, it
does make sense to me to protect user from possible data loss in thi
crazy case.

> 
> This function was added in response to that failure, because
> sometimes, the world *is* out to get you.  It is fair to fix the file
> descriptor leak, so how about this?

Fair enough.

Reviewed-by: Lukas Czerner <lczerner@redhat.com>

Thanks!
-Lukas


> 
> 					- Ted
> 
> 
> commit 352701d9e2258299fe3bffb1d112566c0e4a7cdf
> Author: Theodore Ts'o <tytso@mit.edu>
> Date:   Sat Aug 11 20:47:08 2018 -0400
> 
>     e2fsck: fix fd leak in reserve_stdio_fds
>     
>     Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> 
> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
> index 90065b395..2df22b171 100644
> --- a/e2fsck/unix.c
> +++ b/e2fsck/unix.c
> @@ -617,9 +617,10 @@ static void reserve_stdio_fds(void)
>  			fprintf(stderr, _("ERROR: Couldn't open "
>  				"/dev/null (%s)\n"),
>  				strerror(errno));
> -			break;
> +			return;
>  		}
>  	}
> +	(void) close(fd);
>  }
>  
>  #ifdef HAVE_SIGNAL_H

Patch

diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index 90065b39..268feee0 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -603,25 +603,6 @@  static int e2fsck_update_progress(e2fsck_t ctx, int pass,
 
 #define PATH_SET "PATH=/sbin"
 
-/*
- * Make sure 0,1,2 file descriptors are open, so that we don't open
- * the filesystem using the same file descriptor as stdout or stderr.
- */
-static void reserve_stdio_fds(void)
-{
-	int	fd = 0;
-
-	while (fd <= 2) {
-		fd = open("/dev/null", O_RDWR);
-		if (fd < 0) {
-			fprintf(stderr, _("ERROR: Couldn't open "
-				"/dev/null (%s)\n"),
-				strerror(errno));
-			break;
-		}
-	}
-}
-
 #ifdef HAVE_SIGNAL_H
 static void signal_progress_on(int sig EXT2FS_ATTR((unused)))
 {
@@ -1411,7 +1392,6 @@  int main (int argc, char *argv[])
 			_("while trying to initialize program"));
 		exit(FSCK_ERROR);
 	}
-	reserve_stdio_fds();
 
 	set_up_logging(ctx);
 	if (ctx->logf) {