-
Notifications
You must be signed in to change notification settings - Fork 117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
memory leak in upnpapi.c ? #424
Comments
Just with a short glance:
Is it possible that this callback function frees the action? |
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. |
Sorry for asking but I try to understand it.
What does it do and where? What should it do?
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? |
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 |
Ah, the code generator. Maybe @mrjimenez can enlighten it? |
Hi @ingo-h , Part of the API code is automatically generated by @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 TPJobInit(&job, (start_routine)UpnpThreadDistribution, Param);
TPJobSetFreeFunction(&job, (free_routine)free);
TPJobSetPriority(&job, MED_PRIORITY); Notice that If you are wiling to follow the rabbit down that hole, you can probably find the corresponding
Regards, |
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 |
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. UpnpActionComplete_set_ActionRequest(Evt, Param->Act);
On the other hand, you seem to be correct in the case of ActionResult, since it seems to be alloc'd somewhere inside In fact, To sum up, there seems to be two problems:
Please, feel free to disagree, I am trying to make sense of the code too. 😄 Regards, |
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. |
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. |
100% agree.
I have, but a long time ago, not currently.
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 Regards, |
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 If you want to extend that functionality, I think you should hand create an extended class Regards, |
This intends to fix #424. I've found at least 2 memory leaks 1- When sending Action(Ex)Async The ActionRequest document was not released nor was the ActionResult. I initially tought that this should be done in UpnpActionComplete::UpnpActionComplete_delete() but I was wrong because ActionRequest and ActionResult do not "belong" to the Evt object. The sets methods only set a pointer and modifying generator.c is not the right solution. The reason is that ActionRequest "belongs" to the ACTION job and should be handled as such if we want as well that, when the job is cancelled, the memory is released as well. So I've change the free_func for ACTION to reflect that. 2- When using event subscription The Event was not released upon Timer termination. So I've done as above, created a special free_func that handles that properly. In general, when something prevents the execution/submission of a job, I've also used the actual free function instead of duplicating the individual free(). Note that when terminating a timer, I'm using the job's internal structure free_func() as it was already used to access arg. One can argue that it should be an opaque structure and get/set or the option to call the free function could be use instead. Also, I realized that ActionExAsync has never been working properly because when the job is executed, only an Action is made, never an ActionEx although the Header document is in the job's argument. I've fixed that as well and stopped there to not go too deep in that rabbit hole. I've also removed the upnp_timeout.c/h files as they were not used anymore (and had a strange names anyway)
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
It used to be, in previous version
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
And my configure line is
The text was updated successfully, but these errors were encountered: