Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve SQLite3 parser #25

Merged
merged 3 commits into from
Apr 25, 2024

Conversation

JSCU-CNI
Copy link
Contributor

This PR fixes the following two issues in the SQLite3 parser:

  1. Make column expression and string regexes lazy instead of greedy, which adds compatibility for column definitions such as CHARACTER(int) and VARCHAR(int) (https://www.sqlite.org/datatype3.html#affinity_name_examples)
  2. Do not throw InvalidPageNumber if the database header does not contain a page_count.

@Schamper
Copy link
Member

Schamper commented Apr 25, 2024

Can you elaborate on the page_count change? Maybe why it's necessary or how you encountered it? Is it a corrupt database, or a valid database that can be created?

I've tried to reproduce with sqlite3 empty.db "VACUUM;" but that creates a database with page_count = 1 (and also has another issue with a 0 text_encoding which should also be fixed).

@JSCU-CNI
Copy link
Contributor Author

Is it a corrupt database, or a valid database that can be created?

Certain old versions of SQLite3 do not set or update the header correctly. Bottom line is that other sqlite3 libraries do not seem to care if the page_count is zero. Reading through the dissect implementation it seems that the page_count is not really used either when iterating over tables.

It seems silly to throw a read out of bounds exception if we do not know the bounds, so that is why we added the page_count > 0 check.

@Schamper
Copy link
Member

Can you add a comment explaining as such? ;)

@Schamper Schamper merged commit 067f05d into fox-it:main Apr 25, 2024
16 checks passed
@JSCU-CNI JSCU-CNI deleted the fix/sqlite3-column-tokenizer-regex branch April 25, 2024 14:30
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.

2 participants