Fix endless loop bug #979
Fix endless loop bug #979
Conversation
src/util/irep_serialization.cpp
Outdated
| @@ -327,7 +327,7 @@ irep_idt irep_serializationt::read_gb_string(std::istream &in) | |||
| char c; | |||
| size_t length=0; | |||
|
|
|||
| while((c = in.get()) != 0) | |||
| while(in.good() && (c = in.get()) != 0) | |||
There was a problem hiding this comment.
Shouldn't an error be reported if such a string without 0-termination is encountered?
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. |
|
@tautschnig I see your point, I agree, the error should be detected here and execution should not be continued in this situation. |
src/util/irep_serialization.cpp
Outdated
| { | ||
| if(!in.good()) | ||
| { | ||
| throw irep_serialization_errort("gb string is not null-terminated"); |
There was a problem hiding this comment.
It seems Visual Studio doesn't like this. Please just throw the C string, no need for irep_serialization_errort at this point.
There was a problem hiding this comment.
Better to fix the exception constructor than to throw a char star though surely?
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#e14-use-purpose-designed-user-defined-types-as-exceptions-not-built-in-types
There was a problem hiding this comment.
Not until #911 is merged, or you change all other 1583 uses of throw without a user-defined type.
reuk
force-pushed
the
endless-loop
branch
2 times, most recently
from
d2b6aac to
aee7578
Compare
src/util/irep_serialization.cpp
Outdated
| size_t length=0; | ||
|
|
||
| while((c = in.get()) != 0) | ||
| while(char c = in.get()) |
src/util/irep_serialization.cpp
Outdated
| @@ -14,6 +14,10 @@ Date: May 2007 | |||
| #include "irep_serialization.h" | |||
| #include "string_hash.h" | |||
|
|
|||
| irep_serialization_errort::irep_serialization_errort( | |||
| const std::string &error) | |||
| : std::runtime_error(error) {} | |||
reuk
force-pushed
the
endless-loop
branch
4 times, most recently
from
ea3429d to
d7afb39
Compare
src/util/irep_serialization.cpp
Outdated
| size_t length=0; | ||
|
|
||
| while((c=static_cast<char>(in.get()))!=0) | ||
| { | ||
| if(!in.good()) | ||
| { | ||
| throw irep_serialization_errort("gb string is not null-terminated"); |
There was a problem hiding this comment.
'gb' is not very user friendly.
Throw if input format is incorrect
|
@peterschrammel Could you please review whether you are happy with the current state? |
|
@peterschrammel ping? |
Fixes #543