Patch - plug a memory leak

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Patch - plug a memory leak

Sam Varshavchik
valgrind comes back with a memory leak in the new version of HTBound.c. When
I rewrote HTBound.c last year, I copied some snippets from the old version
into the new version on HTBound.c, I guess without fully understanding other
parts of libwww.  Can't really say for sure, but the old HTBound.c might've
also been leaking in the same spot.  The analogous HTStreamStack call in the
old HTBound.c had the same parameters.

Anyway, this patch makes valgrind happy.  Perhaps someone who's more
familiar with HTMerge can share a comment, here.


Index: Library/src/HTBound.c
===================================================================
RCS file: /cvsroot/lpmtool/libwww/Library/src/HTBound.c,v
retrieving revision 1.2
diff -U3 -r1.2 HTBound.c
--- Library/src/HTBound.c 5 Jan 2006 01:27:20 -0000 1.2
+++ Library/src/HTBound.c 9 Jul 2006 20:59:16 -0000
@@ -410,7 +410,7 @@
 
  if (!isterminal)
  me->target = HTStreamStack(WWW_MIME,me->format,
-   HTMerge(me->orig_target, 2),
+   HTMerge(me->orig_target, 1),
    me->request, YES);
 }
 


attachment0 (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Patch - plug a memory leak

Vic Bancroft-2

Sam Varshavchik wrote:

> valgrind comes back with a memory leak in the new version of
> HTBound.c. When I rewrote HTBound.c last year, I copied some snippets
> from the old version into the new version on HTBound.c, I guess
> without fully understanding other parts of libwww.

No doubt, there is a lot to read in Library/src !

> Can't really say for sure, but the old HTBound.c might've also been
> leaking in the same spot.  The analogous HTStreamStack call in the old
> HTBound.c had the same parameters.

An HTStreamStack is constructed for format conversions, but most callers
do not use the fancy HTMerge action like your invocation.  The only
other usage of HTMerge is an interesting section in HTMIME when there is
the explict handling of two feeds, one from the network and one from the
cache.  It would appear that in the context of HTBound there is only one
feed.  Egads, how did we not see this before :^?

> Anyway, this patch makes valgrind happy.  Perhaps someone who's more
> familiar with HTMerge can share a comment, here.

Most excellent !  It is now in the HEAD as new revision: 2.16.  There
were also some "#if 0" snippets in HTMIME.c which could use some
trimming, giving us a new revision: 2.102 . . .

Taken together cvs diff looked like,

    Index: Library/src/HTBound.c
    ===================================================================
    RCS file: /sources/public/libwww/Library/src/HTBound.c,v
    retrieving revision 2.15
    diff -r2.15 HTBound.c
    412c412
    <                                          HTMerge(me->orig_target, 2),
    ---
     >                                          HTMerge(me->orig_target, 1),
    [bancroft@hilbert libwww]$ cvs diff lLibrary/src/HTMIME.c
    cvs [diff aborted]: no such directory `lLibrary/src'
    [bancroft@hilbert libwww]$ cvs diff Library/src/HTMIME.c
    Index: Library/src/HTMIME.c
    ===================================================================
    RCS file: /sources/public/libwww/Library/src/HTMIME.c,v
    retrieving revision 2.101
    diff -r2.101 HTMIME.c
    198,201d197
    < #if 0
    <           /* @@ JK: change */
    <           if (append) me->target = append;
    < #endif
    661c657
    <       HTTRACE(STREAM_TRACE, "Cache Flush. Flushing and freeing
    PIPE buffer\n");
    ---
     >       HTTRACE(STREAM_TRACE, "Cache Flush. Flushing PIPE buffer\n");
    663,667d658
    < #if 0
    <       /* @@ JK: flush converts the pipe to an open one, we shouldn't
    <          free it as we'll loose our references */
    <       (*pipe->isa->_free)(pipe);
    < #endif
    720,731d710
    < #if 0
    <     /* JK: this doesn't work because this work is repeated before */
    <     /*
    <     **  Create the cache append stream, and a Tee stream
    <     */
    <     {
    <       HTStream * append = HTStreamStack(WWW_CACHE_APPEND,
    output_format,
    <                                         output_stream, request, NO);
    <       if (append) me->target = HTTee(me->target, append, NULL);
    <     }
    < #endif
    <

more,
l8r,
v

BTW, is it time to revisit the release party idea as per ChangeLog
revision 1.57 should we continue to worry over modification to address
Ben's security concerns?  Along those lines,  did anyone come up with an
appropriate Solaris box to construct a patch or vet a clean build as per
the errors experienced by Arthur Smith ?

--
"The future is here. It's just not evenly distributed yet."
 -- William Gibson, quoted by Whitfield Diffie