Messages in this thread | | | Date | Fri, 9 Mar 2012 13:00:09 -0600 | From | Peter Seebach <> | Subject | Re: [PATCH] perf: Incorrect use of snprintf results in SEGV |
| |
On Thu, 8 Mar 2012 08:48:37 +0100 Ingo Molnar <mingo@elte.hu> wrote: > Erm. Doing: > > += snprintf(...); > > is a *very* common pattern within the kernel. It occurs more > than a thousand times - i.e. about 25% of all snprintf uses > (~5000 instances) within the kernel does care about the return > value. > > I found only a single case that did a reallocation if the buffer > did not fit. Lets assume that I missed some and there's 4 > altogether.
I came back to this, because I totally thought I saw one of the horse's legs twitch out of the corner of my eye.
See, after this conversation, I decided to go audit some of my code, and sure enough, I found a similar pattern -- I found a couple of places where my code had this idiom, and did not reallocate.
And in both cases, it was *a bug* that the code did not attempt to reallocate.
This led me to reevaluate my assumptions. It appears to me that if you have a buffer of a given size, and for some reason you have more data than will fit in the buffer, you have four options:
1. Scribble outside the buffer. 2. Truncate the data. 3. Reallocate a larger buffer. 4. Report a failure.
In userspace code, I think it is probably nearly always an error to truncate rather than reallocating. I can't think of a case where truncating would be better. Reporting a failure may be reasonable in some cases.
In the kernel, that's going to be a harder call -- there are probably circumstances where reallocating is impractical. But casually browsing through the many cases where no attempt is made to reallocate, I frequently find myself thinking "boy, truncating that would sorta suck."
The assumption that code reflects intended use is a completely reasonable one, but on evaluating the code not for what it does, but for what it probably ought to do, I find that there are many more cases where the correct answer should be "get a larger buffer" rather than "discard data or possibly cut off halfway through a word".
And in userspace, I don't think I've yet found a case where reallocating isn't the right call. (The C library was not really designed with kernel requirements, such as the possibility that allocation might not be an option, in mind.)
So I think a sample of how snprintf() *is* used is not the right way to determine the "common" use case to design for. So far as I can tell, in nearly all cases you need to know whether snprintf needed more space, and you probably *want* to know how much more it needed; otherwise, you are silently producing corrupted data. And indeed, most of the APIs I checked that aren't correctly checking whether snprintf wanted to overflow are, as a result, potentially returning bogus data.
For a specific example, consider mm/mempolicy.c:
if (flags & MPOL_MODE_FLAGS) { if (buffer + maxlen < p + 2) return -ENOSPC; *p++ = '='; /* * Currently, the only defined flags are mutually exclusive */ if (flags & MPOL_F_STATIC_NODES) p += snprintf(p, buffer + maxlen - p, "static"); ...
This code clearly takes great care to ensure that it returns -ENOSPC if there isn't enough space, but does not check that the result of the snprintf call was in range. Failure to check this is a bug. Because the code is full of other careful checks, it won't result in scribbling, but it could result in returning a buffer which ends with "=sta" and reporting too high a length for it.
So studying the code has left me convinced that the snprintf() interface is the right interface, and that the usage errors you've identified are bugs, not because snprintf() returns the wrong value, but because they are failures to check for a condition which needs to be handled.
-s -- Listen, get this. Nobody with a good compiler needs to be justified.
| |