Re: Strange segfault with improper usage
Paolo <[email protected]>
| Newsgroups | gmane.mail.spam.crm114 |
|---|---|
| Message-ID | <20080324160053.GZ20978@localhost> |
On Wed, Mar 19, 2008 at 11:19:42PM +0100, Paolo wrote: > On Wed, Mar 19, 2008 at 10:47:43PM +0100, Milan Zamazal wrote: > > > > P> cool, that's TRE fault, guess Ville would be interested to know. > > > > Do you think it's completely TRE fault or is it initiated by illegal > > call arguments from crm? Are you still interested in an i386 core dump > > could be either case, though in the latter I'd still consider TRE buggy, the latter (as it seems); TRE gets passed an invalid preg, its internal checks don't suffice and boom. Bug seems in our regex cache handler in crmregex_tre.c - if you check the trace (on amd64 at least) you'd see it never hit a 'found it', moreover there's junk beyond the regex, eg: ... No further expansions possible match pattern expands to =.+= len 2 flags 1 0 Checking the regex cache for .+ couldn't find it Compiling .+½ý^D+ (len 2). About to return ... disabling cache (CACHE_SIZE=0) -> no segv. The attached patch reworks the regex cache code quite a bit; it correctly finds and reuses cached regex, eg: ... No further expansions possible match pattern expands to =^#= len 2 flags 1 0 Checking the regex cache for '^#' [<- rex cache random] regex[211]:'SET' cf=0x1(0x1) len=3(2) regex[268]:'^[ ]*(:[[:graph:]]+:)[ ]+/(.*)/' cf=0x5(0x1) len=32(2) regex[331]:'^#' cf=0x1(0x1) len=2(2) regex[682]:' [[:blank:]]*(insert)[[:blank:]]+([[:graph:]]+)[[:blank:]]*[ ;]' cf=0x7(0x1) len=63(2) regex[724]:'.+' cf=0x1(0x1) len=2(2) strnhash (regex, regex_len) % 1024 = 331 found it at [331]. About to return: preg->re_nsub=0 preg->value=0x7e5e10, i=331, status=0 ... and I was unable to get segv on provided trigger code, also by increasing ENV and (stdin) by 1 char a time for some 1000s runs. @those who reported the segv, pls try the patch and report result. If you get segv again, pls 1st run after recompling with if (internal_trace) in the patch commented out, like: //if (internal_trace) ie don't enable -T; let's see just those specific traces 1st. Note: I didn't bother the REGEX_CACHE_LINEAR_SEARCH blocks, since there's perhaps little point in using LINEAR instead of RANDOM, so it might break somewhere. -- paolo ------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/ _______________________________________________ Crm114-general mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/crm114-general
crmregex_tre.c_segv-080323.diff
(text/plain, 9.2 KB)
--- src/crmregex_tre.c 2007-11-26 13:10:04.000000000 +0100
+++ new/crmregex_tre.c 2008-03-24 16:08:54.000000000 +0100
@@ -25,6 +25,7 @@
// and include the routine declarations file
#include "crm114.h"
+#if CRM_REGEX_CACHESIZE > 0
// Cache for regex compilations
typedef struct {
char *regex;
@@ -34,12 +35,8 @@
int status;
} REGEX_CACHE_BLOCK;
-
-#if CRM_REGEX_CACHESIZE > 0
-
REGEX_CACHE_BLOCK regex_cache[CRM_REGEX_CACHESIZE]
= { { NULL, NULL, 0, 0, 0} } ;
-
#endif
@@ -79,55 +76,65 @@
// We are cacheing. Scan our cache set for the compiled versions
// of this regex. Note that a length of 0 means "empty bucket".
{
- int i, j, found_it;
- long rtsize = sizeof (regex_t);
+ int i, j;
+ int found_it = CRM_REGEX_CACHESIZE;
+ int rtsize = sizeof (regex_t);
regex_t *ppreg_temp = NULL;
char *regex_temp = NULL;
- long rlen_temp = 0;
+ int rlen_temp = 0;
int cflags_temp = 0;
int status_temp = 0;
- if (internal_trace) fprintf (stderr, "Checking the regex cache for %s\n",
- regex);
- j = 0;
+ i=j=0;
+ if (internal_trace)
+ {
+ fprintf (stderr, "Checking the regex cache for '%s'\n", regex);
+ for (j=0; j < CRM_REGEX_CACHESIZE; j++) {
+ if (regex_cache[i].regex_len > 0) fprintf (stderr,
+ " regex[%d]:'%s' cf=%#x(%#x) len=%d(%d)\n",
+ i,regex_cache[i].regex,regex_cache[i].cflags,cflags,
+ regex_cache[i].regex_len,regex_len);
+ }
#ifdef REGEX_CACHE_LINEAR_SEARCH
+ fputs (" [<- rex cache linear]\n", stderr);
+ }
//
// Linear Search uses a strict LRU algorithm to cache
// the precompiled regexes.
//
- found_it = 0;
- i = -1;
- while (!found_it && i < CRM_REGEX_CACHESIZE)
+ while (i < CRM_REGEX_CACHESIZE)
{
- i++;
if (regex_len == regex_cache[i].regex_len
&& cflags == regex_cache[i].cflags
&& strncmp (regex_cache[i].regex, regex, regex_len) == 0)
{
// We Found It! Put it into the _temp vars...
- if (internal_trace) fprintf (stderr, "found it.\n");
ppreg_temp = regex_cache[i].preg;
regex_temp = regex_cache[i].regex;
rlen_temp = regex_len;
cflags_temp = cflags;
status_temp = regex_cache[i].status;
found_it = i;
+ break;
};
+ i++;
};
-#endif
+ if (internal_trace)
+ {
+#else
#ifdef REGEX_CACHE_RANDOM_ACCESS
+ fputs (" [<- rex cache random]\n", stderr);
+ }
//
// Random Access uses an associative cache based on
// the hash of the regex (mod the size of the cache).
//
- found_it = 0;
i = strnhash (regex, regex_len) % CRM_REGEX_CACHESIZE;
if (regex_len == regex_cache[i].regex_len
&& cflags == regex_cache[i].cflags
&& strncmp (regex_cache[i].regex, regex, regex_len) == 0)
{
// We Found It! Put it into the _temp vars...
- if (internal_trace) fprintf (stderr, "found it.\n");
ppreg_temp = regex_cache[i].preg;
regex_temp = regex_cache[i].regex;
rlen_temp = regex_len;
@@ -135,23 +142,32 @@
status_temp = regex_cache[i].status;
found_it = i;
};
-#endif
+ if (internal_trace)
+ {
+ fprintf (stderr,
+ " strnhash (regex, regex_len) %% %d = %d\n", CRM_REGEX_CACHESIZE, i);
+#endif //REGEX_CACHE_RANDOM_ACCESS
+#endif //REGEX_CACHE_LINEAR_SEARCH
+ if (found_it == CRM_REGEX_CACHESIZE)
+ fprintf (stderr, "couldn't find it\n");
+ else
+ fprintf (stderr, "found it at [%d].\n",i);
+ }
// note that on exit, i now is the index where we EITHER found
// the good data, or failed to do so, and found_it tells us which.
//
- if ( ! (found_it))
+ if (found_it == CRM_REGEX_CACHESIZE)
{
// We didn't find it. Do the compilation instead, putting
// the results into the _temp vars.
- if (internal_trace) fprintf (stderr, "couldn't find it\n");
- regex_temp = (char *) malloc (regex_len + 1);
+ regex_temp = (char *) calloc ((size_t) regex_len + 1, sizeof(char));
memcpy (regex_temp, regex, regex_len);
rlen_temp = regex_len;
cflags_temp = cflags;
if (internal_trace)
- fprintf (stderr, "Compiling %s (len %ld).\n", regex_temp, rlen_temp);
- ppreg_temp = (regex_t *) malloc (rtsize);
+ fprintf (stderr, "Compiling '%s' (len %d).\n", regex_temp, rlen_temp);
+ ppreg_temp = (regex_t *) calloc (1, rtsize);
if (ppreg_temp == NULL)
fatalerror5
("Unable to allocate a pattern register buffer header. ",
@@ -162,25 +178,19 @@
// We will always stuff the _temps in at 0
// and pretend that this was at the last index, so it
// moves everything else further down the list.
- i = CRM_REGEX_CACHESIZE - 1;
- };
-
- // Either way, at this point, the _temp vars contain the new and
- // correct regex information; this information has vacated the slot
- // at index i,
-
+ if (internal_trace)
#ifdef REGEX_CACHE_LINEAR_SEARCH
- // If we're in linear search, we move 0 through i-1 down to 1
- // through i and then we stuff the _temp vars into the [i] cache
- // area. Note that if it was the final slot (at
- // CRM_REGEX_CACHESIZE), we have to free the resources up or
- // we'll leak them.
- //
- // Free the resources first, if needed.
- //
- if (i == CRM_REGEX_CACHESIZE - 1)
- {
+ fputs (" [-> rex cache linear]\n", stderr);
+ // If we're in linear search, we move 0 through i-1 down to 1
+ // through i and then we stuff the _temp vars into the [i] cache
+ // area. Note that if it was the final slot (at
+ // CRM_REGEX_CACHESIZE), we have to free the resources up or
+ // we'll leak them.
+ //
+ // Free the resources first, if needed.
+ //
+ i = CRM_REGEX_CACHESIZE - 1;
if (regex_cache[i].preg != NULL)
{
regfree (regex_cache[i].preg);
@@ -189,41 +199,33 @@
if (regex_cache[i].regex != NULL) free (regex_cache[i].regex);
regex_cache[i].regex = NULL;
regex_cache[i].regex_len = 0;
- };
-
- // If needed, slide 0 through i-1 down to 1..i, to make room
- // at [0]
- //
- if (i != 0)
- {
- for (j = i; j > 0; j--)
+ // If needed, slide 0 through i-1 down to 1..i, to make room
+ // at [0]
+ //
+ if (i != 0)
{
- regex_cache[j].preg = regex_cache[j-1].preg;
- regex_cache[j].regex = regex_cache[j-1].regex;
- regex_cache[j].regex_len = regex_cache[j-1].regex_len;
- regex_cache[j].cflags = regex_cache[j-1].cflags;
- regex_cache[j].status = regex_cache[j-1].status;
- };
- };
-
- // and always stuff the _temps (which are correct) in at [0]
- regex_cache[0].preg = ppreg_temp;
- regex_cache[0].regex = regex_temp;
- regex_cache[0].regex_len = rlen_temp;
- regex_cache[0].status = status_temp;
- regex_cache[0].cflags = cflags_temp;
-#endif
+ for (j = i; j > 0; j--)
+ {
+ regex_cache[j].preg = regex_cache[j-1].preg;
+ regex_cache[j].regex = regex_cache[j-1].regex;
+ regex_cache[j].regex_len = regex_cache[j-1].regex_len;
+ regex_cache[j].cflags = regex_cache[j-1].cflags;
+ regex_cache[j].status = regex_cache[j-1].status;
+ };
+ i=0;
+ };
+ // and always stuff the _temps (which are correct) in at [0]
+#else
#ifdef REGEX_CACHE_RANDOM_ACCESS
- //
- // In a random access system, we just overwrite the single
- // slot that we expected our regex to be in...
-
- // Free the resources first, if needed.
- //
- if (! found_it)
- {
+ fputs (" [-> rex cache random]\n", stderr);
+ //
+ // In a random access system, we just overwrite the single
+ // slot that we expected our regex to be in...
+ //
+ // Free the resources first, if needed.
+ //
if (regex_cache[i].preg != NULL)
{
regfree (regex_cache[i].preg);
@@ -232,25 +234,27 @@
if (regex_cache[i].regex != NULL) free (regex_cache[i].regex);
regex_cache[i].regex = NULL;
regex_cache[i].regex_len = 0;
- };
-
- // and stuff the _temps (which are correct) in at [i]
- regex_cache[i].preg = ppreg_temp;
- regex_cache[i].regex = regex_temp;
- regex_cache[i].regex_len = rlen_temp;
- regex_cache[i].status = status_temp;
- regex_cache[i].cflags = cflags_temp;
-
-#endif
+ // and stuff the _temps (which are correct) in at [i]
+#endif //REGEX_CACHE_RANDOM_ACCESS
+#endif //REGEX_CACHE_LINEAR_SEARCH
+ regex_cache[i].preg = ppreg_temp;
+ regex_cache[i].regex = regex_temp;
+ regex_cache[i].regex_len = rlen_temp;
+ regex_cache[i].status = status_temp;
+ regex_cache[i].cflags = cflags_temp;
+ };
// Just about done. Set up the return preg..
- if (internal_trace)
- fprintf (stderr, " About to return\n");
memcpy (preg, ppreg_temp, rtsize);
- return (regex_cache[i].status);
+ if (internal_trace)
+ fprintf (stderr, " About to return:\n"
+ " preg->re_nsub=%d preg->value=%#x, i=%d, status=%d\n",
+ (int)preg->re_nsub, (int)preg->value, i, regex_cache[i].status);
+ return (regex_cache[i].status);
};
-#endif
+#endif //CRM_REGEX_CACHESIZE == 0
}
+
//
//
// How to do a regex execution from the compiled register