Skip to content

Commit

Permalink
Use local variable to stop memory leak.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
alesliehughes committed Apr 4, 2024
1 parent 79de2ea commit 767bfec
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 23 deletions.
4 changes: 2 additions & 2 deletions modules/upnp/doc_classes/UPNPDevice.xml
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
<constant name="IGD_STATUS_HTTP_EMPTY" value="2" enum="IGDStatus">
Empty HTTP response.
</constant>
<constant name="IGD_STATUS_NO_URLS" value="3" enum="IGDStatus">
<constant name="IGD_STATUS_NO_URLS" value="3" enum="IGDStatus" deprecated="This value is no longer used.">
Returned response contained no URLs.
</constant>
<constant name="IGD_STATUS_NO_IGD" value="4" enum="IGDStatus">
Expand All @@ -86,7 +86,7 @@
<constant name="IGD_STATUS_INVALID_CONTROL" value="7" enum="IGDStatus">
Invalid control.
</constant>
<constant name="IGD_STATUS_MALLOC_ERROR" value="8" enum="IGDStatus">
<constant name="IGD_STATUS_MALLOC_ERROR" value="8" enum="IGDStatus" deprecated="This value is no longer used.">
Memory allocation error.
</constant>
<constant name="IGD_STATUS_UNKNOWN_ERROR" value="9" enum="IGDStatus">
Expand Down
29 changes: 8 additions & 21 deletions modules/upnp/upnp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -121,33 +121,20 @@ void UPNP::parse_igd(Ref<UPNPDevice> dev, UPNPDev *devlist) {
return;
}

struct UPNPUrls *urls = (UPNPUrls *)malloc(sizeof(struct UPNPUrls));

if (!urls) {
dev->set_igd_status(UPNPDevice::IGD_STATUS_MALLOC_ERROR);
return;
}

struct UPNPUrls urls = {};
struct IGDdatas data;

memset(urls, 0, sizeof(struct UPNPUrls));

parserootdesc(xml, size, &data);
free(xml);
xml = nullptr;

GetUPNPUrls(urls, &data, dev->get_description_url().utf8().get_data(), 0);

if (!urls) {
dev->set_igd_status(UPNPDevice::IGD_STATUS_NO_URLS);
return;
}
GetUPNPUrls(&urls, &data, dev->get_description_url().utf8().get_data(), 0);

char addr[16];
int i = UPNP_GetValidIGD(devlist, urls, &data, (char *)&addr, 16);
int i = UPNP_GetValidIGD(devlist, &urls, &data, (char *)&addr, 16);

if (i != 1) {
FreeUPNPUrls(urls);
FreeUPNPUrls(&urls);

switch (i) {
case 0:
Expand All @@ -165,18 +152,18 @@ void UPNP::parse_igd(Ref<UPNPDevice> dev, UPNPDev *devlist) {
}
}

if (urls->controlURL[0] == '\0') {
FreeUPNPUrls(urls);
if (urls.controlURL[0] == '\0') {
FreeUPNPUrls(&urls);
dev->set_igd_status(UPNPDevice::IGD_STATUS_INVALID_CONTROL);
return;
}

dev->set_igd_control_url(urls->controlURL);
dev->set_igd_control_url(urls.controlURL);
dev->set_igd_service_type(data.first.servicetype);
dev->set_igd_our_addr(addr);
dev->set_igd_status(UPNPDevice::IGD_STATUS_OK);

FreeUPNPUrls(urls);
FreeUPNPUrls(&urls);
}

int UPNP::upnp_result(int in) {
Expand Down

0 comments on commit 767bfec

Please sign in to comment.