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

[T2580] Fix handling null values in mysql #543

Merged

Conversation

Lagovas
Copy link
Collaborator

@Lagovas Lagovas commented May 23, 2022

On decryption our envelope_detector returned []byte{} on nil values instead leaving nil. MySQL encoded differently empty strings and NULL values. NULL values should have 0xfb value. So on Acra side when it receive null value as 0xfb then it changed to empty array and encode it as LengthEncoded String or Integer that are incorrect.

So in this PR removed processing null data in crypto envelope. If it will be empty array, it will be returned as empty array too.

Checklist

tests/test.py Outdated Show resolved Hide resolved
@@ -196,6 +196,9 @@ func (wrapper *OldContainerDetectorWrapper) OnCryptoEnvelope(ctx context.Context

// OnColumn callback which finds serializedContainer or AcraStruct/AcraBlock for backward compatibility
func (wrapper *OldContainerDetectorWrapper) OnColumn(ctx context.Context, inBuffer []byte) (context.Context, []byte, error) {
if inBuffer == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lagovas Can we place this check on the level of EnvelopeDetector? We are already doing some inBuffer checking here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we return there inBuffer as is. Without allocating new buffer. If inBuffer in OnColumn will be nil, it will returned as is due to if len(inBuffer) < SerializedContainerMinSize

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it;

Copy link
Contributor

@G1gg1L3s G1gg1L3s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

@Zhaars Zhaars self-requested a review May 24, 2022 06:45
Copy link
Contributor

@Zhaars Zhaars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Lagovas Lagovas merged commit 665d654 into cossacklabs:master May 24, 2022
@Lagovas Lagovas deleted the lagovas/mysql-fix-encoding-null-values branch May 24, 2022 21:37
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.

3 participants