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

Remove sqlite patch from sqlite-replication as it's incompatible #72997

Closed

Conversation

@otwieracz
Copy link
Contributor

otwieracz commented Nov 7, 2019

Things done

Removed CVE patch from sqlite-replication package as it wasn't applying and it seems unrelevant.

In master there is already sqlite-3.30 which does not have this patch at all, so it's only relevant to 19.09.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@lordcirth

This comment has been minimized.

Copy link
Contributor

lordcirth commented Nov 7, 2019

The version being built here = "3.27.2+replication3", and NVD seems to believe that CVE-2019-16168 still exists in 3.27.2:

https://nvd.nist.gov/vuln/search/results?adv_search=true&cpe_version=cpe%3a%2fa%3asqlite%3asqlite%3a3.27.2

I can't find any evidence that this was patched in canonical's repo.

@otwieracz

This comment has been minimized.

Copy link
Contributor Author

otwieracz commented Nov 7, 2019

OK, so this was probably my misunderstanding about sqlite3 build system. I will try to address it, as I agree that this patch should be indeed applicable.

@d-goldin

This comment has been minimized.

Copy link
Contributor

d-goldin commented Nov 7, 2019

Thanks for poking. I see, indeed we have not tested sqlite-replication (personally, I was unaware of this one), but it's not surprising that the patch does not apply to this version - due to the amalgamation approach they are taking, chances are higher that such a patch will fail even between minor versions. Maybe it applies cleanly for 3.28+replication - but then again this would still introduce a version update to stable - I guess it would have been good for them to not diverge in the first place. Let me know if you need any help.

@otwieracz

This comment has been minimized.

Copy link
Contributor Author

otwieracz commented Nov 7, 2019

The problem is that sqlite package is using already autoconfigured sources. sqlite-replication however uses raw sources from Github.

With -replication approach, there's no sqlite3.c by the time patchPhase is executed. And due to "--disable-amalgamation" option passed to ./configure I don't think it ever gets created.

@d-goldin

This comment has been minimized.

Copy link
Contributor

d-goldin commented Nov 7, 2019

Yeah, I see. Its a bit iffy to tie those two together in such a way, but maybe worth trying with enabled amalgamation - or overwrite patches with the original upstream patch. I'll keep an eye on this PR and can spend a bit of time helping out if you run into issues.

@otwieracz

This comment has been minimized.

Copy link
Contributor Author

otwieracz commented Nov 7, 2019

Honestly, I thought it will be "easy fix", because I assumed that sqlite-replication is vastly different and does not have sqlite3.c at all.

However, as it's getting more complex, I won't have time (at this moment, at least) to dive deeper into this issue. It will be best for original author to fix it, as he (or she) will be best aware about why choices like no-amalgamations were made.

@d-goldin

This comment has been minimized.

Copy link
Contributor

d-goldin commented Nov 7, 2019

Alright, then I'll propose a fix in a little bit and will reference this PR.

@otwieracz

This comment has been minimized.

Copy link
Contributor Author

otwieracz commented Nov 7, 2019

Thank you!

@d-goldin d-goldin mentioned this pull request Nov 7, 2019
5 of 10 tasks complete
@veprbl veprbl closed this Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.