When the following environment set, crash occured. * mod_jk 1.2.39 or latter * IPv6 * JkLogLevel debug I think char buf[64] is too small. ---mod_jk.log [Tue Apr 22 12:30:44.350 2014] [39832:140226284476384] [debug] jk_open_socket::jk_connect.c (766): socket 14 [2001:c0a8:1424:0:a00:1f49:::59892 -> 2001:c0a8:6422:0:3a35:3938:3932:0:8009] connected --- --- $ gdb /usr/sbin/httpd core.39832 : (gdb) thread apply all backtrace : #5 0x00007f88ef48a587 in jk_open_socket (addr=<value optimized out>, keepalive=0, timeout=5, connect_timeout=5000, sock_buf=<value optimized out>, l=0x7f88fbe832b8) at jk_connect.c:771 #6 0x00007f88ef4a9055 in ajp_connect_to_endpoint (ae=0x7f88fbedb4a0, l=0x7f88fbe832b8) at jk_ajp_common.c:1011 #7 0x00007f88ef4a94e9 in ajp_send_request (e=<value optimized out>, s=0x7fff4a6122d0, l=0x7f88fbe832b8, ae=0x7f88fbedb4a0, op=0x7fff4a611090) at jk_ajp_common.c:1662 : (gdb) f 5 #5 0x00007f88ef48a587 in jk_open_socket (addr=<value optimized out>, keepalive=0, timeout=5, connect_timeout=5000, sock_buf=<value optimized out>, l=0x7f88fbe832b8) at jk_connect.c:771 warning: Source file is more recent than executable. 771 } (gdb) l 766 jk_log(l, JK_LOG_DEBUG, "socket %d [%s] connected", 767 sd, jk_dump_sinfo(sd, buf)); 768 } : --- ---jk_connect.c jk_sock_t jk_open_socket(jk_sockaddr_t *addr, int keepalive, int timeout, int connect_timeout, int sock_buf, jk_logger_t *l) { char buf[64];
Agreed. The use of 'buf' when passed-into inet_ntop4|6 inside jk_dump_sinfo is also not sane: the final argument should be the length of the buffer available. It is blindly set to 16 (for IPv4) or 64 (for IPv6) instead of sending the real value which will be (64 - ps). Unfortunately, jk_dump_sinfo has no way of knowing the size of the buffer being passed to it. That could easily be fixed, as jk_dump_sinfo is only used internally to jk_connect.c (I don't know if anyone bothers using mod_jk as a library for anything else). I'm not increasing the severity of this bug to MAJOR because while it's a buffer-overrun crash, it only happens when in DEBUG mode.
Created attachment 31557 [details] Proposed patch (against mod_jk/trunk) I have raised the size of buf from 64 to 100 characters. The message looks like it will need 96 bytes for a complete IPv6 message (e.g. 2001:0db8:0000:0000:0000:ff00:0042:8329:65556 -> 2001:0db8:0000:0000:0000:ff00:0042:8329:65535 + \0), so I just rounded up to 100 bytes. I've also added proper length-tracking to the buffer and more protection against overruns.
(In reply to Christopher Schultz from comment #2) > Created attachment 31557 [details] > Proposed patch (against mod_jk/trunk) > > I have raised the size of buf from 64 to 100 characters. The message looks > like it will need 96 bytes for a complete IPv6 message (e.g. > 2001:0db8:0000:0000:0000:ff00:0042:8329:65556 -> > 2001:0db8:0000:0000:0000:ff00:0042:8329:65535 + \0), so I just rounded up to > 100 bytes. > > I've also added proper length-tracking to the buffer and more protection > against overruns. The following discussion says that IPv6 address needs maximum 45 characters, not 39. So you would need 12 bytes more than 96 = 108. http://stackoverflow.com/questions/166132/maximum-length-of-the-textual-representation-of-an-ipv6-address Also see tmp buffer size here: http://svn.apache.org/viewvc/apr/apr/trunk/network_io/unix/inet_ntop.c?revision=573491&view=markup#l149
(In reply to Konstantin Kolinko from comment #3) > (In reply to Christopher Schultz from comment #2) > > Created attachment 31557 [details] > > Proposed patch (against mod_jk/trunk) > > > > I have raised the size of buf from 64 to 100 characters. The message looks > > like it will need 96 bytes for a complete IPv6 message (e.g. > > 2001:0db8:0000:0000:0000:ff00:0042:8329:65556 -> > > 2001:0db8:0000:0000:0000:ff00:0042:8329:65535 + \0), so I just rounded up to > > 100 bytes. > > > > I've also added proper length-tracking to the buffer and more protection > > against overruns. > > The following discussion says that IPv6 address needs maximum 45 characters, > not 39. So you would need 12 bytes more than 96 = 108. Bah, I forgot about tunnelled-IPv4. However: "0000:0000:0000:0000:0000:0000:192.168.0.1:65535 -> 0000:0000:0000:0000:0000:0000:192.168.0.1:65535" That's only 97 bytes plus a NULL-terminator. Where did you get the 39 from? > http://stackoverflow.com/questions/166132/maximum-length-of-the-textual- > representation-of-an-ipv6-address > > Also see tmp buffer size here: > http://svn.apache.org/viewvc/apr/apr/trunk/network_io/unix/inet_ntop. > c?revision=573491&view=markup#l149 I did see that code in APR while researching. I suppose I could use that, but I figured that "buf" might be used for other things so I didn't consider it. I think it turns out that "buf" is really just for this purpose, so we could do something like this if you'd prefer: char buf[sizeof "0000:0000:0000:0000:0000:0000:192.168.0.1:65535 -> 0000:0000:0000:0000:0000:0000:192.168.0.1:65535" + 1];
(In reply to Christopher Schultz from comment #4) > That's only 97 bytes plus a NULL-terminator. Where did you get the 39 from? 39 is the size of IPv6 address in the form used in your example. You have two IPv6 addresses in that string. 45-39 = 6 characters missing x 2 IPv6 addresses = 12 more characters are needed in the buffer. > we > could do something like this if you'd prefer: > > char buf[sizeof "0000:0000:0000:0000:0000:0000:192.168.0.1:65535 -> > 0000:0000:0000:0000:0000:0000:192.168.0.1:65535" + 1]; First, the above code has an error. The IPv4 address part can be longer by 4 decimal digits than the one written above. Second, I do not care on the actual implementation. I mentioned APR code as an example of their output. The stackoverflow article mentioned INET6_ADDRSTRLEN constant, but I do not know whether that is portable.
(In reply to Konstantin Kolinko from comment #5) > First, the above code has an error. The IPv4 address part can be longer by 4 > decimal digits than the one written above. Aah, yes. Thanks for pointing that out. > Second, I do not care on the actual implementation. I mentioned APR code as > an example of their output. > > The stackoverflow article mentioned INET6_ADDRSTRLEN constant, but I do not > know whether that is portable. On Linux, I can see them here: $ grep 'INET\(6\)\?_ADDRSTRLEN' `find /usr/include -name "*.h"` /usr/include/netinet/in.h:#define INET_ADDRSTRLEN 16 /usr/include/netinet/in.h:#define INET6_ADDRSTRLEN 46 Mac OS X has them in comparable locations: $ grep 'INET\(6\)\?_ADDRSTRLEN' `find /usr/include -name "*.h"` /usr/include/netinet/in.h:#define INET_ADDRSTRLEN 16 /usr/include/netinet6/in6.h:#define INET6_ADDRSTRLEN 46 The Linux mac page for inet_ntop mentions both INET_ADDRSTRLEN and INET6_ADDRSTRLEN, and the "COMFORMING TO" section says POSIX.1-2001 with no caveats about those constants. If we like POSIX.1-2001 then I think we're all set. I like the use of INET_ADDRSTRLEN better than the sizeof thing for some reason, so I'll use that. Anyone with a problem can file a bug; it's easy to fix if someone has an old system. I'll update the patch.
Chris: any chance you've got the updated patch lying around somewhere? Or can do it during the next few days?
I committed a very similar patch in r1648948. It would be very nice if you could check it, especially whether I got the math for the buffer size correct. Thanks!