Fix read overflow #1812
Fix read overflow #1812
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1812 +/- ##
==========================================
+ Coverage 80.40% 82.97% +2.57%
==========================================
Files 278 284 +6
Lines 44998 48868 +3870
Branches 10245 10375 +130
==========================================
+ Hits 36180 40550 +4370
- Misses 7033 7213 +180
+ Partials 1785 1105 -680
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Packet++/src/RawPacket.cpp
Outdated
| if ((size_t)m_RawDataLen + dataToInsertLen < (size_t)m_RawDataLen) | ||
| { | ||
| PCPP_LOG_ERROR("RawPacket::insertData: dataToInsertLen causes overflow"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Can we throw a std::length_error instead of just returning with a error message? The error message will not indicate the error condition to the caller.
There was a problem hiding this comment.
Will throwing an exception prevent the application from crashing? 🤔
This method was called in BgpLayer so we should probably think how to prevent it from being called with malformed data
There was a problem hiding this comment.
You want a crash if overflow happens. Or at least you want an exception thrown to indicate the failure of the operation, and if the caller doesn't handle it up the call stack, then to crash. It isn't really possible to recover from the situation as the system has reached the limit of how much data can be stored inside a packet.
Silently continuing with just an error message in the log is worse as the program will assume the operation went fine and produce corrupted data down the line. This is exactly the type of improbable situation the exception mechanism was made to handle.
There was a problem hiding this comment.
I totally agree, however I don't think throwing an exception will resolve the OSS-Fuzz issue. We should probably throw an exception and fix the issue that caused it in BgpLayer
There was a problem hiding this comment.
@Marti2203 thanks for working on it!
Catching exceptions is pretty expensive in C++. I'd avoid using try..catch and instead check the sizes and prevent calling extendLayer is we know it'd fail
There was a problem hiding this comment.
Ah! Will rework it then, I know exceptions are expensive but did not understand initially how you wanted it.
There was a problem hiding this comment.
Sure no worries 👍
We try to minimize the use of exceptions in critical paths like packet parsing.
There was a problem hiding this comment.
Generally, I find exceptions are fine as long as they aren't used to dictate common control flow. The current mainstream compilers use an architecture that has a "zero cost" try-catch blocks if the exception isn't thrown. The tradeoff is a significant overhead if an exception is thrown, so you only want them thrown for rare events.
The current usage seems ok to me? How often are overflows expected to happen?
There was a problem hiding this comment.
In most cases, I wouldn't expect an exception to be thrown, however:
- In applications that use PcapPlusPlus with an arbitrary payload (either to test malformed packet handling or because they generate a random payload), an exception can be thrown and we would like to avoid the cost of it
- Most (or all?) of our critical path code doesn't catch exceptions and try to handle edge cases so they don't end up with exceptions. I think we should do the same here for consistency
Fixes the following issue detected by OSS-Fuzz:
This fix was generated by CodeRover-S, an LLM agent for fixing security vulnerabilities. More details can be found at: