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

Stream loss in association establishment when association acceptor sends more PDUs in quick succession #589

Open
Enet4 opened this issue Nov 2, 2024 · 3 comments
Labels
A-lib Area: library bug This is a bug C-ul Crate: dicom-ul

Comments

@Enet4
Copy link
Owner

Enet4 commented Nov 2, 2024

Stream establishment depends on get_client_pdu, which parses the PDU data from an in-memory buffer. When establishing an association, a temporary buffer is created and immediately dropped.

This usually does not pose an issue because an association acceptance is usually followed by the SCP waiting for data from the SCU. However, if for some reason the SCP tries to send an A-ASSOCIATE-AC plus one or more PDUs in quick succession, this can make the association client fetch more bytes than the ones necessary for reading a single PDU.

This is a regression from #542, which was mitigated by #590 so that it would work on other scenarios.

@Enet4 Enet4 changed the title Soundness issue in association establishment when association acceptor sends more PDUs in quick succession Stream loss in association establishment when association acceptor sends more PDUs in quick succession Nov 2, 2024
@Enet4 Enet4 added bug This is a bug A-lib Area: library C-ul Crate: dicom-ul labels Nov 2, 2024
@naterichman
Copy link
Contributor

Getting back here after a while, sorry for introducing this regression! What is left to be done here, can I help?

@Enet4
Copy link
Owner Author

Enet4 commented Feb 4, 2025

This is still an issue, though I'm not sure how much of an impact this has. So far I haven't stumbled upon any failures in association establishment.

I am honestly a bit more worried about get_client_pdu_async, which needs its temporary byte buffer plucked out onto the caller otherwise it can lose data between calls. This is something that should probably earn more attention, as it could put most async code at risk. Please let me know if you are available and willing to take either issue @naterichman . :)

@naterichman
Copy link
Contributor

I will try to take a look at both at some point! I'm pretty busy right now so it might not be for a month or so, but I think I'll first try to make a test to consistently reproduce that for both sync and async code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lib Area: library bug This is a bug C-ul Crate: dicom-ul
Projects
None yet
Development

No branches or pull requests

2 participants