Tuesday 11 October 2011

Sometimes I am just dumb

I have recently been working on some code for NetSurf which builds up an output buffer by repeatedly calling snprintf(); No great shock there, well understood trivial pattern that has been used repeatedly for ages.

Of course I discovered a buffer overflow, which to be fair had already been pointed out to me by another developer and I just failed to see it...Can I blame my old age? no? bah, get off my lawn!

Basically it boils down to me simply not seeing where C helpfully let me subtract one size_t typed value from another for a length and me completely forgetting that a negative result would simply become a large positive value...

I (erroneously) beleived snprintf took a (signed) int as the buffer length, of course it returns one, but it takes a size_t which is, of course unsigned.
Gosh I feel silly now, in fact I was so convinced I was right I wrote a program to "prove" it.
1 /* snprintf.c
2 *
3 * snprintf example
4 *
5 * Public domain (really I do not think its even copyrightable anyway)
6 *
7 * cc -Wall -o snprintf-ex snprintf-ex.c
8 */

9
10 #include <stdio.h>
11 #include <string.h>
12
13 #define SHOW printf("%3ld %.*s*%s\n", string_len - slen, (int)slen, string, string + strlen(string) + 1 )
14
15
16 int main(int argc, char**argv)
17 {
18 char string[64];
19 size_t string_len = sizeof(string) / 2;
20 int bloop;
21 size_t slen = 0;
22
23 /* initialise string */
24 for (bloop = 0; bloop < (sizeof(string) - 1); bloop++) {
25 string[bloop] = '0' + (bloop % 10);
26 }
27 string[bloop] = 0; /* null terminate */
28
29 printf("%3ld %s\n", string_len, string);
30
31 /* try an empty string */
32 slen += snprintf(string + slen, string_len - slen, "%s", "");
33
34 SHOW;
35
36 /* blah blah */
37 slen += snprintf(string + slen, string_len - slen, "Lorem ipsum dolor");
38
39 SHOW;
40
41 /* this one should exceed the allowed length */
42 slen += snprintf(string + slen, string_len - slen, "Lorem ipsum dolor");
43
44 SHOW;
45
46 /* should not call snprintf up if slen exceeds string_len as the
47 * subtraction results in a negative length and snprintf takes a unisigned
48 * size!
49 */

50
51 /* this one starts exceeding the allowed length */
52 slen += snprintf(string + slen, string_len - slen, "Lorem ipsum dolor");
53
54 SHOW;
55
56 return 0;
57 }

Of course all this really proved was that I was wrong and I needed to clean up the original code as soon as possible.

A good lesson to learn here is that no matter how experienced you are, you can be mistaken and that, perhaps, some redemption I can take from this is I have matured enough as a programmer to write a test program to prove myself wrong!

No comments:

Post a Comment