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

fix(bindings): correct poll_flush implementation #4859

Merged
merged 3 commits into from
Oct 26, 2024
Merged

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Oct 23, 2024

Resolved issues:

resolves #4803
If the issue doesn't make sense, check out the test I added to s2n_send_test.

Description of changes:

I fixed flush by allowing the bindings to call s2n_flush directly.

Callouts

Alternatively, we could "fix" s2n_send so that it can be used to flush. The problem is that the s2n_send behavior is deliberate and intended as a safety mechanism to detect an application calling s2n_send wrong. If we remove that safety mechanism, we make it slightly more likely that applications misuse s2n_send. I'm not sure just how much benefit customers get from this particular safety mechanism, but I'm also not sure how much benefit customers really get from flush :)

Another possible alternative would be to just make poll_flush a no-op. It's not super useful as-is since it can only clear the send buffer and not actually retry a full send. I don't know of any current valid use cases for poll_flush. But a no-op method seems stranger than a not particularly useful method :/ Maybe we can instead remove poll_flush before we make the bindings 1.0.0?

Testing:

The bug is kind of hard to imagine, so I wrote up a test to demonstrate why s2n_send isn't a replacement for s2n_flush. That should also prevent us from breaking s2n_flush in the same way, although that seems unlikely.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Oct 23, 2024
Comment on lines +18 to +19
#include <s2n.h>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I vaguely remember that we didn't want to include "s2n.h" in this header, but I don't remember why and I couldn't find the discussion in the original PR.

However, I think this is necessary now because s2n_flush has s2n_blocked_status *blocked as an argument. s2n_blocked_status is currently an anonymous enum we typedef:

s2n-tls/api/s2n.h

Lines 2164 to 2172 in 9cd117f

*/
typedef enum {
S2N_NOT_BLOCKED = 0,
S2N_BLOCKED_ON_READ,
S2N_BLOCKED_ON_WRITE,
S2N_BLOCKED_ON_APPLICATION_INPUT,
S2N_BLOCKED_ON_EARLY_DATA,
} s2n_blocked_status;

The only way I could think to fix this without including "s2n.h" would be to switch s2n_blocked_status from an anonymous enum to a named enum, like in this example. But I'm not a fan of having two different ways to refer to the same enum.

@lrstewart lrstewart marked this pull request as ready for review October 23, 2024 04:16
tls/s2n_internal.h Show resolved Hide resolved
bindings/rust/s2n-tls/src/connection.rs Show resolved Hide resolved
bindings/rust/s2n-tls/src/connection.rs Show resolved Hide resolved
@lrstewart lrstewart enabled auto-merge (squash) October 25, 2024 16:38
@lrstewart lrstewart merged commit 778cd84 into aws:main Oct 26, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

s2n-tls poll_flush doesn't work
3 participants