memory leak in upnpapi.c ?

[philippe44]

Since I'm moved to recent 1.14 (I used to be on very old branch 1.6.19), I've observed a big memory leak at every action sent. Now, I'm very confused by this code in upnpapi.c

    case ACTION: {
        UpnpActionComplete *Evt = UpnpActionComplete_new();
        IXML_Document *actionResult = NULL;
        int errCode = SoapSendAction(Param->Url,
            Param->ServiceType,
            Param->Act,
            &actionResult);
        UpnpActionComplete_set_ErrCode(Evt, errCode);
        UpnpActionComplete_set_ActionRequest(Evt, Param->Act);
        UpnpActionComplete_set_ActionResult(Evt, actionResult);
        UpnpActionComplete_strcpy_CtrlUrl(Evt, Param->Url);
        Param->Fun(UPNP_CONTROL_ACTION_COMPLETE, Evt, Param->Cookie);
        free(Param);
        UpnpActionComplete_delete(Evt);
        break;
    }

It used to be, in previous version

    case ACTION: {
        struct Upnp_Action_Complete Evt;
        memset(&Evt, 0, sizeof(Evt));
        Evt.ActionResult = NULL;
        Evt.ErrCode = SoapSendAction(
            Param->Url,
            Param->ServiceType,
            Param->Act, &Evt.ActionResult);
        Evt.ActionRequest = Param->Act;
        strncpy(Evt.CtrlUrl, Param->Url, sizeof(Evt.CtrlUrl) - 1);
        Param->Fun(UPNP_CONTROL_ACTION_COMPLETE, &Evt, Param->Cookie);
        ixmlDocument_free(Evt.ActionRequest);
        ixmlDocument_free(Evt.ActionResult);
        free(Param);
        break;
    }

Now, I've checked the examples and neither the request noe the action are freed by the code, who receives the ACTION_COMPLETE, so it does not seem to be its duty. Obviously then this memory leaks as nobody releases it. Am I missing something obvious?

Also, here is the generated code for the ActionComplete_delete, which obviously does not free Result or Request. I don't know how it is supposed to be generated, could not find a rule in configure/makefiles

void UpnpActionComplete_delete(UpnpActionComplete *q)
{
    struct s_UpnpActionComplete *p = (struct s_UpnpActionComplete *)q;

    if (!p)
        return;

    p->m_ActionResult = 0;
    p->m_ActionRequest = 0;
    UpnpString_delete(p->m_CtrlUrl);
    p->m_CtrlUrl = 0;
    p->m_ErrCode = 0;

    free(p);
}

And my configure line is

 ./configure --disable-blocking-tcp_connections --enable-static --disable-shared --disable-samples --disable-ipv6

[ingo-h]

Just with a short glance: the ACTION case is called in ´upnpapi.c´ by two functions: UpnpSendActionAsync() and UpnpSendActionExAsync(). Both functions have an argument Upnp_FunPtr Fun that is documented in upnp.h with

/! [in] Pointer to a callback function to be invoked when the operation completes. /

Is it possible that this callback function frees the action?

[philippe44]

It can definitely but it was not the case in 1.6.x and more important it's not what the sample code for tv control point does. So it feels like something is wrong with the generated code, although the generation is not part of the build.

[ingo-h]

Sorry for asking but I try to understand it.

it's not what the sample code for tv control point does.

What does it do and where? What should it do?

So it feels like something is wrong with the generated code, although the generation is not part of the build.

What is the generated code? The library? And why isn't it part of the build? What build? The library? The sample code? The generated code isn't part of one of them?

[philippe44]

No worries. If you look at the UPnP sample code, the TV control point, the event handler when receiving an ACTION_COMPLETE, simply prints some information, but does not free anything. If it were the responsibility of user code to free action results and requests documents, I would assume the exemple would do.

If you look at api/UPnPActionComplete.c which contains the UpnpActionComplete_delete function that is the cleanup code called in upnpapi.c (see my first post), the header of that file says it is generated code, and it says look for generator.c. When you configure and compile the UPnP library, there is no rule to build UPnPactioncomplete.c

[ingo-h]

Ah, the code generator. Maybe @mrjimenez can enlighten it?

[mrjimenez]

Hi @ingo-h ,

Part of the API code is automatically generated by upnp/generator/generator.c. This file takes care of generating the boiler plate code. I don't think that this is the reason of the leak.

@philippe44 , you might have found a leak, and indeed, this is a very cumbersome code. The relevant code parts are the following (e.g. in UpnpSubscribeAsync())

    TPJobInit(&job, (start_routine)UpnpThreadDistribution, Param);
    TPJobSetFreeFunction(&job, (free_routine)free);
    TPJobSetPriority(&job, MED_PRIORITY);

Notice that UpnpThreadDistribution is a thread, and Param is malloc'ed here. Since we don't actually know when the thread will be finished, the following line sets the routine that will free Param. In simple cases, this is just free(), but there might be some more complicated cases like free_upnp_timeout() or free_notify_struct().

If you are wiling to follow the rabbit down that hole, you can probably find the corresponding TPJobInit() that set that thread to run and the TPJobSetFreeFunction() that follows it. Since this parameter seems to never be NULL, the possible explanation is that one or more of these functions are not freeing everything they must:

• free_upnp_timeout()
• free_notify_struct()
• free_handle_request_arg()
• free_ssdp_event_handler_data()
• Or even just free() in case there should be more stuff than just the main pointer to be free'd.

Regards, Marcelo.

[philippe44]

Hi Marcelo - I'm a bit confused by your answer because that leak seems to be simply that the code that frees the action Request & Result is missing. It was in 1.6, as you can see in my first post.

Param which is indeed allocated when the job is submitted is freed in the ACTION response, as expected. That code is in UpnpThreadDistribution (in upnpapi.c) which used to explicitly free the action request & result, but now does not anymore as it seems to delegate that to the cleanup code UpnpActionComplete_delete which is generated but misses the free items.

So we should either change the generated code or move back to explicitly free items. I just don't know how and where generator.c creates UpnpActionComplete.c

[mrjimenez]

Hi Philippe,

From 1.8 on, the lib got radically different. Action Request and Result are suposed to be "weak" pointers (I know, this is C, but lets stick to the concept), so it is the responsibility of whoever alloc'd them to free them. Evt->ActionRequest is a copy of Param->Act:

UpnpActionComplete_set_ActionRequest(Evt, Param->Act);

Param->Act has been malloc'd when Param was malloc'd since it is one of its members, look at UpnpNonblockParam in upnpapi.h. So it is free'd when Param is freed.

On the other hand, you seem to be correct in the case of ActionResult, since it seems to be alloc'd somewhere inside SoapSendAction(). In that case, a free(actionResult) is missing just before free(Param).

In fact, free(Param) itself is very suspicious, since it will probably incurr in a double free, since freeing Param is the job of the FreeFunction

To sum up, there seems to be two problems:

1. actionResult is leaking;
2. Param will be double freed.

Please, feel free to disagree, I am trying to make sense of the code too. :smile:

Regards, Marcelo.

[philippe44]

For 2. I've never seen a double free and I make a very intensive use of libupnp (at least as control point). Intensive means 10+ clients sending Actions every second or less. That's why the leak is catastrophic for me, but I'd think I'd have seen a double free. Now, it seems to me that the "free" function pointer to free the arg passed at job's creation is only used when the thread is shutdown, not otherwise. So I'd say that free(Param) is expected or ...

But the "free" function of the job should be more than free() and should check and free Param->Act as you said. In addition, because it does not seem to be used when the job terminates successfully, it is only used when the job is shutdown, the Param and Param->Act should be freed when the action is completed together with ActionResult which is allocated locally - or- we use the job's free() function all the time. So it seems a bit messy ...

BTW, I have also seen other leaks but I first concentrated on this one. I did not use valgrind on my app when I moved from 1.6 to 1.14 and I should have. Do you run such tests on your side?

[edit]: looking at the free_func() use in TimerPool, it feels to me that ThreadPool was not finished. Do you remember when and who did that? Can't find it easily from the blame record.

[philippe44]

More reading seems to indicate that WorkerThread should, after it has called the job's function, call it's cleanup function as well if you want to apply the principle that these pointers are "weak" and should not be managed by the job function itself but a free function should be set when the job function is set. As said above, would probably be useful to know what was the intent of whoever wrote that code in the first place.

[mrjimenez]

For 2. I've never seen a double free and I make a very intensive use of libupnp (at least as control point). Intensive means 10+ clients sending Actions every second or less. That's why the leak is catastrophic for me, but I'd think I'd have seen a double free. Now, it seems to me that the "free" function pointer to free the arg passed at job's creation is only used when the thread is shutdown, not otherwise. So I'd say that free(Param) is expected or ...

But the "free" function of the job should be more than free() and should check and free Param->Act as you said. In addition, because it does not seem to be used when the job terminates successfully, it is only used when the job is shutdown, the Param and Param->Act should be freed when the action is completed together with ActionResult which is allocated locally - or- we use the job's free() function all the time. So it seems a bit messy ...

100% agree.

BTW, I have also seen other leaks but I first concentrated on this one. I did not use valgrind on my app when I moved from 1.6 to 1.14 and I should have. Do you run such tests on your side?

I have, but a long time ago, not currently.

[edit]: looking at the free_func() use in TimerPool, it feels to me that ThreadPool was not finished. Do you remember when and who did that? Can't find it easily from the blame record.

That code does not seem to be from the original Intel library. I am not sure, but I think this code is from the creator of this project in SourceForge (pupnp), but he left soon after the project creation and never showed up again. At that time, pthread was not in glibc (yes, this is old code).

So, from your experience, we never get a double free. So we could leave the free(Parm) call. But it seems like we must do a ixmlDocument_free(actionResult); just before, what do you think? Can you test it?

Regards, Marcelo.

[philippe44]

I would add a free ActionResult indeed when the action is done but then I think I would try to use the job's free_func as it is intended and call it after the job has been executed, like is is in timer jobs.

Then I would modify the free_func for actions because it's necessary anyway when jobs are shutdown, otherwise we don't have a proper cleanup.

I can give that a try and see how it looks, it does not seem that there are so many cases to handle.

Can you tell me how the code in UPnPActionComplete.c is generated?

[mrjimenez]

I would add a free ActionResult indeed when the action is done but then I think I would try to use the job's free_func as it is intended and call it after the job has been executed, like is is in timer jobs.

Then I would modify the free_func for actions because it's necessary anyway when jobs are shutdown, otherwise we don't have a proper cleanup.

I can give that a try and see how it looks, it does not seem that there are so many cases to handle.

Can you tell me how the code in UPnPActionComplete.c is generated?

The generation is done in generator/generator.c, there is not much to see here, it is just boiler plate code, that in the 1.4.x branch was repeated to exhaustion. The idea was to introduce opaque structures and a template like code generation to limit the propagation of silly bugs. If you want, you can change it for sure, but it is mostly uninteresting. The next logical step with this library would be to change it to C++, and in this case the generator code would be wiped out.

cd ~/{some path}/upnp/generator
./complile.sh

The compile.sh script will copy the files to the proper places. The class definitions are declarative and quite self explanatory (INIT_MEMBER, INIT_CLASS). There is even a TestClass. The rest of the program is just a file generation program, one file for each class.

If you want to extend that functionality, I think you should hand create an extended class UPnPActionCompleteEx.c that uses the UPnPActionComplete.c class and extends its functionality. Just my two cents, you are free to do whatever you prefer.

Regards, Marcelo.