Chris Kuklewicz wrote:
I was working on pulling library calling code from JRegex into Text.Regex.Lazy and I am wondering if this code for compiling regex.h regular expressions is a potential memory leak:
regcomp pattern flags = do regex_fptr <- mallocForeignPtrBytes (#const sizeof(regex_t)) r <- withCString pattern $ \cstr -> withForeignPtr regex_fptr $ \p -> c_regcomp p cstr (fromIntegral flags) if (r == 0) then do addForeignPtrFinalizer ptr_regfree regex_fptr return (Regex regex_fptr) else error "Text.Regex.Posix.regcomp: error in pattern" -- ToDo
The (r/=0) path calls error, but never attaches the finalizer. And the man page for regex says:
The regfree() function frees any dynamically-allocated storage associated with the compiled RE pointed to by preg. The remaining regex_t is no longer a valid compiled RE and the effect of supplying it to regexec() or regerror() is undefined.
So it looks like the c_regcomp might allocate memory associated with regex_fptr even in the event of an error, so this case needs the same finalizer.
We made the opposite change back in 2003: Fri Jul 25 11:03:51 BST 2003 simonmar * [project @ 2003-07-25 10:03:51 by simonmar] regcomp: don't attach the regfree finalizer if c_regcomp failed. M ./Text/Regex/Posix.hsc -2 +2 diff -rN -u old-base/Text/Regex/Posix.hsc new-base/Text/Regex/Posix.hsc --- old-base/Text/Regex/Posix.hsc 2006-07-19 12:50:43.000000000 +0100 +++ new-base/Text/Regex/Posix.hsc 2006-07-19 12:50:46.000000000 +0100 @@ -60,9 +60,9 @@ r <- withCString pattern $ \cstr -> withForeignPtr regex_fptr $ \p -> c_regcomp p cstr (fromIntegral flags) - addForeignPtrFinalizer regex_fptr ptr_regfree if (r == 0) - then return (Regex regex_fptr) + then do addForeignPtrFinalizer regex_fptr ptr_regfree + return (Regex regex_fptr) else error "Text.Regex.Posix.regcomp: error in pattern" -- ToDo So I'm guessing that calling regfree() should not be called if regcomp() failed. I vaguely remember a bug report for this, but don't have it to hand. If you have evidence to the contrary, then perhaps we should revert this. Cheers, Simon