Fix bug with i32 encoding #542
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This also brings new dependency - psycopg v3. This is a lot better version of psycopg2, with new supported features, like extended and prepared queries. In our case, we are interested in additional type parameters in a Parse packets, which psyco specifies by default.
This place is responsible for normalizing values into the one form before encrypting and putting into database. Also, this is where the problem is.
By default, acra converts all integers into int64, then serializes it into a textual representation and then encrypts. But transforming binary int32 into int64 looked like: bytes -> uint32 -> int64 -> text Which is wrong, because if in case of negative int32, the sign was dropped at uint32 and then it wasn't restored after converting into int64. Fix this by properly converting int32 directly to int64, so the compiler can insert correct instruction that can handle negative sign: bytes -> uint32 -> int32 -> int64 -> text
This will prevent out-of-bounds panics.
Lagovas
approved these changes
May 22, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a bug with int32 encoding. Bug was triggered with negative int32 values, when we insert them and then select with text encoding.
There were two ways of fixing this bug:
Fix insertion (which is implemented here)
Acra normalizes integers into int64 text before encrypting and sending into the database. The normalization happens here.
For an int32 the whole process looked like:
But the issue is, expanding from uint32 into int64 happens signless. So, if the integer is
-2061435539the whole process would looks like:This PR adds explicit conversion from uint32 into int32, so the compiler can omit correct instruction for expansions with a negative sign:
But, this will not work with the values encrypted before this change, the bug can still remain in the database of some users
Fix selection
This is the fix I was first thinking of, and even implemented that, so if you believe this way should be used instead/with the previous one, it can be easily applied.
When we encode integer values in postgres, we create
IntValueobject, defined here. In consists of an int value and a string one. String is used as an optimization, because the whole int value is produced from string, so in case of text encoding, we can reuse that string.But because strings are signless for int32 (see reasons above), this doesn't work. The way to correct is to explicitly calculate string representation based on an integer value.
I don't know which variant is better, so I'm here for your opinion.
Checklist
Public API has proper documentation in the Acra documentation site or has PR on documentation repositorywith new changes
CHANGELOG.md is updated (in case of notable or breaking changes)Benchmark results are attached (if applicable)Example projects and code samples are up-to-date (in case of API changes)