Access beyond allocation

[GoogleCodeExporter]

valgrind just reported this (give me a a day or two
to get up to speed and I'll attach a patch. Feel free to be lazy ;-):

(aside)
Note that this report is with MALLOC_CHECK_ defined
(and the unsetenv in main() removed) and with this voo-doo set:

export MALLOC_CHECK_=3
# http://udrepper.livejournal.com/11429.html
export MALLOC_PERTURB_=$(($RANDOM % 255 + 1))

No idea (yet) whether MALLOC_CHECK_  makes any difference ...

==17290== 1 errors in context 1 of 1:
==17290== Invalid read of size 1
==17290==    at 0x400689A: index (mc_replace_strmem.c:142)
==17290==    by 0x804D4F7: scrape_response (analysis.c:842)
==17290==    by 0x80550C3: dir_404_callback (crawler.c:2341)
==17290==    by 0x8063F42: next_from_queue (http_client.c:2065)
==17290==    by 0x8069534: main (skipfish.c:453)
==17290==  Address 0x451b821 is 0 bytes after a block of size 1 alloc'd
==17290==    at 0x4005BDC: malloc (vg_replace_malloc.c:195)
==17290==    by 0x805CDB2: __DFL_ck_alloc (alloc-inl.h:68)
==17290==    by 0x805FFFE: parse_response (http_client.c:1470)
==17290==    by 0x8063F19: next_from_queue (http_client.c:2057)
==17290==    by 0x8069534: main (skipfish.c:453)

Original issue reported on code.google.com by n3npq....@gmail.com on 5 Jul 2010 at 12:36

[GoogleCodeExporter]

MALLOC_CHECK_ causes known false positives with the custom sanity-checking 
allocator used by skipfish. This is because malloc_usable_size() reports an 
actual size of an allocated chunk, while MALLOC_CHECK_ assumes that only the 
amount explicitly requested by malloc() can be used.

In short, there is a reason why it's disabled :-) If you see any actual 
evidence of memory corruption, please open a separate bug.

Original comment by lcam...@gmail.com on 5 Jul 2010 at 12:41

• Changed state: Invalid

[GoogleCodeExporter]

Sorry ...

Using skipfish-1.45b on RHEL6 beta with this command under valgrind:

skipfish -r 1000 -o xxx http://mylocallanhost.org

Original comment by n3npq....@gmail.com on 5 Jul 2010 at 12:43

[GoogleCodeExporter]

Thanks for info (although I'm not yet seeing false positives).

Meanwhile this hack-o-round made the reproducible error go away:

Index: http_client.c
===================================================================
RCS file: /v/rpm/cvs/skipfish/http_client.c,v
retrieving revision 1.1.1.1
diff -p -u -w -r1.1.1.1 http_client.c
--- http_client.c   4 Jul 2010 19:53:33 -0000   1.1.1.1
+++ http_client.c   5 Jul 2010 01:27:30 -0000
@@ -1467,7 +1467,7 @@ u8 parse_response(struct http_request* r
     /* Identity. Ignore actual C-L data, use just as much as we collected. */

     res->pay_len = data_len - cur_data_off;
-    res->payload = ck_alloc(res->pay_len + 1);
+    res->payload = ck_alloc(res->pay_len + 1 + 1);
     res->payload[res->pay_len] = 0; /* NUL-terminate for safer parsing. */

     memcpy(res->payload, data + cur_data_off, res->pay_len);

Original comment by n3npq....@gmail.com on 5 Jul 2010 at 1:30

[GoogleCodeExporter]

Ah, OK. I don't see anything in analysis.c:842 that would obviously access the 
buffer past pay_len, but maybe - here's an alternative suggestion: revert the 
change, go to said line in analysis.c, and prefix it with:

if (tag_end > res->payload + res->pay_len) {
  DEBUG("=== res->payload ===\n%s\n=== cur_str ===\n",res->payload, cur_str);
  FATAL("Bummer!");
}

...and let me know if this check is tripped, and what it returns.

Original comment by lcam...@gmail.com on 5 Jul 2010 at 2:08

[GoogleCodeExporter]

Done: check is not tripped, but valgrind reports the same error.

BTW, its possible to use valgrind annotations for private/spiffy memory
allocators if this turns out to be a false positive as you explained in
comment #1. Likely not worth the portability pain for skipfish,
but let me dig a bit deeper and figger the cause (I'm still having an
out-of-box experience w skipfish, will catch up soonishly).

Original comment by n3npq....@gmail.com on 5 Jul 2010 at 3:09

[GoogleCodeExporter]

Actually, I messed up, sorry - was meant to be:

if (tag_end + 1 > res->payload + res->pay_len) {
  DEBUG("=== res->payload ===\n%s\n=== cur_str ===\n",res->payload, cur_str);
  FATAL("Bummer!");
}

Original comment by lcam...@google.com on 5 Jul 2010 at 3:20

[GoogleCodeExporter]

Bummer! ;-)

--18077-- REDIR: 0x6d2e70 (__mempcpy_ssse3) redirected to 0x4008250 (mempcpy)
^[[1;31m[-] PROGRAM ABORT : ^[[1;37mBummer!^[[1;31m
    Stop location : ^[[0;37mscrape_response(), analysis.c:844
^[[0mskipfish version 1.49b by <lcamtuf@google.com>

The hack-o-round is perhaps the easiest fixing. I'll try to dig out a better 
patch.

Original comment by n3npq....@gmail.com on 5 Jul 2010 at 3:26

[GoogleCodeExporter]

BTW, there's another valgrind error displayed accessing uninitialized memory in 
same file.

Here's the simple fixes:

@@ -1855,7 +1859,7 @@ binary_checks:
    ratproxy, with some improvements and additions. */

 static void detect_mime(struct http_request* req, struct http_response* res) {
-  u8 sniffbuf[SNIFF_LEN];
+  u8 sniffbuf[SNIFF_LEN] = "";

   if (res->sniff_mime_id) return;

@@ -2208,7 +2212,7 @@ static void detect_mime(struct http_requ
 static void check_for_stuff(struct http_request* req,
                             struct http_response* res) {

-  u8 sniffbuf[SNIFF_LEN];
+  u8 sniffbuf[SNIFF_LEN] = "";
   u8* tmp;

   if (!res->pay_len || !is_mostly_ascii(res) || res->stuff_checked) return;

Original comment by n3npq....@gmail.com on 5 Jul 2010 at 3:31

[GoogleCodeExporter]

Ok, two more things :-)

1) This line has a typo:

DEBUG("=== res->payload ===\n%s\n=== cur_str ===\n",res->payload, cur_str);

...should obviously be:

DEBUG("=== res->payload ===\n%s\n=== cur_str ===\n%s\n",res->payload, cur_str);

2) With this change made, can you paste or send me (lcamtuf@gmail.com) all the 
stderr output from "=== res->payload ===" (after doing 'make debug')? 

I suspect this is specific to the page you are scanning, as I don't hit this on 
my box with my test target, so this data would be very helpful.

Original comment by lcam...@google.com on 5 Jul 2010 at 3:34

[GoogleCodeExporter]

Original comment by lcam...@gmail.com on 5 Jul 2010 at 3:35

• Changed state: New

[GoogleCodeExporter]

[ That sniffbuf thing shouldn't be of concern, the buffer is not zeroed for 
performance reasons; although I will tweak it for the next release to move the 
NUL terminator to the end of the actual payload. ]

Original comment by lcam...@google.com on 5 Jul 2010 at 3:47

[GoogleCodeExporter]

re sniffbuf: yep lots of cpu cycles wasted with memset/calloc. but it should be 
just '\0'

hmm, __attribute__ for printf-like under DEBUG todo++.

crap, forgot to turn on debug ... hang on

Original comment by n3npq....@gmail.com on 5 Jul 2010 at 4:05

[GoogleCodeExporter]

Ok, that analysis.c thing should be fixed. Credited you in ChangeLog, thanks!

Original comment by lcam...@gmail.com on 5 Jul 2010 at 4:12

• Changed state: Fixed