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

[BR]: Database repair test fails with sqlite 3.42.0 #3586

Closed
2 of 3 tasks
AdamWill opened this issue Sep 27, 2023 · 7 comments
Closed
2 of 3 tasks

[BR]: Database repair test fails with sqlite 3.42.0 #3586

AdamWill opened this issue Sep 27, 2023 · 7 comments
Labels

Comments

@AdamWill
Copy link

AdamWill commented Sep 27, 2023

  • Fail2Ban version : 1.0.2-8.fc39
  • OS, including release name/version : Fedora 39
  • Fail2Ban installed via OS/distribution mechanisms
  • You have not applied any additional foreign patches to the codebase
  • Some customizations were done to the configuration (provide details below is so)

The issue:

The database recovery test - testRepairDb in fail2ban/tests/databasetestcase.py - does not work with sqlite 3.42.0. The test case expects that the test database file truncated to 14000 bytes is considered corrupt by the sqlite3 Python library, but can be dumped by the sqlite3 console command. With sqlite versions before 3.42.0, that was the case. With 3.42.0, it no longer is. sqlite 3.42.0 fails to dump the test file truncated to 14000 bytes (or any size in 100 byte increments up to 14300 bytes). If I instead truncate the test file to 14400 bytes, sqlite 3.42.0 can dump it, but the sqlite3 Python library does not consider it corrupt. So I have not found a way to make the test work with sqlite 3.42.0.

Steps to reproduce

Run the test case with sqlite 3.42.0 installed. You can also quite easily fiddle around with this at a console: copy out the test database file (fail2ban/tests/files/database_v1.db), truncate it to various lengths with truncate, and examine the output of sqlite3 (testfilename.db) .dump on the truncated versions. At lengths of 14400 bytes and longer, both old and new sqlite can dump it 'perfectly'. At lengths of 14300 bytes and shorter, old sqlite dumps a slightly messed-up version (with the CREATE INDEX logs_jail_path statement truncated and several other statements missing); new sqlite just errors out with ERROR: (11) database disk image is malformed.

Expected behavior

Test case should pass.

Observed behavior

Test case fails.

@AdamWill AdamWill added the bug label Sep 27, 2023
@sebres
Copy link
Contributor

sebres commented Sep 27, 2023

Thx for detailed description and comprehensive analysis.
I'd try to fix that later. Or just ignore unexpected behavior for new sqlite version (the sole purpose of the test was simply the coverage for a repair and recreation in error case).

@AdamWill
Copy link
Author

One way to do it would just be to mock it out more, I guess, if all we want to test is that fail2ban follows the path we want it to...instead of trying to produce a db that actually makes sqlite3 error out, we could just mock out sqlite3 and have the mock raise the desired exception.

@sebres
Copy link
Contributor

sebres commented Sep 27, 2023

we could just mock out sqlite3 and have the mock raise the desired exception.

Well in that case it would be enough to create another test additionally to testRepairDb invoking db.repairDB() directly, e. g. instead of trim-14000 attempt (so it'd basically repair healthy database), then no mockup needed at all.
The idea was to create a broken DB that is a) surely detected as broken; and b) restorable in parts (log positions, bad IPs, etc), so at least some data become loaded from the dump... that (a) we can not guarantee anymore, since with mockup we'd be even unsure whether the expected exception throwed at all (and not changed in newer version to something different); and (b) is even possible at all, and it doesn't expect some switch now.

@AdamWill
Copy link
Author

yeah, it's definitely awkward. the only benefit of the mock approach over just testing it directly would be that it would at least check we catch the currently-expected exception and call repairDB(), I guess.

For downstream I've just disabled the test, I'll leave it up to you to decide what to do upstream as I can't find any great fix!

@sebres
Copy link
Contributor

sebres commented Sep 27, 2023

the only benefit of the mock approach over just testing it directly would be that it would at least check we catch the currently-expected exception and call repairDB(), I guess.

This will anyway happen for truncate-4000 attempt, since there are 2 (one was restorable another not):

for truncSize in (14000, 4000):

@thesamesam
Copy link

For completeness for the bug: see https://sqlite.org/forum/forumpost/c20515073f2ff970.

sebres added a commit to sebres/fail2ban that referenced this issue Dec 14, 2023
@sebres sebres closed this as completed in cabcc9b Dec 14, 2023
@sebres
Copy link
Contributor

sebres commented Dec 14, 2023

Should be fixed in cabcc9b (tested on sqlite3 3.44.x from deb-sid). Thus closed.
I fixed it with a smart conditional mockup (only for reparable case), so should work also for "partially" broken DBs in the same way.

Don't hesitate to ping here, if it still broken on your side.

pbiering pushed a commit to pbiering/fail2ban that referenced this issue Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants