Re: [PATCH v2 3/4] userdiff: fix typo in ruby and python word regexes

From: Thomas Rast <trast@student.ethz.ch>
Date: Dec 19, 2010, 02:10 UTC
Message-ID: <201012190310.43308.trast@student.ethz.ch>
In-Reply-To: <7vr5deg5fp.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 11 quoted lines

> Thomas Rast <trast@student.ethz.ch> writes:
> 
> > Both had an unclosed ] that ruined the safeguard against not matching
> > a non-space char.
> 
> Thanks.
> 
> Couldn't we have found this without your "sanity check" patch?  Are we
> ignoring error returns from regcomp() in some codepath, or is it just that
> we are catching them but our test suite lacks ruby and python test
> vectors?

We lacked test vectors, but we still couldn't have caught it. We do check for errors in regcomp():

	if (o->word_regex) {
		ecbdata.diff_words->word_regex = (regex_t *)
			xmalloc(sizeof(regex_t));
		if (regcomp(ecbdata.diff_words->word_regex,
				o->word_regex,
				REG_EXTENDED | REG_NEWLINE))
			die ("Invalid regular expression: %s",
					o->word_regex);
	}

(Now that I'm seeing this and comparing with regcomp(3), we should actually report regerror() as part of the error message.)

The problem is that the pattern is still valid. Consider that it was a final two arms to the regex:

-        "|[^[:space:]|[\x80-\xff]+"),
+        "|[^[:space:]]|[\x80-\xff]+"),

In the preimage, it parses like so:

  | [^
      [:space:]|[\x80-\xff
     ]+

That is, the third [ is part of the (negated) character class. So the only problem is with | or [ characters in the input. Any other non-space character is part of the class.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch