Closed GoogleCodeExporter closed 9 years ago
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
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
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
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
Original issue reported on code.google.com by
eric.d.l...@gmail.com
on 14 Mar 2009 at 12:30