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

[Ken Raeburn <raeburn@permabit.com>]

Jens Axboe <axboe@kernel.dk> writes:

> On Mon, Jan 28 2013, Ken Raeburn wrote:
>> 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.

> And:
>
>> +	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