[PATCH] e2fsck/e2fsprogs: use sscanf() instead of atoi() in the option parser

[PATCH] e2fsck/e2fsprogs: use sscanf() instead of atoi() in the option parser

[Bernd Schubert]

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

[resent since the first mail doesn't seem to make it to this list]

Hi,

using atoi() should be avoided in the option parser since it doesn't check
for errors. I almost destroyed an important filesystem since I
specified "e2fsck -C -n" and -n was parsed as option to -C


Cheers,
Bernd



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sscanf_instead_of_atoi.patch --]
[-- Type: text/x-diff; name="sscanf_instead_of_atoi.patch", Size: 2247 bytes --]

diff -r 7b057872ec06 e2fsck/unix.c
--- a/e2fsck/unix.c	Fri Jul 27 16:08:50 2007 +0200
+++ b/e2fsck/unix.c	Tue Jul 31 12:20:22 2007 +0200
@@ -585,6 +585,10 @@ static errcode_t PRS(int argc, char *arg
 #endif
 	char		*extended_opts = 0;
 	char		*cp;
+	int 		res;		/* result of sscanf */
+#ifdef CONFIG_JBD_DEBUG
+	char 		*jbd_debug;
+#endif
 
 	retval = e2fsck_allocate_context(&ctx);
 	if (retval)
@@ -614,7 +618,10 @@ static errcode_t PRS(int argc, char *arg
 		switch (c) {
 		case 'C':
 			ctx->progress = e2fsck_update_progress;
-			ctx->progress_fd = atoi(optarg);
+			res = sscanf(optarg, "%d", &ctx->progress_fd);
+			if (res != 1)
+				goto sscanf_err;
+
 			if (!ctx->progress_fd)
 				break;
 			/* Validate the file descriptor to avoid disasters */
@@ -674,20 +681,26 @@ static errcode_t PRS(int argc, char *arg
 			/* What we do by default, anyway! */
 			break;
 		case 'b':
-			ctx->use_superblock = atoi(optarg);
+			res = sscanf(optarg, "%d", &ctx->use_superblock);
+			if (res != 1)
+				goto sscanf_err;
 			ctx->flags |= E2F_FLAG_SB_SPECIFIED;
 			break;
 		case 'B':
 			ctx->blocksize = atoi(optarg);
 			break;
 		case 'I':
-			ctx->inode_buffer_blocks = atoi(optarg);
+			res = sscanf(optarg, "%d", &ctx->inode_buffer_blocks);
+			if (res != 1)
+				goto sscanf_err;
 			break;
 		case 'j':
 			ctx->journal_name = string_copy(ctx, optarg, 0);
 			break;
 		case 'P':
-			ctx->process_inode_size = atoi(optarg);
+			res = sscanf(optarg, "%d", &ctx->process_inode_size);
+			if (res != 1)
+				goto sscanf_err;
 			break;
 		case 'L':
 			replace_bad_blocks++;
@@ -830,10 +843,22 @@ static errcode_t PRS(int argc, char *arg
 		putenv(newpath);
 	}
 #ifdef CONFIG_JBD_DEBUG
-	if (getenv("E2FSCK_JBD_DEBUG"))
-		journal_enable_debug = atoi(getenv("E2FSCK_JBD_DEBUG"));
+	jbd_debug = getenv("E2FSCK_JBD_DEBUG");
+	if (jbd_debug)
+		res = sscanf(jbd_debug, "%d", &journal_enable_debug);
+		if (res != 1) {
+			fprintf(stderr,
+			        _("\nInvalid argument \"%s\", not an integer\n\n"),
+			        jbd_debug);
+			exit (1);
+		}
 #endif
 	return 0;
+
+sscanf_err:
+	fprintf(stderr, _("\nInvalid argument \"%s\", not an integer\n\n"),
+	        optarg);
+	exit (1);
 }
 
 static const char *my_ver_string = E2FSPROGS_VERSION;

Re: [PATCH] e2fsck/e2fsprogs: use sscanf() instead of atoi() in the option parser

[Theodore Tso]

On Wed, Aug 01, 2007 at 04:50:04PM +0200, Bernd Schubert wrote:
> [resent since the first mail doesn't seem to make it to this list]
> 
> using atoi() should be avoided in the option parser since it doesn't check
> for errors. I almost destroyed an important filesystem since I
> specified "e2fsck -C -n" and -n was parsed as option to -C

Hi Bernd,

Can you please include a DCO.  i.e.,

Signed-off-by: Bernd Schubert <bschubert@q-leap.de>

In your patch submissions.  See the SUBMITTING-PATCHES file in the
top-level directory of e2fsprogs source tree for the significance of
the Signed-off-by line.  For this patch, can you indicate whether you
are willing to add a Signed-off-by: DCO?

Thanks, regards,

						- Ted

Re: [PATCH] e2fsck/e2fsprogs: use sscanf() instead of atoi() in the option parser

[Bernd Schubert]

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

Hello Ted,

> In your patch submissions.  See the SUBMITTING-PATCHES file in the
> top-level directory of e2fsprogs source tree for the significance of
> the Signed-off-by line.  For this patch, can you indicate whether you
> are willing to add a Signed-off-by: DCO?

sorry that I did not read that file and so didn't know a signed-off is 
required.

Sure I'm willing to add a signed-off :) I'm also adding the patch again to 
this mail.


Signed-off-by: Bernd Schubert <bs@q-leap.de>


Cheers,
Bernd

-- 
Bernd Schubert
Q-Leap Networks GmbH

[-- Attachment #2: sscanf_instead_of_atoi.patch --]
[-- Type: text/x-diff, Size: 2246 bytes --]

diff -r 7b057872ec06 e2fsck/unix.c
--- a/e2fsck/unix.c	Fri Jul 27 16:08:50 2007 +0200
+++ b/e2fsck/unix.c	Tue Jul 31 12:20:22 2007 +0200
@@ -585,6 +585,10 @@ static errcode_t PRS(int argc, char *arg
 #endif
 	char		*extended_opts = 0;
 	char		*cp;
+	int 		res;		/* result of sscanf */
+#ifdef CONFIG_JBD_DEBUG
+	char 		*jbd_debug;
+#endif
 
 	retval = e2fsck_allocate_context(&ctx);
 	if (retval)
@@ -614,7 +618,10 @@ static errcode_t PRS(int argc, char *arg
 		switch (c) {
 		case 'C':
 			ctx->progress = e2fsck_update_progress;
-			ctx->progress_fd = atoi(optarg);
+			res = sscanf(optarg, "%d", &ctx->progress_fd);
+			if (res != 1)
+				goto sscanf_err;
+
 			if (!ctx->progress_fd)
 				break;
 			/* Validate the file descriptor to avoid disasters */
@@ -674,20 +681,26 @@ static errcode_t PRS(int argc, char *arg
 			/* What we do by default, anyway! */
 			break;
 		case 'b':
-			ctx->use_superblock = atoi(optarg);
+			res = sscanf(optarg, "%d", &ctx->use_superblock);
+			if (res != 1)
+				goto sscanf_err;
 			ctx->flags |= E2F_FLAG_SB_SPECIFIED;
 			break;
 		case 'B':
 			ctx->blocksize = atoi(optarg);
 			break;
 		case 'I':
-			ctx->inode_buffer_blocks = atoi(optarg);
+			res = sscanf(optarg, "%d", &ctx->inode_buffer_blocks);
+			if (res != 1)
+				goto sscanf_err;
 			break;
 		case 'j':
 			ctx->journal_name = string_copy(ctx, optarg, 0);
 			break;
 		case 'P':
-			ctx->process_inode_size = atoi(optarg);
+			res = sscanf(optarg, "%d", &ctx->process_inode_size);
+			if (res != 1)
+				goto sscanf_err;
 			break;
 		case 'L':
 			replace_bad_blocks++;
@@ -830,10 +843,22 @@ static errcode_t PRS(int argc, char *arg
 		putenv(newpath);
 	}
 #ifdef CONFIG_JBD_DEBUG
-	if (getenv("E2FSCK_JBD_DEBUG"))
-		journal_enable_debug = atoi(getenv("E2FSCK_JBD_DEBUG"));
+	jbd_debug = getenv("E2FSCK_JBD_DEBUG");
+	if (jbd_debug)
+		res = sscanf(jbd_debug, "%d", &journal_enable_debug);
+		if (res != 1) {
+			fprintf(stderr,
+			        _("\nInvalid argument \"%s\", not an integer\n\n"),
+			        jbd_debug);
+			exit (1);
+		}
 #endif
 	return 0;
+
+sscanf_err:
+	fprintf(stderr, _("\nInvalid argument \"%s\", not an integer\n\n"),
+	        optarg);
+	exit (1);
 }
 
 static const char *my_ver_string = E2FSPROGS_VERSION;