1.3.2 stats call can result in segfault due to buffer over flow in memcached.c:server_stats

[GoogleCodeExporter]

the server_stats function allocates a 1024 buffer to hold the stats as it 
collects them. A ptr called 
pos is used to track the location in the buffer and to write new data into the 
buffer. We increment 
the pointer as we move along but do not protect against *pos going off the end 
of the buffer. See 
code below

static char *server_stats(uint32_t (*add_stats)(char *buf, const char *key,
                          const uint16_t klen, const char *val,
                          const uint32_t vlen, void *cookie), conn *c,
                          int *buflen) {
    char temp[1024];
    char val[128];
    char *buf = NULL;
    char *pos = temp;
....
    vlen = sprintf(val, "%lu", (long)pid);
    nbytes = add_stats(pos, "pid", strlen("pid"), val, vlen, (void *)c);
    pos += nbytes;
    *buflen += nbytes;

Original issue reported on code.google.com by eric.d.l...@gmail.com on 14 Mar 2009 at 12:30

[GoogleCodeExporter]

I'm having trouble overrunning that buffer.  I maxed out a bunch of stats and 
only
got it up to 828 -- close, but not enough to do damage:

STAT pid 35893
STAT uptime 152
STAT time 1237000260
STAT version 1.3.2
STAT pointer_size 32
STAT rusage_user 0.002487
STAT rusage_system 0.005412
STAT curr_connections 4
STAT total_connections 5
STAT connection_structures 5
STAT cmd_get 18446744073709551610
STAT cmd_set 18446744073709551610
STAT get_hits 18446744073709551610
STAT get_misses 18446744073709551610
STAT delete_misses 18446744073709551610
STAT delete_hits 18446744073709551610
STAT incr_misses 18446744073709551610
STAT incr_hits 18446744073709551610
STAT decr_misses 18446744073709551610
STAT decr_hits 18446744073709551610
STAT cas_misses 18446744073709551610
STAT cas_hits 18446744073709551610
STAT cas_badval 18446744073709551610
STAT bytes_read 1
STAT bytes_written 18446744073709551610
STAT limit_maxbytes 67108864
STAT threads 5
STAT bytes 18446744073709551610
STAT curr_items 4294967290
STAT total_items 4294967290
STAT evictions 18446744073709551610

  (not sure yet why bytes_read doesn't hold the initial value)

  I'm going to declare this fixed with a larger buffer and assertion.

  Please try it again in 0a7d84694cdbe721aadcc5d327992914fa48dc86 -- or make it
easier to trigger the bug.  :)

Original comment by dsalli...@gmail.com on 14 Mar 2009 at 3:47

• Changed state: Fixed

[GoogleCodeExporter]

Hey Dustin:

I think I see why the bug was not reproduced with your Max out scenario. The 
test I was running when I hit 
this was using the binary protocol to get the stats, a fact I did *not* mention 
in the bug report(sorry),  and the 
example above appears to be using ASCII. (Another difference, though I am sure 
it is not significant, is that 
my test box was a 64 Sun Sparc machine, and looks like you ran this on a 32 bit 
box). I think if we were to 
rerun the test using the binary protocol, we'd overrun the buffer. The binary 
get stat prepends the 24 byte 
binary header to each stat it sends back, where the ASCII get stat only 
prepends the 5 byte 'STAT ' string to 
each stat. My rough 'back of the envelope calculation' indicates that if we 
rerun the Max out scenario and use 
binary get, we would need 1341 bytes, and would overrun the buffer. 

This is all moot of course, cause your fix should handle this. :-)

Eric

Original comment by eric.d.l...@gmail.com on 14 Mar 2009 at 9:52

[GoogleCodeExporter]

Oh good -- that explains a lot.  And thanks for the math.

The code I used to max out the stats was kind of nasty, so I didn't really want 
to
keep it in, but something like that would be good.  The assertion is a start, 
but
it's not as good as more proactive verification.

Original comment by dsalli...@gmail.com on 14 Mar 2009 at 11:14

[GoogleCodeExporter]

Is this bug really fixed?  In append_ascii_stats of memcached.c, I see this 
line:
c->stats.offset += nbytes;

However, nbytes is the return value from snprintf.  The man page states that:
"... then the return value  is  the  number of characters (excluding the 
terminating null byte) which would have been written to the final string"

So after one call to append_ascii_stats, c->stats.offset can be greater than 
c->stats.size.  On a subsequent call, variables remaining and room will be 
negative, but since snprintf takes an unsigned size_t, for the size parameter, 
it will risk overflow.

Propose that c->stats.offset += nbytes become c->stats.offset = 
min(c->stats.offset + nbytes, c->stats.size), and that the function have an 
early return: if (c->stats.offset == c->stats.size) return;

Original comment by mfs...@gmail.com on 29 May 2013 at 9:54