jsonbuilder: attempt to handle memory allocation errors - v5#8916
jsonbuilder: attempt to handle memory allocation errors - v5#8916jasonish wants to merge 2 commits into
Conversation
Some very minor changes to formatting.
Use try_reserve before growing the internal buffer, and the internal state vector. This allows allocation errors to be caught and an error returned instead of just aborting the process. Ticket: OISF#6057
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #8916 +/- ##
==========================================
+ Coverage 82.30% 82.34% +0.04%
==========================================
Files 969 969
Lines 273335 273376 +41
==========================================
+ Hits 224961 225111 +150
+ Misses 48374 48265 -109
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 14035 |
| } | ||
|
|
||
| fn encode_base64(&mut self, val: &[u8]) -> Result<&mut Self, JsonError> { | ||
| let encoded_len = ((val.len() / 4) * 3) + 2; |
There was a problem hiding this comment.
This looks wrong : if val.len() == 1, this gives 2 instead of 4
This should rather be 4*((val.len()+2)/3), no ?
| /// than building onto the buffer. | ||
| /// | ||
| /// TODO: Revisit this, would be nice to build directly onto the | ||
| /// existing buffer. |
There was a problem hiding this comment.
I think this is worth revisiting this part.
Note that std::str::from_utf8 error case seems unreachable (as you encode the string correctly directly), so you should be able to push onto the self.buf directly
catenacyber
left a comment
catenacyber
left a comment
There was a problem hiding this comment.
Still the base64 length looks suspicious + encode_string could/should be simplified
I feel touching encode_string here is out of scope, unless the changes added within scope are wrong. |
I think you can have simpler changes with refactoring the |
|
Encoded length fixed here: #8927 |
3 participants
Ticket: https://redmine.openinfosecfoundation.org/issues/6057
Previous PR: #8855
Changes from last PR: