Skip to content

Conversation

@reuk
Copy link
Contributor

@reuk reuk commented May 31, 2017

Fixes #543

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't an error be reported if such a string without 0-termination is encountered?

Copy link
Contributor

@mgudemann mgudemann left a comment

Choose a reason for hiding this comment

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

tested, original problem is solved with this
error reporting would be nice, but this is from fuzzed binary GOTO, so it is very unlikely to appear in the wild

@tautschnig
Copy link
Collaborator

error reporting would be nice, but this is from fuzzed binary GOTO, so it is very unlikely to appear in the wild

This patch would move the status from denial-of-service (not great, but low risk of exposing private data) to possibly arbitrary crashes at a later stage that may include buffer overflows. Error detection on invalid input MUST happen as early as possible.

@mgudemann
Copy link
Contributor

@tautschnig I see your point, I agree, the error should be detected here and execution should not be continued in this situation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems Visual Studio doesn't like this. Please just throw the C string, no need for irep_serialization_errort at this point.

Copy link
Contributor Author

@reuk reuk Jun 2, 2017

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not until #911 is merged, or you change all other 1583 uses of throw without a user-defined type.

@reuk reuk force-pushed the endless-loop branch 2 times, most recently from d2b6aac to aee7578 Compare June 2, 2017 21:36
Copy link
Member

Choose a reason for hiding this comment

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

spacing around =

Copy link
Member

Choose a reason for hiding this comment

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

coding style for :

@tautschnig tautschnig changed the base branch from master to develop August 22, 2017 12:23
@reuk reuk force-pushed the endless-loop branch 4 times, most recently from ea3429d to d7afb39 Compare August 23, 2017 12:25
Copy link
Member

Choose a reason for hiding this comment

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

'gb' is not very user friendly.

Throw if input format is incorrect
@tautschnig tautschnig assigned peterschrammel and unassigned reuk Dec 1, 2017
@tautschnig
Copy link
Collaborator

@peterschrammel Could you please review whether you are happy with the current state?

@tautschnig
Copy link
Collaborator

@peterschrammel ping?

@reuk reuk closed this Apr 9, 2018
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.

4 participants