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

Process sequence of pending packets #580

Merged
merged 19 commits into from
Sep 20, 2022

Conversation

Lagovas
Copy link
Collaborator

@Lagovas Lagovas commented Sep 19, 2022

What was the problem? DB drivers can send set of packets in one TCP packet. And usually, they are related to each other and look like Parse + Bind + Describe + Execute + Sync. Driver registers query in Parse, then binds data, then asks to describe types, executes query and commit state. Acra registers PreparedStatement on ParseComplete packet that DB responds to driver after validation and acceptance. Bind packet relates to Parse packet because it should specify statement name for which it sends values.
In this flow acra do next: remembers Parse packet and save it as pendingParse. Then handle Bind packet, loads finds pendignParse and link them together.

Previously we expected that one set of packets in TCP packet contains only one packet with a query like SimpleQuery or Parse packet. And when Acra receives ParsePacket, it resets state, forgets about Bind and other kind of packets - https://github.com/cossacklabs/acra/blob/master/decryptor/postgresql/protocol.go#L121

But drivers not limited in the number of query packets. They can send several of them in one set. And they should be processed in same order. If acra got 2 Describe packets from driver to database and then receives 2 RowDescription responses from the database, it should link first RowDescription to the first Describe, second one to the second.
So Acra should not store only last packet, but should all of them in same order.

In this PR was added new component that stores all pendingPackets and store them separately according to the type, works as a queue. And was refactored logic with working with pending packets. Now it removes item from the queue, not nilify a field.

Due to limitations of python's drivers that don't allow to compile such set of packets to write regression test, were added unit-test with dumped packets that reproduces the problem.

Checklist

Copy link
Collaborator Author

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

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

@Zhaars , added nil checks and replaced panic with returning error

decryptor/postgresql/pg_decryptor.go Show resolved Hide resolved
@Lagovas Lagovas requested a review from Zhaars September 20, 2022 00:17
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.

Cool, thanks.

@Lagovas Lagovas merged commit de03470 into master Sep 20, 2022
@Lagovas Lagovas deleted the lagovas/T2663-process-sequence-pending-packets branch September 20, 2022 15:46
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.

None yet

2 participants