fix: rust alert getter should not modify#5756
Conversation
|
|
||
| int s2n_connection_peek_alert(struct s2n_connection *conn) | ||
| { | ||
| POSIX_ENSURE_REF(conn); |
There was a problem hiding this comment.
Why does this have to be a new API? Can we just fix the behavior of the original s2n_connection_get_alert?
There was a problem hiding this comment.
The C API isn't obviously broken. Unlike the Rust API, it doesn't promise that the connection isn't modified.
Changing its behavior seems like unnecessary risk for existing callers.
There was a problem hiding this comment.
Hmm. I guess we aren't breaking any promised API contract, but I do consider it basically broken. No reasonable application would expect that s2n_connection_get_alert isn't idempotent.
There was a problem hiding this comment.
I don't think it's worth changing the behavior of a very old API. Because the new API is private, this is not a one-way door. If you later decide that the change in behavior is worth potentially breaking customers, that can always be done.
There was a problem hiding this comment.
I think it's very low probability that changing the old C API would break anyone. I mean, either we change the behavior of the existing C API or the existing Rust one.
But I also don't have an issue with just creating a new internal-only API 🤷♀️.
This reverts commit 8800df8.
Goal
Make Connection::alert non-mutating, since it takes a non-mut reference.
Why
Connection::alert can currently violate the borrow rules, which is dangerous. It also blocks reporting the error from the bindings: #5754
How
Add a private alternative "s2n_connection_peek_alert" C method that retrieves the alert without modifying the connection in any way. Swap that method into the bindings.
Callouts
This is arguably a behavior change, if a customer was relying on Connection::alert modifying the connection. But customers shouldn't expect a &self method to modify self, so this change should be way less risky than changing Connection::alert to take a &mut self.
Testing
Added C unit tests.
Related
#5754
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.