UPnP segfault #156

Reproduction steps: When UPnP port mapping is enabled, open settings, and attempt to uncheck UPnP port mapping.

Expected behavior: Checkbox unchecks.

Observed behavior: Segfault.

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xb3c0cb90 (LWP 23682)]
0xb70c67e2 in free () from /lib/i686/cmov/libc.so.6
(gdb) bt
[#0](/bitcoin-bitcoin/0/)  0xb70c67e2 in free () from /lib/i686/cmov/libc.so.6
[#1](/bitcoin-bitcoin/1/)  0x0837b96b in FreeUPNPUrls ()
[#2](/bitcoin-bitcoin/2/)  0x08096a59 in ThreadMapPort2 (parg=0x0) at net.cpp:940
[#3](/bitcoin-bitcoin/3/)  0x08096bff in ThreadMapPort (parg=0x0) at net.cpp:877
[#4](/bitcoin-bitcoin/4/)  0xb71b74c0 in start_thread () from /lib/i686/cmov/libpthread.so.0
[#5](/bitcoin-bitcoin/5/)  0xb71366de in clone () from /lib/i686/cmov/libc.so.6

I'm not sure why there aren't full symbols. I'm using the standard makefile with the -g option.

Note that I've also seen a segfault when originally enabling UPnP port mapping after clicking the checkbox and hitting okay. I can't reproduce it now because I'm unable to disable port mapping due to the segfault above.

System/build information: git checkout as of b37f09aa2e80b17028ad7fe1e87362c0f07c7406 Debian 5.0 Linux 2.6.26-1-686 #1 SMP Sat Jan 10 18:29:31 UTC 2009 i686 GNU/Linux gcc 4.3.2 miniupnpc 1.5

Apparently attempting to check the checkbox is unnecessary. All that I need to do is open the settings when UPnP is already enabled, wait a moment, and then get the segfault above.

It keeps getting better and better.. All I need to do now is leave the bitcoin client running on the main screen without even opening the settings, and it segfaults as above within a minute.

net.cpp:940 gets called when no router/UPnP port mapping device has been found. I don't know what the UI could have to do with this. Is it possible the box (un)checking timing lining up just happened to be a coincidence? Also, do you have UPnP on on your router when this happens?

Yup, I think clicking the UI checkbox has nothing to do with it, since the segfault occurs if the client just sits there for a while doing nothing (see my comment above).

UPnP is enabled on my router, but no port forwards are enabled.

Hm, I can't reproduce. What router are you using (do you happen to know what UPnP library it is using?) Does the miniupnpc example client work (it comes with miniupnpc, upnpc_static/shared, try it with -a your internal IPv4 8333 8333 tcp)

My Linksys WRT54GL router is running dd-wrt. I'm not sure what UPnP library it's using. Interestingly, upnpc doesn't appear to find the router:

$ ./upnpc-static -a 10.0.0.2 8333 8333 tcp
upnpc : miniupnpc library test client. (c) 2006-2010 Thomas Bernard
Go to http://miniupnp.free.fr/ or http://miniupnp.tuxfamily.org/
for more information.
No IGD UPnP Device found on the network !

But then I disabled my local firewall, and I get this:

$ ./upnpc-static -a 10.0.0.2 8333 8333 tcp
upnpc : miniupnpc library test client. (c) 2006-2010 Thomas Bernard
Go to http://miniupnp.free.fr/ or http://miniupnp.tuxfamily.org/
for more information.
List of UPNP devices found on the network :
 desc: http://10.0.0.1:5431/dyndev/uuid:0022-6b8c-XX
 st: urn:schemas-upnp-org:device:InternetGatewayDevice:1

Found valid IGD : http://10.0.0.1:5431/uuid:0022-6b8c-XX/WANIPConnection:1
Local LAN ip address : 10.0.0.2
ExternalIPAddress = XX.YY.ZZ.114
InternalIP:Port = 10.0.0.2:8333
external XX.YY.ZZ.114:8333 TCP is redirected to internal 10.0.0.2:8333

(IPs masked.) So then I tried bitcoin again.. No crash, even after leaving it running for a while! I then stopped bitcoin, re-enabled my firewall, and restarted bitcoin. Sure enough, bitcoin crashed quite reliably within a few seconds.

So the segfault appears to occur when using UPnP with a firewall that prevents UPnP packets from being sent.

The firewall configuration is a pretty standard iptables setup. Outgoing-initiated connections are allowed. Most incoming packets are dropped. Packets for existing connections are allowed.

Hm, I still can't seem to reproduce this issue. Can you narrow it down to one individual or set of iptables rules?

If I add a rule to just drop all input packets, and then I start bitcoin, that will trigger the segfault:

$ sudo iptables -F
$ sudo iptables -A INPUT -j DROP
$ sudo iptables -L -v
Chain INPUT (policy ACCEPT 1350 packets, 1561K bytes)
 pkts bytes target     prot opt in     out     source               destination      
   69  4595 DROP       all  --  any    any     anywhere             anywhere         

Chain FORWARD (policy ACCEPT 0 packets, 0 bytes)
 pkts bytes target     prot opt in     out     source               destination      

Chain OUTPUT (policy ACCEPT 1134 packets, 224K bytes)
 pkts bytes target     prot opt in     out     source               destination      

Chain fail2ban-ssh (0 references)
 pkts bytes target     prot opt in     out     source               destination      

And then if I remove the drop rule and start bitcoin, no segfault.

Still cannot reproduce, maybe someone else can? Do you still get the segfault if you comment out net.cpp:940 (FreeUPNPUrls(&urls), probably creating a memleak)?

Yes, commenting out that line does prevent the segfault from occurring.

I updated to commit bf3a0902ef98365d803e4a03853dbf0f83511026 and the segfault still occurs.

I think I found the problem. At net.cpp:909, UPNP_GetValidIGD() is called to, among other things, initialize the urls parameter. The documentation for miniupnpc.c's UPNP_GetValidIGD() is as follows:

/* UPNP_GetValidIGD() :
 * return values :
 *     0 = NO IGD found
 *     1 = A valid connected IGD has been found
 *     2 = A valid IGD has been found but it reported as
 *         not connected
 *     3 = an UPnP device has been found but was not recognized as an IGD
 *
 * In any non zero return case, the urls and data structures
 * passed as parameters are set. Donc forget to call FreeUPNPUrls(urls) to
 * free allocated memory.
 */

To me, that suggests that we should only call FreeUPNPUrls(urls) if the UPNP_GetValidIGD() return value is non-zero. However at net.cpp:940, we're calling FreeUPNPUrls(urls) if UPNP_GetValidIGD() returns any value other than 1. And I confirmed that on my machine, with the firewall on, the return value is zero.

Because urls is uninitialized at the time FreeUPNPUrls(urls) is called, it's probably trying to free garbage pointers and thus causing a segfault.

Looks good to me, added a pull request for that and one other small thing at #165

I think this bug may have returned -- at least, as of commit 93193c8ff, I'm getting a new seg fault on line 1164 of src/net.cpp, inside this call:

FreeUPNPUrls(&urls);

In GDB, the value pointed to by urls as of the seg fault is:

{controlURL = 0x0, ipcondescURL = 0x0, controlURL_CIF = 0x0, controlURL_6FC = 0x0}

Possibly they're just all NULL because they've already been correctly free()'d by that point? Or, the problem is that they're all NULL when free()'ing is attempted -- this seems more likely, from what I can see so far. According to the docs for UPNP_GetValidIGD(), which filled in urls, if the function's return value is non-zero, then FreeUPNPUrls() should be called on urls later. Well, we're in the case where the return value is 1 (see r ==1 conditional earlier in the code), but FreeUPNPUrls() is definitely not happy, because it is calling free() on an invalid pointer.

Unfortunately I don't have the libminiupnpc sources at hand, otherwise I'd dig in further, but FWIW the stack trace is:

[#0](/bitcoin-bitcoin/0/)  0x00007ffff3827077 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
[#1](/bitcoin-bitcoin/1/)  0x00007ffff3828458 in __GI_abort () at abort.c:89
[#2](/bitcoin-bitcoin/2/)  0x00007ffff3864fb4 in __libc_message (do_abort=do_abort@entry=1, fmt=fmt@entry=0x7ffff3957bc0 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/posix/libc_fatal.c:175
[#3](/bitcoin-bitcoin/3/)  0x00007ffff386a78e in malloc_printerr (action=1, str=0x7ffff3953c7e "free(): invalid pointer", ptr=<optimized out>) at malloc.c:4996
[#4](/bitcoin-bitcoin/4/)  0x00007ffff386b496 in _int_free (av=<optimized out>, p=<optimized out>, have_lock=0) at malloc.c:3840
[#5](/bitcoin-bitcoin/5/)  0x00007ffff45c5df4 in FreeUPNPUrls () from /usr/lib/x86_64-linux-gnu/libminiupnpc.so.10
[#6](/bitcoin-bitcoin/6/)  0x0000555555740a7d in ThreadMapPort () at net.cpp:1164
[#7](/bitcoin-bitcoin/7/)  0x000055555574f1a0 in TraceThread<void (*)()> (name=0x555555903278 "upnp", func=0x555555740660 <ThreadMapPort()>) at util.h:207
[#8](/bitcoin-bitcoin/8/)  0x00007ffff75455ba in ?? () from /usr/lib/x86_64-linux-gnu/libboost_thread.so.1.55.0
[#9](/bitcoin-bitcoin/9/)  0x00007ffff52140a4 in start_thread (arg=0x7fff99ee2700) at pthread_create.c:309
[#10](/bitcoin-bitcoin/10/) 0x00007ffff38d7c2d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111

One possible clue:

Earlier in net.cpp:ThreadMapPort(), devlist is allocated in this call:

#ifndef UPNPDISCOVER_SUCCESS
    /* miniupnpc 1.5 */
    devlist = upnpDiscover(2000, multicastif, minissdpdpath, 0);
#else
    /* miniupnpc 1.6 */
    int error = 0;
    devlist = upnpDiscover(2000, multicastif, minissdpdpath, 0, 0, &error);
#endif

My system (Debian GNU/Linux testing distro) appears to have libminiupnpc 1.9, which corresponds to the inaccurately-labeled "miniupnpc 1.6" case above. In that case, an error pointer is passed, but is never checked. Is it possible that if something goes wrong with upnpDiscover(), then later when we pass the resultant devlist pointer as the first argument to UPNP_GetValidIGD(), there is something wrong with devlist and thus with the results of UPNP_GetValidIGD()?

I don't have time to debug further right now. I'll try to look more into it, as soon as I get a chance to build latest libminiupnpc from source and rebuild bitcoin (with full debugging info) against it. In the meantime, I hope this report helps.

-Karl

Okay, bug goes away with latest libminiupnpc-dev. See #4861 (comment) for details.

Thanks for reporting this and figuring this out. At least when someone else stumbles on the problem they'll have a place to look.