lib/39353: libpuffs double-free

[Taylor R Campbell <campbell%mumble.net@localhost>]

>Number:         39353
>Category:       lib
>Synopsis:       libpuffs double-free
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Aug 15 00:30:00 +0000 2008
>Originator:     Taylor R Campbell <campbell%mumble.net@localhost>
>Release:        NetBSD 4.0_STABLE
>Organization:
        
>Environment:
System: NetBSD smalltalk.localdomain 4.0_STABLE NetBSD 4.0_STABLE 
(RIAXEN3_DOM0) #2: Fri Jul 18 23:32:56 UTC 2008 
riastradh@smalltalk:/home/riastradh/netbsd/4/obj/sys/arch/i386/compile/RIAXEN3_DOM0
 i386
Architecture: i386
Machine: i386
>Description:

        In src/lib/libpuffs/framebuf.c, puffs__framev_input calls
        puffs__framev_readclose when the user-supplied reader yields an
        error.  puffs__framev_readclose destroys a frame buffer,
        fio->cur_in, which puffs__framev_input then proceeds to destroy
        again, under the name pufbuf (which is the buffer that it set
        fio->cur_in to earlier), as soon as puffs__framev_readclose
        returns.

>How-To-Repeat:

        Mount an sshfs.  Pull out the ethernet cable.  List a directory
        in the sshfs.  Make some tea, and then watch mount_psshfs dump
        core when the ssh connection fails.

>Fix:

        Don't destroy pufbuf after `puffs__framev_readclose' has
        already done it:

--- framebuf.c  30 Jan 2008 16:04:08 +0000      1.28
+++ framebuf.c  14 Aug 2008 23:28:26 +0000      
@@ -679,11 +679,6 @@
                /* error */
                if (rv) {
                        puffs__framev_readclose(pu, fio, rv);
                        fio->cur_in = NULL;
-                       if ((pufbuf->istat & ISTAT_DIRECT) == 0) {
-                               assert((pufbuf->istat & ISTAT_NODESTROY) == 0);
-                               puffs_framebuf_destroy(pufbuf);
-                       }
                        return;
                }
 
        The deleted assertion is also superfluous, since
        `puffs_framebuf_destroy' asserts the same condition on entry.

        Unfortunately, even with this fix, although the file system
        operations now correctly signal ECONNRESET (in the example
        above), puffs segfaults afterward anyway, and the stack trace
        is not helpful.  I am not yet familiar enough with puffs to
        understand what is going wrong.