Page MenuHomeWildfire Games

Fix miniupnpc leak in rP14370
ClosedPublic

Authored by elexis on Aug 16 2019, 5:16 PM.

Details

Summary

rP14332 introduced a leak that was fixed by rP14348 and (probably unintentionally reverted) few days later rP14370,
as discovered in D2181.

Valgrind shows this:

==15080== 148 bytes in 1 blocks are definitely lost in loss record 2,224 of 3,227
==15080==    at 0x483877F: malloc (vg_replace_malloc.c:299)
==15080==    by 0x630E7E5: ssdpDiscoverDevices (in /usr/lib/libminiupnpc.so.17)
==15080==    by 0x630973A: upnpDiscover (in /usr/lib/libminiupnpc.so.17)
==15080==    by 0x191253: CNetServerWorker::SetupUPnP(void*) (NetServer.cpp:252)
==15080==    by 0x667757E: start_thread (in /usr/lib/libpthread-2.29.so)
==15080==    by 0x678B0E2: clone (in /usr/lib/libc-2.29.so)


==15080== 36 bytes in 1 blocks are definitely lost in loss record 788 of 3,227
==15080==    at 0x483877F: malloc (vg_replace_malloc.c:299)
==15080==    by 0x671A36E: strdup (in /usr/lib/libc-2.29.so)
==15080==    by 0x63097EC: GetUPNPUrls (in /usr/lib/libminiupnpc.so.17)
==15080==    by 0x6309BE4: UPNP_GetValidIGD (in /usr/lib/libminiupnpc.so.17)
==15080==    by 0x191282: CNetServerWorker::SetupUPnP(void*) (NetServer.cpp:257)
==15080==    by 0x667757E: start_thread (in /usr/lib/libpthread-2.29.so)
==15080==    by 0x678B0E2: clone (in /usr/lib/libc-2.29.so)

=15080== 26 bytes in 1 blocks are definitely lost in loss record 243 of 3,227
==15080==    at 0x483877F: malloc (vg_replace_malloc.c:299)
==15080==    by 0x6308D5E: ??? (in /usr/lib/libminiupnpc.so.17)
==15080==    by 0x630980C: GetUPNPUrls (in /usr/lib/libminiupnpc.so.17)
==15080==    by 0x6309BE4: UPNP_GetValidIGD (in /usr/lib/libminiupnpc.so.17)
==15080==    by 0x191282: CNetServerWorker::SetupUPnP(void*) (NetServer.cpp:257)
==15080==    by 0x667757E: start_thread (in /usr/lib/libpthread-2.29.so)
==15080==    by 0x678B0E2: clone (in /usr/lib/libc-2.29.so)
==15080== 
==15080== 26 bytes in 1 blocks are definitely lost in loss record 244 of 3,227
==15080==    at 0x483877F: malloc (vg_replace_malloc.c:299)
==15080==    by 0x6308D5E: ??? (in /usr/lib/libminiupnpc.so.17)
==15080==    by 0x6309825: GetUPNPUrls (in /usr/lib/libminiupnpc.so.17)
==15080==    by 0x6309BE4: UPNP_GetValidIGD (in /usr/lib/libminiupnpc.so.17)
==15080==    by 0x191282: CNetServerWorker::SetupUPnP(void*) (NetServer.cpp:257)
==15080==    by 0x667757E: start_thread (in /usr/lib/libpthread-2.29.so)
==15080==    by 0x678B0E2: clone (in /usr/lib/libc-2.29.so)
==15080== 
==15080== 26 bytes in 1 blocks are definitely lost in loss record 245 of 3,227
==15080==    at 0x483877F: malloc (vg_replace_malloc.c:299)
==15080==    by 0x6308D5E: ??? (in /usr/lib/libminiupnpc.so.17)
==15080==    by 0x630983E: GetUPNPUrls (in /usr/lib/libminiupnpc.so.17)
==15080==    by 0x6309BE4: UPNP_GetValidIGD (in /usr/lib/libminiupnpc.so.17)
==15080==    by 0x191282: CNetServerWorker::SetupUPnP(void*) (NetServer.cpp:257)
==15080==    by 0x667757E: start_thread (in /usr/lib/libpthread-2.29.so)
==15080==    by 0x678B0E2: clone (in /usr/lib/libc-2.29.so)
==15080== 
==15080== 26 bytes in 1 blocks are definitely lost in loss record 246 of 3,227
==15080==    at 0x483877F: malloc (vg_replace_malloc.c:299)
==15080==    by 0x6308D5E: ??? (in /usr/lib/libminiupnpc.so.17)
==15080==    by 0x6309857: GetUPNPUrls (in /usr/lib/libminiupnpc.so.17)
==15080==    by 0x6309BE4: UPNP_GetValidIGD (in /usr/lib/libminiupnpc.so.17)
==15080==    by 0x191282: CNetServerWorker::SetupUPnP(void*) (NetServer.cpp:257)
==15080==    by 0x667757E: start_thread (in /usr/lib/libpthread-2.29.so)
==15080==    by 0x678B0E2: clone (in /usr/lib/libc-2.29.so)
==15080==

From IRC:

http://irclogs.wildfiregames.com/2013-12/2013-12-13-QuakeNet-%230ad-dev.log

23:41 <@historicbruno> will also have to fix the OS X build
23:41 < Josh> I think I messed up the build I did
23:42 < Josh> historicbruno: Thanks for sorting that out BTW
23:42 <@historicbruno> we don't call freeUPNPDevlist
23:44 < Josh> historicbruno: Are you fixing that already?
23:44 < Josh> Or should I commit a fix?
23:44 <@historicbruno> nope, just noticing it while reading the comments :)
23:44 <@historicbruno> also FreeUPNPUrls, and there may be more
23:46 < Josh> historicbruno: Alright. Our implementation is really just a modified version of bits and pieces of the demo/diagnostic in upnpc.c.
Test Plan

Use logic and reason to find any leaks.
Use valgrind to verify logic and reason: valgrind --leak-check=full ./pyrogenesis
Check that there aren't further structures that need to be removed.
Notice that theres much more TO DO here, but one step at a time.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Aug 16 2019, 5:16 PM
elexis retitled this revision from Fix miniupnpc leak in to Fix miniupnpc leak in rP14370.Aug 16 2019, 5:18 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/401/display/redirect

elexis updated this revision to Diff 9360.Aug 16 2019, 6:04 PM

Use lambda.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/403/display/redirect

elexis updated this revision to Diff 9361.Aug 16 2019, 6:14 PM

Reference and order

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/404/display/redirect

This revision was not accepted when it landed; it landed in state Needs Review.Aug 16 2019, 8:07 PM
This revision was automatically updated to reflect the committed changes.