path name lengths

PATCH In review Ken Raeburn 2 participants 10 messages Jan 29, 2013 – Jan 30, 2013

Share & tools

Ken Raeburn

author

2013-01-29 01:31 UTC #0 lore raw

I was trying to set up a somewhat perverse test to look at the cost of using LVMs on Linux, by stacking them many levels deep. When I ran fio, it crashed before exiting, reporting memory corruption.

I've found a couple issues still present in today's git sources:

The path array in struct disk_util is only 256 bytes long, and there's no check to avoid overflowing it, so if the string you try to put into it is "/sys/block/dm-47/slaves/../../dm-46/slaves/../../dm-45/slaves/../.." (you get the idea), you can easily scribble past the end of the array, and the containing disk_util structure.

I'll have a patch for this shortly.

In find_add_disk_slaves, where the code uses readlink on entries found in the "slaves" subdirectory and appends to the previous directory name, there is no check for exceeding the size of the automatic character arrays. Since those arrays are 4KB I'm not having any problems with them, but it might be good to add checks there.

Ken

Ken Raeburn

author

2013-01-29 01:48 UTC #1 lore raw

PATCH 1/1 Fix crash with absurdly but not impossibly deeply nested device stacks.

diskutil.c: Check for overflow in disk_util.path.
diskutil.h: Expand disk_util.path to PATH_MAX.

---
 diskutil.c |    8 +++++++-
 diskutil.h |    2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

diskutil.c +7 -1

index fbc4268..22dc58f 100644
@@ -276,13 +276,19 @@ static struct disk_util *disk_util_add(struct thread_data *td, int majdev,
 {
 	struct disk_util *du, *__du;
 	struct flist_head *entry;
+	int l;
 
 	dprint(FD_DISKUTIL, "add maj/min %d/%d: %s\n", majdev, mindev, path);
 
 	du = smalloc(sizeof(*du));
 	memset(du, 0, sizeof(*du));
 	INIT_FLIST_HEAD(&du->list);
-	sprintf(du->path, "%s/stat", path);
+	l = snprintf(du->path, sizeof(du->path), "%s/stat", path);
+	if (l < 0 || l >= sizeof(du->path)) {
+		log_err("constructed path \"%.100s[...]/stat\" larger than buffer (%zu bytes)\n",
+			path, sizeof(du->path) - 1);
+		exit(1);
+	}
 	strncpy((char *) du->dus.name, basename(path), FIO_DU_NAME_SZ);
 	du->sysfs_root = path;
 	du->major = majdev;

diskutil.h +1 -1

index b89aacc..ddd6471 100644
@@ -42,7 +42,7 @@ struct disk_util {
 
 	char *name;
 	char *sysfs_root;
-	char path[256];
+	char path[PATH_MAX];
 	int major, minor;
 
 	struct disk_util_stat dus;

Signature · 1 line

1.7.9.5

Jens Axboe

2013-01-29 09:17 UTC #2 lore raw

Re: [PATCH] Fix crash with absurdly but not impossibly deeply nested device stacks.

Quoted 2 lines · Ken Raeburn

diskutil.c: Check for overflow in disk_util.path.
diskutil.h: Expand disk_util.path to PATH_MAX.

Good stuff, thanks. Though I think that we should just return NULL on failing to setup the path.

And:

diskutil.c static struct disk_util *disk_util_add(struct thread_data *td, int majdev, · line 287

Quoted 2 lines

diskutil.c +2 -0

+	l = snprintf(du->path, sizeof(du->path), "%s/stat", path);
+	if (l < 0 || l >= sizeof(du->path)) {

cosmetically, that should never be > sizeof(du->path), but it doesn't hurt.

Signature · 1 line

Jens Axboe

Jens Axboe

2013-01-29 09:19 UTC #3 lore raw

Re: [PATCH] Fix crash with absurdly but not impossibly deeply nested device stacks.

Quoted 7 lines · Jens Axboe

Quoted 2 lines · Ken Raeburn
diskutil.c: Check for overflow in disk_util.path.
diskutil.h: Expand disk_util.path to PATH_MAX.
Good stuff, thanks. Though I think that we should just return NULL on failing to setup the path.

Quoted 7 lines

And:
diskutil.c static struct disk_util *disk_util_add(struct thread_data *td, int majdev, · line 287
Quoted 2 lines
diskutil.c +2 -0
+	l = snprintf(du->path, sizeof(du->path), "%s/stat", path);
+	if (l < 0 || l >= sizeof(du->path)) {
cosmetically, that should never be > sizeof(du->path), but it doesn't hurt.

Committed this:

http://git.kernel.dk/?p=fio.git;a=commit;h=4b919f7740f4875d73c0619a08d269d5c679e19f

Signature · 1 line

Jens Axboe

Ken Raeburn

author

2013-01-29 21:06 UTC #4 lore raw

Re: [PATCH] Fix crash with absurdly but not impossibly deeply nested device stacks.

In reply to Jens Axboe

Quoted 6 lines · Jens Axboe

Quoted 2 lines · Ken Raeburn
diskutil.c: Check for overflow in disk_util.path.
diskutil.h: Expand disk_util.path to PATH_MAX.
Good stuff, thanks. Though I think that we should just return NULL on failing to setup the path.

That seems fine, too. The preferred error handling in that area of the code wasn't clear to me. Especially since smalloc failure (which doesn't appear to be impossible) leads to a null pointer dereference.

Quoted 7 lines

And:
diskutil.c static struct disk_util *disk_util_add(struct thread_data *td, int majdev, · line 287
Quoted 2 lines
diskutil.c +2 -0
+	l = snprintf(du->path, sizeof(du->path), "%s/stat", path);
+	if (l < 0 || l >= sizeof(du->path)) {
cosmetically, that should never be > sizeof(du->path), but it doesn't hurt.

According to the GNU libc man page, in truncation cases, snprintf returns the number of characters that would have been written, excluding the trailing \0, if the buffer were long enough. So if we're appending "/stat" to something just under the buffer size, the return value could be larger.

So, actually, I think the vsnprintf usage in log.c is wrong in assuming the return value is no more than the buffer size...

Also, skimming the other uses, I think some of the other calls don't really need to subtract one from sizeof(buffer), since the passed length is the maximum number of bytes written, always including a trailing \0.

Ken

Jens Axboe

2013-01-29 21:15 UTC #5 lore raw

Re: [PATCH] Fix crash with absurdly but not impossibly deeply nested device stacks.

Quoted 6 lines · Ken Raeburn

Quoted 4 lines · Jens Axboe
Quoted 2 lines · Ken Raeburn
diskutil.c: Check for overflow in disk_util.path.
diskutil.h: Expand disk_util.path to PATH_MAX.

Quoted 6 lines

Quoted 2 lines
Good stuff, thanks. Though I think that we should just return NULL on failing to setup the path.
That seems fine, too. The preferred error handling in that area of the code wasn't clear to me. Especially since smalloc failure (which doesn't appear to be impossible) leads to a null pointer dereference.

Those should be fixed up, smalloc() failure can happen. Fio definitely isn't malloc() fail proof, usually it happily ignores that. But that smalloc() isn't caught in diskutil.c is an oversight.

Quoted 8 lines

Quoted 7 lines
And:
diskutil.c static struct disk_util *disk_util_add(struct thread_data *td, int majdev, · line 287
Quoted 2 lines
diskutil.c +2 -0
+	l = snprintf(du->path, sizeof(du->path), "%s/stat", path);
+	if (l < 0 || l >= sizeof(du->path)) {
cosmetically, that should never be > sizeof(du->path), but it doesn't hurt.

Quoted 5 lines

According to the GNU libc man page, in truncation cases, snprintf returns the number of characters that would have been written, excluding the trailing \0, if the buffer were long enough. So if we're appending "/stat" to something just under the buffer size, the return value could be larger.

Good point!

Quoted 2 lines

So, actually, I think the vsnprintf usage in log.c is wrong in assuming the return value is no more than the buffer size...

Indeed they are. Can't believe I never realized how horrible an API the *snprintf() functions are.

Quoted 3 lines

Also, skimming the other uses, I think some of the other calls don't really need to subtract one from sizeof(buffer), since the passed length is the maximum number of bytes written, always including a trailing \0.

Care to send in a patch or patches?

Signature · 1 line

Jens Axboe

Ken Raeburn

author

2013-01-29 22:09 UTC #6 lore raw

Re: [PATCH] Fix crash with absurdly but not impossibly deeply nested device stacks.

Sure, I'll work up some patches for the [v]snprintf calls and the missing smalloc failure check...

Ken

Jens Axboe

2013-01-30 11:59 UTC #7 lore raw

Re: [PATCH] Fix crash with absurdly but not impossibly deeply nested device stacks.

Quoted 3 lines · Ken Raeburn

Sure, I'll work up some patches for the [v]snprintf calls and the missing smalloc failure check...

I patched up the smalloc already, but the *snprintf() stuff would be excellent to have fixed up.

Signature · 1 line

Jens Axboe

Ken Raeburn

author

2013-01-30 21:25 UTC #8 lore raw

PATCH 1/1 Fix bugs in [v]snprintf usage.

When calling snprintf, supply the full buffer size instead of (usually) one byte less.

When using the returned length from vsnprintf for logging, don't write more than the actual buffer size (minus one for the trailing \0), in case the formatted string was truncated.

---
 engines/falloc.c |    2 +-
 filesetup.c      |    4 ++--
 fio.h            |    2 +-
 init.c           |    2 +-
 iolog.c          |    2 +-
 log.c            |    4 ++++
 server.c         |    1 +
 stat.c           |    6 +++---
 t/log.c          |    2 ++
 9 files changed, 16 insertions(+), 9 deletions(-)

engines/falloc.c +1 -1

index 525a0aa..4654fe8 100644
@@ -44,7 +44,7 @@ open_again:
 	if (f->fd == -1) {
 		char buf[FIO_VERROR_SIZE];
 		int __e = errno;
-		snprintf(buf, sizeof(buf) - 1, "open(%s)", f->file_name);
+		snprintf(buf, sizeof(buf), "open(%s)", f->file_name);
 		td_verror(td, __e, buf);
 	}

filesetup.c +2 -2

index 6f0a876..5aadf12 100644
@@ -563,7 +563,7 @@ open_again:
 		if (__e == EMFILE && file_close_shadow_fds(td))
 			goto open_again;
 
-		snprintf(buf, sizeof(buf) - 1, "open(%s)", f->file_name);
+		snprintf(buf, sizeof(buf), "open(%s)", f->file_name);
 
 		if (__e == EINVAL && (flags & OS_O_DIRECT)) {
 			log_err("fio: looks like your file system does not " \

@@ -1250,7 +1250,7 @@ static int recurse_dir(struct thread_data *td, const char *dirname)
 	if (!D) {
 		char buf[FIO_VERROR_SIZE];
 
-		snprintf(buf, FIO_VERROR_SIZE - 1, "opendir(%s)", dirname);
+		snprintf(buf, FIO_VERROR_SIZE, "opendir(%s)", dirname);
 		td_verror(td, errno, buf);
 		return 1;
 	}

fio.h +1 -1

index 2fd354a..d18029a 100644
@@ -568,7 +568,7 @@ enum {
 		int e = (err);						\
 		(td)->error = e;					\
 		if (!(td)->first_error)					\
-			snprintf(td->verror, sizeof(td->verror) - 1, "file:%s:%d, func=%s, error=%s", __FILE__, __LINE__, (func), (msg));		\
+			snprintf(td->verror, sizeof(td->verror), "file:%s:%d, func=%s, error=%s", __FILE__, __LINE__, (func), (msg));		\
 	} while (0)

init.c +1 -1

index f0ad019..dfc5a8f 100644
@@ -627,7 +627,7 @@ static char *to_kmg(unsigned int val)
 		p++;
 	} while (*p);
 
-	snprintf(buf, 31, "%u%c", val, *p);
+	snprintf(buf, 32, "%u%c", val, *p);
 	return buf;
 }

iolog.c +1 -1

index 12f09d0..e4c1fef 100644
@@ -534,7 +534,7 @@ void finish_log_named(struct thread_data *td, struct io_log *log,
 {
 	char file_name[256], *p;
 
-	snprintf(file_name, 200, "%s_%s.log", prefix, postfix);
+	snprintf(file_name, sizeof(file_name), "%s_%s.log", prefix, postfix);
 	p = basename(file_name);
 	__finish_log(log, p);
 }

log.c +4 -0

index af974f8..08509b3 100644
@@ -12,6 +12,7 @@ int log_valist(const char *str, va_list args)
 	size_t len;
 
 	len = vsnprintf(buffer, sizeof(buffer), str, args);
+	len = min(len, sizeof(buffer) - 1);
 
 	if (log_syslog)
 		syslog(LOG_INFO, "%s", buffer);

@@ -40,6 +41,7 @@ int log_local(const char *format, ...)
 	va_start(args, format);
 	len = vsnprintf(buffer, sizeof(buffer), format, args);
 	va_end(args);
+	len = min(len, sizeof(buffer) - 1);
 
 	if (log_syslog)
 		syslog(LOG_INFO, "%s", buffer);

@@ -58,6 +60,7 @@ int log_info(const char *format, ...)
 	va_start(args, format);
 	len = vsnprintf(buffer, sizeof(buffer), format, args);
 	va_end(args);
+	len = min(len, sizeof(buffer) - 1);
 
 	if (is_backend)
 		return fio_server_text_output(buffer, len);

@@ -77,6 +80,7 @@ int log_err(const char *format, ...)
 	va_start(args, format);
 	len = vsnprintf(buffer, sizeof(buffer), format, args);
 	va_end(args);
+	len = min(len, sizeof(buffer) - 1);
 
 	if (is_backend)
 		return fio_server_text_output(buffer, len);

server.c +1 -0

index 7ec8531..ad78572 100644
@@ -811,6 +811,7 @@ int fio_server_log(const char *format, ...)
 	va_start(args, format);
 	len = vsnprintf(buffer, sizeof(buffer), format, args);
 	va_end(args);
+	len = min(len, sizeof(buffer) - 1);
 
 	return fio_server_text_output(buffer, len);
 }

stat.c +3 -3

index 7e2feea..62eee9a 100644
@@ -753,7 +753,7 @@ static void add_ddir_status_json(struct thread_stat *ts,
 			json_object_add_value_int(percentile_object, "0.00", 0);
 			continue;
 		}
-		snprintf(buf, sizeof(buf) - 1, "%2.2f", ts->percentile_list[i].u.f);
+		snprintf(buf, sizeof(buf), "%2.2f", ts->percentile_list[i].u.f);
 		json_object_add_value_int(percentile_object, (const char *)buf, ovals[i]);
 	}

@@ -959,9 +959,9 @@ static struct json_object *show_thread_status_json(struct thread_stat *ts,
 	for (i = 0; i < 7; i++) {
 		char name[20];
 		if (i < 6)
-			snprintf(name, 19, "%d", 1 << i);
+			snprintf(name, 20, "%d", 1 << i);
 		else
-			snprintf(name, 19, ">=%d", 1 << i);
+			snprintf(name, 20, ">=%d", 1 << i);
 		json_object_add_value_float(tmp, (const char *)name, io_u_dist[i]);
 	}

t/log.c +2 -0

index ac02303..76ae68e 100644
@@ -10,6 +10,7 @@ int log_err(const char *format, ...)
 	va_start(args, format);
 	len = vsnprintf(buffer, sizeof(buffer), format, args);
 	va_end(args);
+	len = min(len, sizeof(buffer) - 1);
 
 	return fwrite(buffer, len, 1, stderr);
 }

@@ -23,6 +24,7 @@ int log_info(const char *format, ...)
 	va_start(args, format);
 	len = vsnprintf(buffer, sizeof(buffer), format, args);
 	va_end(args);
+	len = min(len, sizeof(buffer) - 1);
 
 	return fwrite(buffer, len, 1, stdout);
 }

Signature · 1 line

1.7.9.5

Jens Axboe

2013-01-30 21:49 UTC #9 lore raw

Re: [PATCH] Fix bugs in [v]snprintf usage.

Quoted 7 lines · Ken Raeburn

When calling snprintf, supply the full buffer size instead of (usually) one byte less.
When using the returned length from vsnprintf for logging, don't write more than the actual buffer size (minus one for the trailing \0), in case the formatted string was truncated.

Thanks Ken, applied.

Signature · 1 line

Jens Axboe