Skip to content

Fixes Exceptions and permanent Hang when parsing bad input, Adds bad input tests #197

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

Merged
merged 10 commits into from
Apr 12, 2021

Conversation

kulgg
Copy link
Contributor

@kulgg kulgg commented Apr 10, 2021

Fixes #196 and #198

@kulgg kulgg changed the title Fixes #196 Adds BadInput Tests, Fixes Exceptions and permanent Hang Apr 10, 2021
@kulgg kulgg changed the title Adds BadInput Tests, Fixes Exceptions and permanent Hang Adds bad input Tests, fixes Exceptions and permanent Hang Apr 10, 2021
@kulgg kulgg changed the title Adds bad input Tests, fixes Exceptions and permanent Hang Fixes Exceptions and permanent Hang when parsing bad input, Adds bad input tests Apr 10, 2021
@github-actions

This comment has been minimized.

Copy link
Member

@milkshakeuk milkshakeuk left a comment

Choose a reason for hiding this comment

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

Looks mostly fine, just a few comments, left me know what you think.

@github-actions

This comment has been minimized.

@kulgg
Copy link
Contributor Author

kulgg commented Apr 11, 2021

Fixes #198

@github-actions

This comment has been minimized.

@milkshakeuk
Copy link
Member

@JlKmn did you see some of the other comments?

@kulgg
Copy link
Contributor Author

kulgg commented Apr 11, 2021

@milkshakeuk I think i answered to all of them, what am I missing?

@milkshakeuk
Copy link
Member

milkshakeuk commented Apr 11, 2021

@JlKmn

#197 (comment)

#197 (comment)

@kulgg
Copy link
Contributor Author

kulgg commented Apr 11, 2021

@milkshakeuk Very strange they didn't show for me on brave and edge. I can hover over the link and see some of the comment but can't click on it. They also don't show for me in Files changed with comments?

@kulgg
Copy link
Contributor Author

kulgg commented Apr 11, 2021

Can you comment them again

Copy link
Member

@milkshakeuk milkshakeuk left a comment

Choose a reason for hiding this comment

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

@JlKmn
Looks like I needed to "finish the review" which is why you maybe couldn't see the comments.

@github-actions

This comment has been minimized.

@milkshakeuk
Copy link
Member

@JlKmn
Okay looks good, the final thing would if I could ask you to rebase your forked master branch from main (NHapi) master branch, currently your master branch is 2 behind and 10 ahead.

@kulgg
Copy link
Contributor Author

kulgg commented Apr 12, 2021

@milkshakeuk should be fine now right?

@github-actions
Copy link

Unit Test Results

    5 files    56 suites   8s ⏱️
416 tests 414 ✔️ 2 💤 0 ❌
830 runs  827 ✔️ 3 💤 0 ❌

Results for commit fbde865.

@milkshakeuk
Copy link
Member

@JlKmn Yes thanks!

@milkshakeuk milkshakeuk merged commit d1b5e80 into nHapiNET:master Apr 12, 2021
@milkshakeuk milkshakeuk linked an issue Apr 12, 2021 that may be closed by this pull request
@milkshakeuk
Copy link
Member

@JlKmn I have rebased my branch from master so it now includes your changes however now some of my tests are failing because of changes I have made to ParserBase so may need your support to amend those BadInputTests

@kulgg
Copy link
Contributor Author

kulgg commented Apr 12, 2021

@milkshakeuk ok how do we go about that

@milkshakeuk
Copy link
Member

@JlKmn Once my PR is ready ill push up a branch which you could fork and have a look at?

@kulgg
Copy link
Contributor Author

kulgg commented Apr 12, 2021

@milkshakeuk ok

@milkshakeuk
Copy link
Member

@JlKmn ok here is my branch.

@kulgg
Copy link
Contributor Author

kulgg commented Apr 13, 2021

@milkshakeuk ok i will look at it soon

@kulgg
Copy link
Contributor Author

kulgg commented Apr 14, 2021

@milkshakeuk I looked at it and the changes in exceptions for the tests happen because you rewrote the Parse function in ParserBase.cs that gets called for every BadInput test. One fix would be to extend the one test to accept EncodingNotSupported because it is a HL7 child exception. Maybe move the others to the Gracefully test?
But yeah the Parse function is very different making the tests change behavior

@milkshakeuk
Copy link
Member

@JlKmn ok I'll look to make those changes.
Once they are in could you run your fuzzing exercize against the new PipeParser?

@kulgg
Copy link
Contributor Author

kulgg commented Apr 15, 2021

@milkshakeuk Yes

@milkshakeuk
Copy link
Member

@JlKmn ok the PR has been merged.

@kulgg
Copy link
Contributor Author

kulgg commented Apr 15, 2021

@milkshakeuk I will do a Fuzzing run on the new Parser tonight, we'll see tomorrow

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.

BadInputs cause StackOverflow/Access Violation Bad Inputs cause unhandled exceptions and permanent hang
2 participants