Hi Is it just me or the qregex patch has a memory leak ? (valgrind spotted it right away) Here is the incriminating code: int bmcheck(which) int which; { int i = 0; int j = 0; int x = 0; int negate = 0; stralloc bmb = {0}; stralloc curregex = {0}; if (which == BMCHECK_BMF) { if (!stralloc_copy(&bmb,&bmf)) die_nomem(); } else if (which == BMCHECK_BMT) { if (!stralloc_copy(&bmb,&bmt)) die_nomem(); } else { die_control(); } bmb is a locally declared variable initilized each function call with {0}. stralloc_copy will allocate memory to hold the copied result. This memory is never free()d. this also happens to curregex variabile used a little down in the same function. I think a user could trigger a DOS pretty simple my having a program connect to such a qmail-smtpd and issue "mail from: someuser@somedomain.com" indefinetly. while looking into a solution on this I found that DJB/qmail doesnt ship with a stralloc_free() function and thus no "clean" way to free the stralloc-ated strings. Any qmail guru wants to help ? :) Thanks! Reproducible: Always Steps to Reproduce: 1. 2. 3.
Whoops, I forgot a very important information, the piece of code and the memory leak happens into qmail-smtpd.c.
I edited the title, I was wrong, qregex patch seems to only come with -r12 ebuild of qmail.
Simple test program: #include "stdio.h" #include "stralloc.h" void die_nomem() { printf("die_nomem()\n"); } void die_control() { printf("die_control()\n"); } void die_done() { printf("die_done()\n"); } int main() { stralloc bmb = {0}; stralloc bmf = {0}; stralloc bmt = {0}; int bmfok; int which = 0; bmfok = control_readfile(&bmf,"test-string.c",0); bmfok = control_readfile(&bmt,"test-string.c",0); if (which == 0) { if (!stralloc_copy(&bmb,&bmf)) die_nomem(); } else if (which == 1) { if (!stralloc_copy(&bmb,&bmt)) die_nomem(); } else { die_control(); } die_done(); }
curie-int qmail-1.03 # valgrind --leak-check=yes ./test-string ==31006== Memcheck, a.k.a. Valgrind, a memory error detector for x86-linux. ==31006== Copyright (C) 2002-2003, and GNU GPL'd, by Julian Seward. ==31006== Using valgrind-20030725, a program supervision framework for x86-linux. ==31006== Copyright (C) 2000-2003, and GNU GPL'd, by Julian Seward. ==31006== Estimated CPU clock rate is 2022 MHz ==31006== For more details, rerun with: -v ==31006== die_done() ==31006== ==31006== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) ==31006== malloc/free: in use at exit: 1168 bytes in 2 blocks. ==31006== malloc/free: 3 allocs, 1 frees, 1632 bytes allocated. ==31006== For counts of detected errors, rerun with: -v ==31006== searching for pointers to 2 not-freed blocks. ==31006== checked 3599436 bytes. ==31006== ==31006== 1168 bytes in 2 blocks are definitely lost in loss record 1 of 1 ==31006== at 0x400296E5: malloc (in /usr/lib/valgrind/vgskin_memcheck.so) ==31006== by 0x8048E77: alloc (in /var/tmp/portage/qmail-1.03-r12/work/qmail-1.03/test-string) ==31006== by 0x8048EE9: alloc_re (in /var/tmp/portage/qmail-1.03-r12/work/qmail-1.03/test-string) ==31006== by 0x8048E0C: stralloc_readyplus (in /var/tmp/portage/qmail-1.03-r12/work/qmail-1.03/test-string) ==31006== ==31006== LEAK SUMMARY: ==31006== definitely lost: 1168 bytes in 2 blocks. ==31006== possibly lost: 0 bytes in 0 blocks. ==31006== still reachable: 0 bytes in 0 blocks. ==31006== suppressed: 0 bytes in 0 blocks. ==31006== Reachable blocks (those to which a pointer was found) are not shown. ==31006== To see them, rerun with: --show-reachable=yes ==31006==
bug reported to author evan@unixpimps.org I'm not sure of a good fix for the bug for now.
One solution would be to add a stralloc_free() function and use it. However this would use too much CPU and unneeded overhead on the memory allocator. My quick hack was to move declare those 2 local function variables as "static" inside the function so the allocated space can be reused (of course, when function starts we need to reset the "used" length of those strings). What do you think ? :) diff -u -r1.5 qmail-smtpd.c --- qmail-smtpd.c 7 Oct 2003 11:50:43 -0000 1.5 +++ qmail-smtpd.c 9 Oct 2003 10:50:43 -0000 @@ -344,8 +344,10 @@ int j = 0; int x = 0; int negate = 0; - stralloc bmb = {0}; - stralloc curregex = {0}; + static stralloc bmb = {0}; + static stralloc curregex = {0}; + + bmb.len = curregex.len = 0; if (which == BMCHECK_BMF) { if (!stralloc_copy(&bmb,&bmf)) die_nomem(); PS: the patch above may not apply clean couse I use a lots of patche over qmail-smtpd but one should get the ideea
upstream has NOT fixed the bug and i'm strongly inclined to avoid your 'static' fix due to possible interactions with other code.
Can you please detail on what interactions with other code ? Those 2 variables are local to the function. They have the problem of beeing memory allocated each call to that function, thus we defined them static and initilize the len field to 0 (but the allocated len and the pointer to data field remains with the last value from the last call so we reuse the previous allocated memory). Thanks :)
*** Bug 34601 has been marked as a duplicate of this bug. ***
sorry, i've been thinking about multithreaded java too much due to an assignment, and it's insane behavior as to where a static variable exists inside the VM. Done for -r14.
if you are feeling experimental, -r14 is now out. It's hardmasked until I personally test it some more, but it should work.
I am running -r14 for more than a week now and seems fine.
Mihai: have a look at alloc.c/alloc.h the alloc_free(x) function
Yes, I know about it. There is no stralloc_free() to work on stralloc types. Using alloc_free() we have to bypass the stralloc API and work directly on the struct fields (which is dirty but I think I already done that by setting len to 0 in the beginning of the func with my fix). So whatever you think its better, my fix (lesser operations on the memory allocater but leaves those static local function variables allocated on exit) or using alloc_free() on the data pointer of the stralloc types before every exit points of the function (more memory allocator operations but should leave no memory behind when qmail-smtpd exits). :) I dont have a preference which solution to use over the other. I am curently using my fix in a private qmail CVS tree for use on my employer's servers and seems to be fine (not that the memleak was a very serious issue when running with softlimit).
