Closed GoogleCodeExporter closed 9 years ago
What is the value of offset and *s ?
Original comment by adrian.c...@gmail.com
on 29 Jul 2009 at 4:19
gslin@rproxy-pic-large-1 [~] (12:59) sudo gdb /usr/local/squid/sbin/squid
GNU gdb 6.1.1 [FreeBSD]
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB. Type "show warranty" for details.
This GDB was configured as "amd64-marcel-freebsd"...
(gdb) run -NsY
Starting program: /usr/local/squid/sbin/squid -NsY
[New LWP 100166]
[New Thread 0x800f020b0 (LWP 100166)]
[New Thread 0x800f023d0 (LWP 100185)]
[New Thread 0x800f02560 (LWP 100186)]
[New Thread 0x800f026f0 (LWP 100187)]
[New Thread 0x800f02880 (LWP 100188)]
[New Thread 0x800f02a10 (LWP 100189)]
[New Thread 0x800f02ba0 (LWP 100190)]
[New Thread 0x800f02d30 (LWP 100191)]
[New Thread 0x800f02ec0 (LWP 100192)]
[New Thread 0x800f03050 (LWP 100193)]
[New Thread 0x800f031e0 (LWP 100194)]
[New Thread 0x800f03370 (LWP 100195)]
[New Thread 0x800f03500 (LWP 100196)]
[New Thread 0x800f03690 (LWP 100197)]
[New Thread 0x800f03820 (LWP 100198)]
[New Thread 0x800f039b0 (LWP 100199)]
[New Thread 0x800f03b40 (LWP 100200)]
Program received signal SIGABRT, Aborted.
[Switching to Thread 0x800f020b0 (LWP 100166)]
0x0000000800c8ba9c in thr_kill () from /lib/libc.so.7
(gdb) bt
#0 0x0000000800c8ba9c in thr_kill () from /lib/libc.so.7
#1 0x0000000800d1affb in abort () from /lib/libc.so.7
#2 0x000000000046d4fd in xassert (msg=Variable "msg" is not available.
) at debug.c:270
#3 0x000000000046ea7f in strCut (s=0x802ff9550, offset=14) at String.c:198
#4 0x000000000041fa1c in clientFollowXForwardedForDone (answer=Variable
"answer" is
not available.
) at client_side.c:294
#5 0x0000000000406ce5 in aclCheckCallback (checklist=0x807355698,
answer=ACCESS_ALLOWED) at acl.c:2338
#6 0x000000000042062a in clientTryParseRequest (conn=0x807355158) at
client_side.c:4304
#7 0x0000000000421285 in clientReadRequest (fd=22, data=Variable "data" is not
available.
) at client_side.c:4428
#8 0x000000000047c256 in comm_select (msec=Variable "msec" is not available.
) at comm_generic.c:264
#9 0x000000000043e59b in main (argc=Variable "argc" is not available.
) at main.c:933
(gdb) up
#1 0x0000000800d1affb in abort () from /lib/libc.so.7
(gdb) up
#2 0x000000000046d4fd in xassert (msg=Variable "msg" is not available.
) at debug.c:270
270 abort();
(gdb) up
#3 0x000000000046ea7f in strCut (s=0x802ff9550, offset=14) at String.c:198
198 assert(offset < strLen(*s));
(gdb) p *s
$1 = {size = 36, len = 14, buf = 0x8030525e0 "61.228.130.130"}
(gdb) p offset
$2 = 14
(gdb) p
$3 = 14
(gdb) up
#4 0x000000000041fa1c in clientFollowXForwardedForDone (answer=Variable
"answer" is
not available.
) at client_side.c:294
294 strCut(&request->x_forwarded_for_iterator, l);
(gdb) p l
$4 = 14
(gdb) p request->x_forwarded_for_iterator
$5 = {size = 36, len = 14, buf = 0x8030525e0 "61.228.130.130"}
(gdb) up
#5 0x0000000000406ce5 in aclCheckCallback (checklist=0x807355698,
answer=ACCESS_ALLOWED) at acl.c:2338
2338 checklist->callback(answer, checklist->callback_data);
(gdb) p answer
$6 = ACCESS_ALLOWED
(gdb) p checklist->callback_data
$7 = (void *) 0x802f45818
(gdb) up
#6 0x000000000042062a in clientTryParseRequest (conn=0x807355158) at
client_side.c:4304
4304 clientCheckFollowXForwardedFor(http);
(gdb) p http
$8 = (clientHttpRequest *) 0x802f45818
(gdb) The program is running. Exit anyway? (y or n) y
Original comment by darkkiller
on 29 Jul 2009 at 5:02
Right. This is due to the changes I made to the string API a while ago.
strCut() was
a #define which just added the NUL character where needed without any bounds
checking.
This particular use of strCut() wants to cut the string -after- the string
length -
which "works" for C strings because the string length doesn't include the
trailing
NUL. In this case, the trailing NUL is just overwritten. The interesting issue
is the
string length being set incorrectly - it now includes the trailing NUL when it
shouldn't.
Anyway. The short short term fix is to just return from strCut() if l >=
length. The
long(er) term fix (which is what I'll commit) is to audit the three places
strCut()
is called to make sure its called correctly; then document the String functions
a bit
better. :)
Thanks for this!
Original comment by adrian.c...@gmail.com
on 29 Jul 2009 at 7:13
Thanks, I'll try to change assert() code to workaround it.
Original comment by darkkiller
on 29 Jul 2009 at 10:23
Try patching libmem/String.c with this:
Index: String.c
===================================================================
--- String.c (revision 14244)
+++ String.c (working copy)
@@ -192,9 +192,23 @@
return -1;
}
+/*
+ * Cut the given string at the given offset.
+ * "offset" -should- be less than the length of the string but
+ * at least the client_side X-Forwarded-For code currently (ab)uses
+ * the API and passes in an out of bounds iterator. In this case,
+ * don't cut the string.
+ */
extern void
strCut(String *s, int offset)
{
+ /*
+ * XXX this should eventually be removed and all code
+ * XXX which triggers it should be fixed!
+ */
+ if (offset >= strLen(*s))
+ return;
+
assert(offset < strLen(*s));
s->buf[offset] = '\0';
s->len = offset;
Original comment by adrian.c...@gmail.com
on 29 Jul 2009 at 2:55
Yes, I had patched when I replied, and it works fine.
Original comment by darkkiller
on 29 Jul 2009 at 5:11
Fixed in revision r14254.
Original comment by adrian.c...@gmail.com
on 30 Jul 2009 at 2:59
Original issue reported on code.google.com by
darkkiller
on 28 Jul 2009 at 7:05