[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [Libevent-users] Fwd: leaking munmap



It boils down to a single line of code to show the leak, but I've attached a simple test program to show it.

Bob.


On Tue, Jul 30, 2013 at 11:27 PM, Thomas Dial <dialtr@xxxxxxxxx> wrote:
There is a way to write a test for descriptor leaks, and I'd be willing to take a crack at it; could the original poster respond to me directly with some problematic code?  
Note: I'm relatively unfamiliar with the code base these days, so it may take me a little time to get up to speed on how tests are set up.

Regards,
Tom Dial




From: Nick Mathewson <nickm@xxxxxxxxxxxxx>
To: libevent-users@xxxxxxxxxxxxx
Sent: Tuesday, July 30, 2013 8:46 AM
Subject: Re: [Libevent-users] Fwd: leaking munmap

On Tue, Jul 30, 2013 at 1:48 AM, Black Hole <randomblackhole@xxxxxxxxx> wrote:
>
>
> ---------- Forwarded message ----------
> From: Black Hole <randomblackhole@xxxxxxxxx>
> Date: Tue, Jul 30, 2013 at 3:46 PM
> Subject: leaking munmap
> To: majordomo@xxxxxxxx
>
>
> Using 2.1-alpha.
>
> In evbuffer_file_segment_free, buffer.c:3077
>
> I think the line:
>
> if (munmap(seg->mapping, seg->length) == -1)
>
> should read:
>
> off_t offset_leftover = seg->file_offset & (page_size - 1);
>
> if (munmap(seg->mapping, seg->length + offset_leftover) == -1)
>
>
> Without that change, it would leak memory and kernel descriptors.

Hm. Seems plausible; I wonder if there's a way to write a test for this.



> On a related note, would there be any benefit in caching the pagesize
> instead of calling sysconf() each time? Something like:
>>
>> /* pagesize should be a power of 2 */
>> long getpagesize_(void) {
>>    static long page_size = -2;
>>    if (page_size == -2) {
>> #ifdef SC_PAGE_SIZE
>>        page_size = sysconf(SC_PAGE_SIZE);
>> #elif defined(_SC_PAGE_SIZE)
>>        page_size = sysconf(_SC_PAGE_SIZE);
>> #else
>>        page_size = 4096;
>> #endif
>>    }
>>    return page_size;
>> }

Is there a platform where sysconf is slow or expensive?  My
understanding is that it's usually (always?) a libc function, not a
syscall, and it should run pretty fast.



> Also, in evbuffer_file_segment_materialize() around line 2980, the
> calculation of "offset_leftover" we can remove the modulus in favour of a
> mask.
>>
>> long page_size = getpagesize_();
>>
>> if (page_size == -1)
>>
>>        goto err;
>>
>> offset_leftover = offset & (page_size - 1);

Assuming that the page size is always a power of 2, yeah.  (I'm not
aware of any systems pathological enough to have 6K pages or
something, so this should be fine.)


--
Nick
***********************************************************************
To unsubscribe, send an e-mail to majordomo@xxxxxxxxxxxxx with
unsubscribe libevent-users    in the body.



#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <string.h>
#include <errno.h>

#include <event2/buffer.h>

int main(int argc, char *argv[])
{
    struct evbuffer_file_segment *seg=NULL;
    struct stat st;
    int fd = -1;
    ev_off_t offset, length;
    long pagesize;
    char cmd[128], buf[512];
    FILE *fp;

    if (argc < 2) {
	fprintf(stderr, "I need a valid filename\n");
	exit(-1);
    }
    if ((fd = open(argv[1], O_RDONLY)) < 0) {
	fprintf(stderr, "open() error: %s\n", strerror(errno));
	exit(-1);
    }
    fstat(fd, &st);
    pagesize = sysconf(_SC_PAGE_SIZE);

    /* Choose an offset that doesn't fall on a page boundary to trigger the leak. */
    offset = 331;	    /* a random prime that is not a pagesize */
    length = pagesize*2;    /* not important */

    if (offset + length > st.st_size) {
	fprintf(stderr, "Pick a file larger than %zd bytes for testing.\n", offset+length);
	exit(-1);
    }

    /*
     * THIS IS THE ONLY LINE THAT MATTERS!
     * This line should be enough to trigger a 'materialize' which mmap's and leaks during free.
     */
    seg = evbuffer_file_segment_new(fd, offset, length, EVBUF_FS_DISABLE_LOCKING | EVBUF_FS_DISABLE_SENDFILE);

    /* loose cleanup */
    evbuffer_file_segment_free(seg);
    close(fd);


    /* Run 'lsof' command to show that part of the file is still mapped.
     * This could be done on the command line instead of in code,
     * or we could use valgrind. */
    snprintf(cmd, sizeof(cmd), "/usr/sbin/lsof -nlPp %d", getpid());
    if ((fp = popen(cmd, "r")) != NULL) {
	while (fgets(buf, sizeof(buf), fp) != NULL) {
	    buf[sizeof(buf)-1] = 0;
	    printf(buf);
	}
	printf("\n");
	pclose(fp);
    } else {
	printf("popen() error: %s\n", strerror(errno));
    }
    return 0;
}