UPNP: Use local variable for UPNPUrls to stop memory leak. #89897
Conversation
alesliehughes
force-pushed
the
upnp_memory_leak
branch
from
caf499f
to
4858de4
Compare
akien-mga
changed the title
akien-mga
changed the title
Ouch, reading the upstream source, you are right. Seems whichever source I used as inspiration back when implementing this also got that part wrong. I built this locally and found no issues during admittedly brief testing and the changes look good to me. I'm somewhat on the move this week but will try to do some more testing; in the mean time, this has my approval (with the above caveats). Thanks for investigating and fixing issues you found :) |
| if (!urls) { | ||
| dev->set_igd_status(UPNPDevice::IGD_STATUS_NO_URLS); | ||
| return; | ||
| } |
There was a problem hiding this comment.
With the removal of this check, and of the previous one, IGD_STATUS_MALLOC_ERROR and IGD_STATUS_NO_URL are unused, and could be unexposed.
Unexposing them would break the API compatibility, but they can at least be flagged as deprecated and their documentation changed to clarify that they're never used.
alesliehughes
force-pushed
the
upnp_memory_leak
branch
from
4858de4
to
f88dddd
Compare
I've change the urls variable to be a local, instead of manually allocating it all the time. This is only used locally and was causing a memory leak because FreeUPNPUrls gave the impression it free it. 1. FreeUPNPUrls doesn't free the pointer passed in, that's up to caller. 2. The second if(!urls) produced dead code as we checked the pointer just after allocation.
alesliehughes
force-pushed
the
upnp_memory_leak
branch
from
f88dddd
to
767bfec
Compare
|
Thanks! |
I've change the urls variable to be a local, instead of manually allocating it all the time. This is only used locally and was causing a memory leak because FreeUPNPUrls gave the impression it free it but didn't.