On 01/15/2014 07:48 PM, Marcelo Roberto Jimenez wrote:
Hi Peng,

Thank you very much for your patches, I will take a look at them as soon as possible.

Just to get things straight, 0001-rewrite-soap_device.c.patch is the same patch you have previously sent, just cherry picked to the master branch, and 0002-Fix-memory-leaks-caused-by-DOMString only applies to master. Are these statements correct?
Yes. And the memory leaks only exists for the master branch. Please review these patches, and comment if necessary. I will modify them according to comments.

Again, thank you for your work.

Regards,
Marcelo.



On Wed, Jan 15, 2014 at 7:20 AM, Peng <howtofly@gmail.com> wrote:
I cherry-picked it to the master branch (see the first patch).

When doing unit test (with test cases attached in the previous mail) for soap device, I found several memory leaks:
==19438== 2 bytes in 1 blocks are definitely lost in loss record 1 of 4
==19438==    at 0x40299D8: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==19438==    by 0x414B3B0: strdup (strdup.c:42)
==19438==    by 0x407E6CE: ixmlCloneDOMString (ixml.c:458)
==19438==    by 0x405D710: UpnpStateVarRequest_set_CurrentVal (TemplateSource.h:334)
==19438==    by 0x804B583: TvDeviceHandleGetVarRequest (tv_device.c:373)
==19438==    by 0x804B8EF: TvDeviceCallbackEventHandler (tv_device.c:1272)
==19438==    by 0x4: ???
==19438==
==19438== 2 bytes in 1 blocks are definitely lost in loss record 2 of 4
==19438==    at 0x40299D8: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==19438==    by 0x414B3B0: strdup (strdup.c:42)
==19438==    by 0x407E6CE: ixmlCloneDOMString (ixml.c:458)
==19438==    by 0x405D710: UpnpStateVarRequest_set_CurrentVal (TemplateSource.h:334)
==19438==    by 0x804B583: TvDeviceHandleGetVarRequest (tv_device.c:373)
==19438==    by 0x804B8EF: TvDeviceCallbackEventHandler (tv_device.c:1272)
==19438==    by 0xA: ???
==19438==
==19438== 18 bytes in 2 blocks are definitely lost in loss record 3 of 4
==19438==    at 0x40299D8: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==19438==    by 0x414B3B0: strdup (strdup.c:42)
==19438==    by 0x407E6CE: ixmlCloneDOMString (ixml.c:458)
==19438==    by 0x405C8B0: UpnpFileInfo_set_ContentType (TemplateSource.h:334)
==19438==    by 0x403E97F: get_file_info (webserver.c:347)
==19438==    by 0x4051D38: web_server_callback (webserver.c:1173)
==19438==    by 0x1: ???
==19438==

These leaks also exist for previous version, the second patch fixed these.


On 01/03/2014 08:52 PM, Peng wrote:
Hi, everyone.

I just re-write soap_device.c with the following aims in mind:

1) separate HTTP handling from SOAP handling
HTTP and SOAP handling are mixed together, and some HTTP errors are treated (inappropriately) as SOAP error (see 3 below). Mixed handling makes fixing 3) difficult.

2) remove repeated (and unnecessary) validity checks, each check should be performed *exactly once*
For example, SOAPACTION hdr is checked in both get_request_type and check_soap_action_header. Another such example is the xml entity (Envelope). These checks are both inefficient and unnecessary, and are thus removed.

3) fix HTTP status code per UPnP spec, SOAP spec and RFC 2774
UPnP spec states that if the CONTENT-TYPE header specifies an unsupported value (other then “text/xml”) the device must return an HTTP status code “415 Unsupported Media Type”. RFC 2774 states that a server receiving a mandatory request including the "M-" method name prefix without any mandatory extension declarations to follow MUST return a 510 (Not Extended) response. Also, I try to follow SOAP 1.2's spirit, which has a complete description of the finite state machine of the server, since SOAP 1.1 is quite vague on this.

As a side effect, the patch fixed the following over-read error, which was detected by valgrind when running unit tests against the previous version(see attached testing scripts):
==10174== Thread :
==10174== Invalid read of size 4
==10174==    at 0x40402DC: check_soap_action_header (soap_device.c:444)
==10174==    by 0x404058D: get_device_info (soap_device.c:527)
==10174==    by 0x4040EC1: handle_invoke_action (soap_device.c:779)
==10174==    by 0x4041518: soap_device_callback (soap_device.c:870)
==10174==    by 0x40429CA: handle_request (miniserver.c:157)
==10174==    by 0xBBA10001: ???

If cherry-picking this patch were time-consuming, I would prepare another patch for the master branch.

PS: Happy new years!


--
Peng


------------------------------------------------------------------------------
CenturyLink Cloud: The Leader in Enterprise Cloud Services.
Learn Why More Businesses Are Choosing CenturyLink Cloud For
Critical Workloads, Development Environments & Everything In Between.
Get a Quote or Start a Free Trial Today.
http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
_______________________________________________
pupnp-devel mailing list
pupnp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/pupnp-devel




------------------------------------------------------------------------------
CenturyLink Cloud: The Leader in Enterprise Cloud Services.
Learn Why More Businesses Are Choosing CenturyLink Cloud For
Critical Workloads, Development Environments & Everything In Between.
Get a Quote or Start a Free Trial Today. 
http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk


_______________________________________________
pupnp-devel mailing list
pupnp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/pupnp-devel

-- 
Peng