Bug 14780 - [PATCH] handle malloc() and realloc() failures in regcomp()
Summary: [PATCH] handle malloc() and realloc() failures in regcomp()
Status: WAITING
Alias: None
Product: glibc
Classification: Unclassified
Component: regex (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-28 13:26 UTC by Jindrich Makovicka
Modified: 2014-06-25 06:43 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments
Patch for regcomp.c (3.36 KB, patch)
2012-10-28 13:26 UTC, Jindrich Makovicka
Details | Diff
patch w/ malloc() fuzzing, used for testing (3.70 KB, patch)
2012-10-28 13:27 UTC, Jindrich Makovicka
Details | Diff
test case (452 bytes, text/x-csrc)
2012-10-28 13:28 UTC, Jindrich Makovicka
Details
handle malloc() and realloc() failures in regcomp() (3.85 KB, patch)
2012-10-28 14:42 UTC, Jindrich Makovicka
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jindrich Makovicka 2012-10-28 13:26:59 UTC
Created attachment 6705 [details]
Patch for regcomp.c

Hi,

currently, regcomp() misses a lot of checks for memory allocation
failures, and it also does not properly release memory on error paths.
This means a malloc error usually causes either a SEGV or a memory
leak.

The attached patch (regex.diff) adds the return value checks and
memory deallocation on failures.
Comment 1 Jindrich Makovicka 2012-10-28 13:27:55 UTC
Created attachment 6706 [details]
patch w/ malloc() fuzzing, used for testing
Comment 2 Jindrich Makovicka 2012-10-28 13:28:28 UTC
Created attachment 6707 [details]
test case
Comment 3 Jindrich Makovicka 2012-10-28 13:28:54 UTC
I have been debugging this issue by fuzzing re_malloc() and
re_realloc(), making them randomly return NULL. The patch with added
fuzzing is attached as regex-fuzzed.diff . testcase.c has been used to
exercise the modified regcomp().
Memory violations or leaks have been tested using valgrind: valgrind
--leak-check=full --show-reachable=yes --trace-children=yes
./testrun.sh ./testcase
Comment 4 Jindrich Makovicka 2012-10-28 14:42:10 UTC
Created attachment 6708 [details]
handle malloc() and realloc() failures in regcomp()

patch updated for current glibc git
Comment 5 Siddhesh Poyarekar 2013-01-17 14:10:10 UTC
Thanks for the patch.  Please use the following wiki document as a guideline:

http://sourceware.org/glibc/wiki/Contribution%20checklist

and post your patch for review on the libc-alpha mailing list:

http://www.gnu.org/software/libc/development.html
Comment 6 Rich Felker 2013-09-12 03:26:40 UTC
Are there any actual cases where malloc failure is not checked? I reviewed regcomp.c briefly and it seems the result is eventually (just not immediately) checked before use. However, there are major leaks when malloc has failed, since multiple results are checked together and no effort is made to free the ones that did succeed.
Comment 7 Jindrich Makovicka 2013-09-12 17:21:48 UTC
(In reply to Rich Felker from comment #6)
> Are there any actual cases where malloc failure is not checked? I reviewed
> regcomp.c briefly and it seems the result is eventually (just not
> immediately) checked before use. However, there are major leaks when malloc
> has failed, since multiple results are checked together and no effort is
> made to free the ones that did succeed.

I do not really recall anymore if there _really_ was a segfault, or it was only caused when I tried to free such partially compiled regex using regfree(). But you can insert the fuzzing code from the first patch, consisting of xxmalloc and xxrealloc from regcomp.c and #defines from regcomp.h, and run the attached testcase with, say, 100000 iterations and look what happens.

The memory leaks are obviously real, and were the main reason I was looking into this.
Comment 8 Rich Felker 2013-09-14 16:50:01 UTC
If regcomp() returned failure, passing the regex to regfree() is invalid, so crashes there would not be a bug. In any case, the memory leaks are a bug and should be fixed.
Comment 9 Ondrej Bilka 2013-10-14 16:09:57 UTC
Did you send this patch to libc-alpha?