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

Verify that usage after error does not lead to undefined behavior #21

Open
Marcono1234 opened this issue Oct 26, 2023 · 3 comments
Open
Labels
enhancement New feature or request internal Issue or pull request which is internal and has no user-visible effect

Comments

@Marcono1234
Copy link
Owner

Marcono1234 commented Oct 26, 2023

Problem solved by the enhancement

Both JsonReader and JsonWriter say that you should abort using them on most errors, but they also say (emphasis mine):

Trying to call any reader methods afterwards can lead to unspecified behavior, such as errors, panics or incorrect data. However, no undefined behavior occurs.

It should be verified that this is actually the case. In version 0.3.0 that should be the case because it does not use unsafe; however in the future the utf8.rs module might use unsafe to avoid UTF-8 validation where the data should be validated already.

The question is however if this assumption that no undefined behavior will occur is still correct if users continue using JsonReader or JsonWriter after an error occurred.


Either way, the recommendation that JsonReader and JsonWriter should not be used after an error was returned still applies.

Enhancement description

Not necessarily a code change, but mainly reviewing the code

Code changes which are guaranteed to prevent any possible undefined behavior would of course be even better though.

@Marcono1234 Marcono1234 added the enhancement New feature or request label Oct 26, 2023
@kskalski
Copy link

kskalski commented Nov 1, 2023

There would be value in hardening the code to actually maintain correct state in case of some errors. For example io error WouldBlock opens up possibility for non-blocking operation or using implementation of Read that is filled outside of the calls performed during json reader processing time.

@Marcono1234
Copy link
Owner Author

Marcono1234 commented Nov 2, 2023

This GitHub issue here is mainly intended to verify that undefined behavior is not possible, but it would still allow the reader to malfunction such as returning incorrect results.

Maybe it would be worth having a separate issue for what you are proposing, something like "Support recovering from IO errors" or similar (assuming the IO error kind allows recovering, but that is not relevant to Struson itself), if I understood you correctly.

I assume it would be possible to support that, but it might be a bit tricky, because the reader must not change its state until it successfully processed a value or name, and it must keep the partial value in its buffer (and must not override it by accident when refilling the buffer).

@kskalski
Copy link

kskalski commented Nov 3, 2023

Noted, I agree that recovery from IO errors is a feature on its own. It could be useful in some cases I suppose, when downstream caller can also preserve the state correctly and re-try continuing processing later (this is not what seems to be practical with using serde for my use case, so I will just leave it as a remark here).

@Marcono1234 Marcono1234 added the internal Issue or pull request which is internal and has no user-visible effect label Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request internal Issue or pull request which is internal and has no user-visible effect
Projects
None yet
Development

No branches or pull requests

2 participants