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

Be robust towards input containing null bytes (regression from #98) #131

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

hartwork
Copy link
Collaborator

Regression from #98, may need coordination with #115.

@hartwork hartwork requested a review from edgar-bonet November 28, 2023 14:45
@hartwork hartwork changed the title ttyplot.c: Be robust towards input containing null bytes Be robust towards input containing null bytes Nov 28, 2023
@edgar-bonet
Copy link
Collaborator

may need coordination with #115

It indeed does. Your fix seems perfect to me. Should I add this to refactor-input? Or maybe let you rebase this PR once #115 gets merged?

@hartwork
Copy link
Collaborator Author

It indeed does. Your fix seems perfect to me. Should I add this to refactor-input? Or maybe let you rebase this PR once #115 gets merged?

@edgar-bonet I can rebase it onto #115 after merge but if acceptible I would prefer the other way around: merging #131 here first and rebasing #115 onto this after. It would reduce the number of things in flight, get the fix out earlier, and I'm imagining the conflict resolution to be easy here, if not I'd definitely volunteer to do the resolution. It would help us separate bugfixes from the refactoring, which I consider a good thing if feasible.

@edgar-bonet
Copy link
Collaborator

It would reduce the number of things in flight

That's exactly why I would like to merge #115 first. It has been “in flight” for quite a while now. But if you insist in getting this first, OK, go ahead.

@hartwork
Copy link
Collaborator Author

@edgar-bonet you gave #129 to me where order was more important to me, I don't insist, let's do your #115 before my #131 here then, happy to rebase after, it's still a good deal for me 👍

@hartwork hartwork changed the title Be robust towards input containing null bytes [After #115] Be robust towards input containing null bytes Nov 29, 2023
@hartwork hartwork force-pushed the fix-handling-of-null-bytes branch from 37174fa to 2eff5fe Compare November 29, 2023 21:55
@hartwork hartwork added this to the 1.6.0 milestone Nov 29, 2023
@hartwork hartwork changed the title [After #115] Be robust towards input containing null bytes [After #115] Be robust towards input containing null bytes (regression from #98) Nov 30, 2023
> # { python3 -c 'print(chr(0))'; seq 20; } | ./ttyplot
@hartwork hartwork force-pushed the fix-handling-of-null-bytes branch from 2eff5fe to e53366d Compare November 30, 2023 19:34
@hartwork
Copy link
Collaborator Author

hartwork commented Nov 30, 2023

@edgar-bonet rebased and ready for re-review now

@hartwork hartwork requested a review from edgar-bonet November 30, 2023 19:34
@hartwork hartwork changed the title [After #115] Be robust towards input containing null bytes (regression from #98) Be robust towards input containing null bytes (regression from #98) Nov 30, 2023
Copy link
Collaborator

@edgar-bonet edgar-bonet left a comment

Choose a reason for hiding this comment

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

Is there a reason for doing this in handle_input_event rather than handle_input_data? I seems to me it belongs to the latter. For instance, handle_input_event has so far no notion of “valid delimiter”: as far as it is concerned, the buffer is just a chunk of arbitrary binary data. Making sense of those bytes is the job of handle_input_data.

@hartwork
Copy link
Collaborator Author

hartwork commented Nov 30, 2023

Is there a reason for doing this in handle_input_event rather than handle_input_data? I seems to me it belongs to the latter. For instance, handle_input_event has so far no notion of “valid delimiter”: as far as it is concerned, the buffer is just a chunk of arbitrary binary data. Making sense of those bytes is the job of handle_input_data.

@edgar-bonet handle_input_data does not have knowledge of how much data was just read — variable bytes_read — and so we would need to null-check the whole buffer every time rather than just the bytes read this time, correct? Also one could argue that handle_input_data should get safe to process data and that null bytes could be not considered safe. The last part is a question of view, the former is a question of performance, at least in part. What do you think?

Copy link
Collaborator

@edgar-bonet edgar-bonet left a comment

Choose a reason for hiding this comment

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

The last part is a question of view, the former is a question of performance, at least in part. What do you think?

OK, it does make sense.

@hartwork hartwork merged commit 8d42fff into tenox7:development Nov 30, 2023
7 checks passed
@hartwork hartwork deleted the fix-handling-of-null-bytes branch November 30, 2023 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants