Skip to content

Fix LIKE/GLOB in GTMSQLite to backtrack after a wildcard - #616

Open
IncognitoQuack wants to merge 1 commit into
google:mainfrom
IncognitoQuack:gtmsqlite-like-glob-backtracking
Open

IncognitoQuack wants to merge 1 commit into
google:mainfrom
IncognitoQuack:gtmsqlite-like-glob-backtracking

Conversation

@IncognitoQuack

Copy link
Copy Markdown

The contract

Foundation/GTMSQLite.h:62-65 says the CFString-based LIKE and GLOB give
better handling of case and composed character sequences, but that
"whereever reasonable, SQLite semantics have been retained", and then
enumerates the specific places they deliberately differ. Greedy matching
is not one of them.

Foundation/GTMSQLite.m:1019-1021 is more explicit still: LikeGlobCompare()
is described as "essentially a reimplementation of patternCompare() in
func.c of the SQLite sources". patternCompare() recurses at the matchAll
character, trying every position at which the rest of the pattern could
start, and that recursion is the whole reason it is correct.

What the code does instead

LikeGlobCompare() walks the pattern once and never reconsiders. Each
pattern element is matched at the first place it fits (CFStringFindWithOptions
/ CFStringFindCharacterFromSet return the earliest match), the walk
commits to that position, and if the rest of the pattern then fails it
reports a non-match. The matchAll ahead of it (% for LIKE, * for GLOB)
is never given the chance to consume more of the target string, so any
match that needs a later occurrence of what follows the wildcard is missed.

'banana' LIKE '%na' is the whole bug in one line: % matches nothing,
na is found at offset 2, the walk ends at offset 4 with banana not
consumed to the end, and the answer is 0. SQLite answers 1, with %
absorbing bana.

Repro

// repro.m
#import <Foundation/Foundation.h>
#import "GTMSQLite.h"

static void Check(GTMSQLiteDatabase *cf, GTMSQLiteDatabase *plain, NSString *sql) {
  int err, value[2] = {-1, -1};
  GTMSQLiteDatabase *dbs[2] = { cf, plain };
  for (int i = 0; i < 2; i++) {
    GTMSQLiteStatement *st = [GTMSQLiteStatement statementWithSQL:sql
                                                       inDatabase:dbs[i]
                                                        errorCode:&err];
    if ([st stepRow] == SQLITE_ROW) value[i] = [st resultInt32AtPosition:0];
    [st finalizeStatement];
  }
  printf("%-42s CFAdditions=%d  SQLite=%d  %s\n", [sql UTF8String],
         value[0], value[1], value[0] == value[1] ? "" : "<-- WRONG");
}

int main(void) {
  @autoreleasepool {
    int err;
    GTMSQLiteDatabase *cf =
        [[GTMSQLiteDatabase alloc] initInMemoryWithCFAdditions:YES
                                                          utf8:YES
                                                     errorCode:&err];
    GTMSQLiteDatabase *plain =
        [[GTMSQLiteDatabase alloc] initInMemoryWithCFAdditions:NO
                                                          utf8:YES
                                                     errorCode:&err];
    Check(cf, plain, @"SELECT 'banana' LIKE '%na';");
    Check(cf, plain, @"SELECT 'abab' LIKE 'a%b';");
    Check(cf, plain, @"SELECT 'aa' LIKE '%a';");
    Check(cf, plain, @"SELECT '/usr/bin/bin' GLOB '*/bin';");
    Check(cf, plain, @"SELECT 'a.com.com' LIKE '%.com';");
    Check(cf, plain, @"SELECT 'banana' GLOB '*a?a';");
  }
  return 0;
}
clang -fno-objc-arc -framework Foundation -lsqlite3 \
  -I Foundation -I DebugUtils -I Sources/Defines/Public \
  repro.m Foundation/GTMSQLite.m -o /tmp/repro && /tmp/repro

At 491a17e:

SELECT 'banana' LIKE '%na';                CFAdditions=0  SQLite=1  <-- WRONG
SELECT 'abab' LIKE 'a%b';                  CFAdditions=0  SQLite=1  <-- WRONG
SELECT 'aa' LIKE '%a';                     CFAdditions=0  SQLite=1  <-- WRONG
SELECT '/usr/bin/bin' GLOB '*/bin';        CFAdditions=0  SQLite=1  <-- WRONG
SELECT 'a.com.com' LIKE '%.com';           CFAdditions=0  SQLite=1  <-- WRONG
SELECT 'banana' GLOB '*a?a';               CFAdditions=0  SQLite=1  <-- WRONG

With this change all six agree with SQLite.

These are not exotic inputs. The condition is just "the text the pattern
looks for after the wildcard occurs more than once in the value", which is
ordinary for suffix queries over paths, hostnames, filenames and any
repetitive text. % is in the pattern of essentially every LIKE query
anyone writes.

The reason it went unnoticed is that the existing coverage cannot see it.
Every row in t1 in testLikeGlobCFAdditions contains what the patterns
search for at most once -- '%bcd' is tested against abcd and bcd,
'ab%d' against abcd and abd -- so the first match is always also the
only match, and the greedy shortcut is indistinguishable from a correct
matcher.

The fix

At each matchAll, record the pattern index just past it and how much of the
target string it has consumed. When the rest of the pattern fails, hand the
matchAll one more composed character sequence and resume the walk from the
recorded pattern index. That is what patternCompare() gets from recursing.

Failures that backtracking provably cannot help stay final:

  • the target string ran out, or the literal run is longer than what is left
    of it -- every position in a retry is at or past where it was, so these
    can only get worse;
  • an unanchored search that found nothing -- it already covered the whole
    rest of the string, so no later starting point can do better.

This is the same pruning SQLite gets from SQLITE_NOWILDCARDMATCH, and it
is what keeps non-matching multi-wildcard patterns cheap. On a successful
unanchored search the skipped region is also handed to the matchAll
immediately, since no element can start there, rather than being rediscovered
one character at a time on each backtrack.

The recorded position only ever moves forward, so a walk backtracks at most
as many times as the target string is long: no exponential blowup.

Evidence

Differential testing against this same database's built-in LIKE/GLOB
(initInMemoryWithCFAdditions:NO), which is the reference implementation
the header says is being followed.

Exhaustive. 555,982 pattern/target pairs, enumerating all patterns up to
length 5 over {a, b, %, _} and {a, b, *, ?}, patterns over alphabets
including [ab], [^a], [a-c] and []a], an ESCAPE-clause suite, a
precomposed/decomposed suite, and both the UTF-8 (Like8/Glob8) and
UTF-16 (Like16/Glob16) entry points:

answers unchanged 536,839 (96.56%)
answers changed 19,143
...changed to what SQLite returns 19,143 (100%)
regressions (was right, now wrong) 0

Before the change every one of those 19,143 was a false negative; there
were no false positives, which is the expected signature of a sound but
incomplete matcher.

Randomised. 7.2M further pairs over random patterns (length 0-8,
including character sets) and random targets (length 0-11), across LIKE,
GLOB and both text encodings: 0 mismatches. 0 mismatches under
-fsanitize=address,undefined as well.

No regression in the existing tests. Replaying every query in
testLikeGlobCFAdditions operation-for-operation, including the
setLikeComparisonOptions: / index create/drop sequence, on both the UTF-8
and UTF-16 databases, produces byte-identical result sets before and after
(72 result lines).

Failing before, passing after. The nine cases added to
testLikeGlobCFAdditions all fail at 491a17e and pass with this change,
on both the UTF-8 and UTF-16 databases. They use a separate table so no
existing expectation moves.

Cost. No measurable change over the 555,982-pair corpus (1.77s / 1.80s
before, 1.78s / 1.80s after). Worst case I could construct is a 64,000-
character target with a pattern like %a that has to walk the whole string:
2.5ms, against 0.24ms for built-in SQLite -- and the old code was returning
the wrong answer. Without the two pruning rules above the same case took
5.1s, which is why they are there.

Caveats

  • I could not run the project's own CI locally. This machine has only
    the Command Line Tools, so there is no XCTest: xcodebuild ... build test
    for GTM.xcodeproj and GTMiPhone.xcodeproj and pod lib lint were all
    impossible. GTMSQLite is not in Package.swift or BUILD, so the Xcode
    workflow is the only one that covers it, and I am relying on CI for it.
    Everything above was produced by compiling Foundation/GTMSQLite.m
    directly with clang into standalone harnesses; the modified
    GTMSQLiteTest.m was checked with clang -fsyntax-only -Wall -Wextra
    against a stub XCTest header, and everything was re-verified by applying
    the patch to a fresh clone of main.
  • The walk keeps one backtrack point -- the most recent matchAll -- rather
    than a stack. That is the standard greedy-wildcard argument: each earlier
    segment is already matched at the earliest position it can be, so moving
    one later never helps. I have exercised that empirically rather than
    proved it here, which is what the exhaustive multi-wildcard suites above
    (patterns up to length 5 over alphabets containing two wildcard
    characters) are for.
  • The change is confined to LikeGlobCompare(). Two other places where the
    CF implementations differ from SQLite are visible in the corpus and are
    deliberately left alone as separate concerns: a malformed character set
    ('a[') raises an error where SQLite returns 0, which the existing tests
    assert; and the UTF-16 entry points reject an empty pattern or empty
    target. Both are byte-for-byte unchanged by this patch.

The CFString implementations of LIKE and GLOB match each pattern
element at the first place it fits and never reconsider. When the rest
of the pattern then fails, the matchAll ("%" for LIKE, "*" for GLOB)
ahead of it is never allowed to consume more of the target string, so
matches that need a later occurrence are reported as non-matches:

  SELECT 'banana' LIKE '%na';   -- CFAdditions 0, SQLite 1
  SELECT 'abab' LIKE 'a%b';     -- CFAdditions 0, SQLite 1

GTMSQLite.h:64 says SQLite semantics are retained wherever reasonable,
and GTMSQLite.m:1020 describes LikeGlobCompare() as a reimplementation
of patternCompare() in func.c, which recurses at matchAll precisely to
get this backtracking. Only false negatives are produced; nothing that
matched before stops matching.

Record the pattern and string position at each matchAll, and on failure
hand it one more composed character sequence and retry the rest of the
pattern from there. Failures that backtracking cannot help -- the
string running out, or an unanchored search that already covered the
whole remainder -- stay final, which is the same pruning SQLite's
SQLITE_NOWILDCARDMATCH gives it. Because the recorded position only
ever moves forward, a walk backtracks at most as many times as the
target string is long.

Diffed against the built-in SQLite LIKE/GLOB over 555,982 exhaustively
enumerated pattern/target pairs: 19,143 change, every one of them from
a wrong answer to the answer SQLite gives, and no answer that was
already correct changes. A further 7.2M randomised pairs agree
exactly. Existing testLikeGlobCFAdditions queries return identical
rows before and after.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant