]> granicus.if.org Git - postgresql/commitdiff
Fix regex back-references that are directly quantified with *.
authorTom Lane <tgl@sss.pgh.pa.us>
Mon, 20 Feb 2012 05:52:33 +0000 (00:52 -0500)
committerTom Lane <tgl@sss.pgh.pa.us>
Mon, 20 Feb 2012 05:52:33 +0000 (00:52 -0500)
The syntax "\n*", that is a backref with a * quantifier directly applied
to it, has never worked correctly in Spencer's library.  This has been an
open bug in the Tcl bug tracker since 2005:
https://sourceforge.net/tracker/index.php?func=detail&aid=1115587&group_id=10894&atid=110894

The core of the problem is in parseqatom(), which first changes "\n*" to
"\n+|" and then applies repeat() to the NFA representing the backref atom.
repeat() thinks that any arc leading into its "rp" argument is part of the
sub-NFA to be repeated.  Unfortunately, since parseqatom() already created
the arc that was intended to represent the empty bypass around "\n+", this
arc gets moved too, so that it now leads into the state loop created by
repeat().  Thus, what was supposed to be an "empty" bypass gets turned into
something that represents zero or more repetitions of the NFA representing
the backref atom.  In the original example, in place of
^([bc])\1*$
we now have something that acts like
^([bc])(\1+|[bc]*)$
At runtime, the branch involving the actual backref fails, as it's supposed
to, but then the other branch succeeds anyway.

We could no doubt fix this by some rearrangement of the operations in
parseqatom(), but that code is plenty ugly already, and what's more the
whole business of converting "x*" to "x+|" probably needs to go away to fix
another problem I'll mention in a moment.  Instead, this patch suppresses
the *-conversion when the target is a simple backref atom, leaving the case
of m == 0 to be handled at runtime.  This makes the patch in regcomp.c a
one-liner, at the cost of having to tweak cbrdissect() a little.  In the
event I went a bit further than that and rewrote cbrdissect() to check all
the string-length-related conditions before it starts comparing characters.
It seems a bit stupid to possibly iterate through many copies of an
n-character backreference, only to fail at the end because the target
string's length isn't a multiple of n --- we could have found that out
before starting.  The existing coding could only be a win if integer
division is hugely expensive compared to character comparison, but I don't
know of any modern machine where that might be true.

This does not fix all the problems with quantified back-references.  In
particular, the code is still broken for back-references that appear within
a larger expression that is quantified (so that direct insertion of the
quantification limits into the BACKREF node doesn't apply).  I think fixing
that will take some major surgery on the NFA code, specifically introducing
an explicit iteration node type instead of trying to transform iteration
into concatenation of modified regexps.

Back-patch to all supported branches.  In HEAD, also add a regression test
case for this.  (It may seem a bit silly to create a regression test file
for just one test case; but I'm expecting that we will soon import a whole
bunch of regex regression tests from Tcl, so might as well create the
infrastructure now.)

src/backend/regex/regcomp.c
src/backend/regex/regexec.c
src/test/regress/expected/regex.out [new file with mode: 0644]
src/test/regress/parallel_schedule
src/test/regress/serial_schedule
src/test/regress/sql/regex.sql [new file with mode: 0644]

index 4f9da5b0468d53b997ddb52eb0b5a66a17696318..6b80140e90940b4a348c342090e26fdd0bc82c8f 100644 (file)
@@ -1088,8 +1088,12 @@ parseqatom(struct vars * v,
                NOERR();
        }
 
-       /* it's quantifier time; first, turn x{0,...} into x{1,...}|empty */
-       if (m == 0)
+       /*
+        * It's quantifier time.  If the atom is just a BACKREF, we'll let it deal
+        * with quantifiers internally.  Otherwise, the first step is to turn
+        * x{0,...} into x{1,...}|empty
+        */
+       if (m == 0 && atomtype != BACKREF)
        {
                EMPTYARC(s2, atom->end);        /* the bypass */
                assert(PREF(qprefer) != 0);
index f8e31f8f4ade89d9a4bec2ab8287b4689c1d082b..224da5064b69b9577856b21793bd9a92afb03377 100644 (file)
@@ -720,7 +720,7 @@ cdissect(struct vars * v,
                case '|':                               /* alternation */
                        assert(t->left != NULL);
                        return caltdissect(v, t, begin, end);
-               case 'b':                               /* back ref -- shouldn't be calling us! */
+               case 'b':                               /* back reference */
                        assert(t->left == NULL && t->right == NULL);
                        return cbrdissect(v, t, begin, end);
                case '.':                               /* concatenation */
@@ -962,12 +962,12 @@ cbrdissect(struct vars * v,
                   chr *begin,                  /* beginning of relevant substring */
                   chr *end)                    /* end of same */
 {
-       int                     i;
        int                     n = t->subno;
-       size_t          len;
-       chr                *paren;
+       size_t          numreps;
+       size_t          tlen;
+       size_t          brlen;
+       chr                *brstring;
        chr                *p;
-       chr                *stop;
        int                     min = t->min;
        int                     max = t->max;
 
@@ -978,46 +978,65 @@ cbrdissect(struct vars * v,
 
        MDEBUG(("cbackref n%d %d{%d-%d}\n", t->retry, n, min, max));
 
+       /* get the backreferenced string */
        if (v->pmatch[n].rm_so == -1)
                return REG_NOMATCH;
-       paren = v->start + v->pmatch[n].rm_so;
-       len = v->pmatch[n].rm_eo - v->pmatch[n].rm_so;
+       brstring = v->start + v->pmatch[n].rm_so;
+       brlen = v->pmatch[n].rm_eo - v->pmatch[n].rm_so;
 
        /* no room to maneuver -- retries are pointless */
        if (v->mem[t->retry])
                return REG_NOMATCH;
        v->mem[t->retry] = 1;
 
-       /* special-case zero-length string */
-       if (len == 0)
+       /* special cases for zero-length strings */
+       if (brlen == 0)
+       {
+               /*
+                * matches only if target is zero length, but any number of
+                * repetitions can be considered to be present
+                */
+               if (begin == end && min <= max)
+               {
+                       MDEBUG(("cbackref matched trivially\n"));
+                       return REG_OKAY;
+               }
+               return REG_NOMATCH;
+       }
+       if (begin == end)
        {
-               if (begin == end)
+               /* matches only if zero repetitions are okay */
+               if (min == 0)
+               {
+                       MDEBUG(("cbackref matched trivially\n"));
                        return REG_OKAY;
+               }
                return REG_NOMATCH;
        }
 
-       /* and too-short string */
-       assert(end >= begin);
-       if ((size_t) (end - begin) < len)
+       /*
+        * check target length to see if it could possibly be an allowed number of
+        * repetitions of brstring
+        */
+       assert(end > begin);
+       tlen = end - begin;
+       if (tlen % brlen != 0)
+               return REG_NOMATCH;
+       numreps = tlen / brlen;
+       if (numreps < min || (numreps > max && max != INFINITY))
                return REG_NOMATCH;
-       stop = end - len;
 
-       /* count occurrences */
-       i = 0;
-       for (p = begin; p <= stop && (i < max || max == INFINITY); p += len)
+       /* okay, compare the actual string contents */
+       p = begin;
+       while (numreps-- > 0)
        {
-               if ((*v->g->compare) (paren, p, len) != 0)
-                       break;
-               i++;
+               if ((*v->g->compare) (brstring, p, brlen) != 0)
+                       return REG_NOMATCH;
+               p += brlen;
        }
-       MDEBUG(("cbackref found %d\n", i));
 
-       /* and sort it out */
-       if (p != end)                           /* didn't consume all of it */
-               return REG_NOMATCH;
-       if (min <= i && (i <= max || max == INFINITY))
-               return REG_OKAY;
-       return REG_NOMATCH;                     /* out of range */
+       MDEBUG(("cbackref matched\n"));
+       return REG_OKAY;
 }
 
 /*
diff --git a/src/test/regress/expected/regex.out b/src/test/regress/expected/regex.out
new file mode 100644 (file)
index 0000000..5694908
--- /dev/null
@@ -0,0 +1,36 @@
+--
+-- Regular expression tests
+--
+-- Don't want to have to double backslashes in regexes
+set standard_conforming_strings = on;
+-- Test simple quantified backrefs
+select 'bbbbb' ~ '^([bc])\1*$' as t;
+ t 
+---
+ t
+(1 row)
+
+select 'ccc' ~ '^([bc])\1*$' as t;
+ t 
+---
+ t
+(1 row)
+
+select 'xxx' ~ '^([bc])\1*$' as f;
+ f 
+---
+ f
+(1 row)
+
+select 'bbc' ~ '^([bc])\1*$' as f;
+ f 
+---
+ f
+(1 row)
+
+select 'b' ~ '^([bc])\1*$' as t;
+ t 
+---
+ t
+(1 row)
+
index 862f5b20077a66d80aa0009522d310f875e93487..8852e0a40fc5ca9d0123bdda955e3a04fc71ce0a 100644 (file)
@@ -30,7 +30,7 @@ test: point lseg box path polygon circle date time timetz timestamp timestamptz
 # geometry depends on point, lseg, box, path, polygon and circle
 # horology depends on interval, timetz, timestamp, timestamptz, reltime and abstime
 # ----------
-test: geometry horology oidjoins type_sanity opr_sanity
+test: geometry horology regex oidjoins type_sanity opr_sanity
 
 # ----------
 # These four each depend on the previous one
index 142fc9cf0d1a177fe5881e6065ab343f88ade78d..0bc5df7fe73f59b4868ca881247487eadc83107d 100644 (file)
@@ -42,6 +42,7 @@ test: tstypes
 test: comments
 test: geometry
 test: horology
+test: regex
 test: oidjoins
 test: type_sanity
 test: opr_sanity
diff --git a/src/test/regress/sql/regex.sql b/src/test/regress/sql/regex.sql
new file mode 100644 (file)
index 0000000..242a81e
--- /dev/null
@@ -0,0 +1,13 @@
+--
+-- Regular expression tests
+--
+
+-- Don't want to have to double backslashes in regexes
+set standard_conforming_strings = on;
+
+-- Test simple quantified backrefs
+select 'bbbbb' ~ '^([bc])\1*$' as t;
+select 'ccc' ~ '^([bc])\1*$' as t;
+select 'xxx' ~ '^([bc])\1*$' as f;
+select 'bbc' ~ '^([bc])\1*$' as f;
+select 'b' ~ '^([bc])\1*$' as t;