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

normalize s2n_stuffer and s2n_blob #4944

Open
jmayclin opened this issue Dec 3, 2024 · 1 comment
Open

normalize s2n_stuffer and s2n_blob #4944

jmayclin opened this issue Dec 3, 2024 · 1 comment

Comments

@jmayclin
Copy link
Contributor

jmayclin commented Dec 3, 2024

Problem:

s2n_stuffer and s2n_blob have confusingly non-normalized fields.

s2n_stuffer defines growable and allocd fields.
s2n_blob also defines growable and allocated fields.

The semantics of this are very confusing. What would it mean for an allocated stuffer to have a non-allocated blob? Isn't "allocated-ness" purely a property of the blob?

Solution:

After some investigation with #4943, my understanding is as follows.

  1. s2n_blob should remove the growable field. This is extensively documented in the above PR, but essentially, any blob that is alloced is growable. If it isn't alloced, it isn't growable.
  2. s2n_stuffer should remove the allocd field. I was playing around with some unit tests, and I was able to make this change with breaking any unit tests. But trying to remove growable from the stuffer broke lots of things, which makes sense.

stuffer "growable" semantics are used to deliberately fail writes that are too large. This is a valuable behavior that we want to keep. (but this logic should be documented in the code 🙂)

Requirements / Acceptance Criteria:

  1. remove growable from s2n_blob.
  2. remove alloced from s2n_stuffer.

And preferably add some comments about why the fields exist and what they accomplish.

@jmayclin
Copy link
Contributor Author

jmayclin commented Dec 5, 2024

After some further research, this goal will be a little complicated

remove growable from s2n_blob.

I think at a minimum the field should be renamed to be something more accurate, but currently it's necessary to track whether a 0 sized blob is allocated or a reference.

I can not think of a valid case where we have a zero sized allocation? So I would prefer to just explicitly make that a part of the s2n_blob contract, but that will require some further investigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants