jsonbuilder: attempt to handle memory allocation errors - v2 by jasonish · Pull Request #8855 · OISF/suricata

jasonish · 2023-05-09T22:16:30Z

Previous PR: #8847
Ticket: https://redmine.openinfosecfoundation.org/issues/6057

Attempt to handle all "normal" memory allocation errors in JsonBuilder where
the Rust APIs allow us. This primarily means growing the underlying String
buffer as data is added.

As the JsonBuilder already used a Result for most methods, most users of
JsonBuilder already handle errors.

Changes from last PR:

I think this is complete now.

Things to investigate:

Box::new failing. This might not be catchable. Box::try_new is an
experimental API that will allow us to handle this better.

Some very minor changes to formatting.

Convert "new_object" and "new_array" functions that return a Result and use "try_reserve" to allocate the amount of data requested. This should allow memory allocation errors to be detected and handled in a Rust-ful matter without resorting to catching a panic. Ticket: OISF#6057

Provide a wrapper around "push" and "push_str" on the internal buffer that will "try_reserve" data before growing in an attempt to handle memory allocation errors. Ticket: OISF#6057

Convert encode_string() to use try_reserve to catch memory allocation errors.

As base64 encoding is done by a 3rd party library directly into the buffer and there is no "try" style function, first reserve enough data to fit the complete base64 encoded string (and then some).

Required minor updates to users of the base64 crate.

codecov · 2023-05-10T00:44:43Z

Codecov Report

Merging #8855 (4a8f3e4) into master (9a4231d) will increase coverage by 0.01%.
The diff coverage is 87.30%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8855      +/-   ##
==========================================
+ Coverage   82.25%   82.26%   +0.01%     
==========================================
  Files         969      969              
  Lines      273176   273207      +31     
==========================================
+ Hits       224689   224752      +63     
+ Misses      48487    48455      -32

Flag	Coverage Δ
fuzzcorpus	`64.52% <79.08%> (+0.05%)`	⬆️
suricata-verify	`60.34% <82.35%> (-0.01%)`	⬇️
unittests	`62.98% <63.95%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

suricata-qa · 2023-05-10T00:45:04Z

WARNING:

field	baseline	test	%
	SURI_TLPW1_stats_chk
.tcp.rst	113274	144976	127.99%

Pipeline 13706

catenacyber · 2023-05-12T07:17:08Z

Have you tried running jsonbuilder unit tests with an allocation failure framework ? (ie a LD_PRELOAD overload malloc to fail, running the tests X times, X being the number of allocations, and making each allocation fail)

jasonish · 2023-05-12T14:51:16Z

Have you tried running jsonbuilder unit tests with an allocation failure framework ? (ie a LD_PRELOAD overload malloc to fail, running the tests X times, X being the number of allocations, and making each allocation fail)

No. Any existing examples of this?

catenacyber · 2023-05-14T14:09:50Z

Like https://github.com/tavianator/oomify but there may be others (and it is quite easy to recode)

catenacyber · 2023-05-14T14:27:35Z

Jason, I had created this https://redmine.openinfosecfoundation.org/issues/5851 for your info

jasonish · 2023-05-16T15:11:39Z

Like https://github.com/tavianator/oomify but there may be others (and it is quite easy to recode)

Will look. I did test in a low memory Docker container, unfortunately the Linux OOM kicks in before Rust is able to detect an allocation error.

So while this may test if the code works OK, I wonder if it would actually ever error out in production on Linux, or just get killed by the kernel anyways.

jasonish · 2023-05-16T16:04:38Z

Like https://github.com/tavianator/oomify but there may be others (and it is quite easy to recode)

Quick test with oomify does show the reserve calls failing in a catchable way..

[jason@t470 …/suricata/jsonbuilder-alloc/rust]  jsonbuilder-alloc/v3 ❯ oomify -n20 ./target/debug/test-jsonbuilder
size: 1
size: 2
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Memory', src/bin/test-jsonbuilder.rs:10:48
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
oomify: alloc 20: ./target/debug/test-jsonbuilder exited with status 101

The panic there is just a result on my unwrap, so that shows that if Rust isn't killed by the kernel, the error is rippled back to the application.

catenacyber · 2023-05-16T16:14:54Z

Where can I see test-jsonbuilder.rs source ?

jasonish · 2023-05-16T16:17:07Z

Where can I see test-jsonbuilder.rs source ?

Its just in my working directory, not something suitable to check in, so no PR...

use suricata::jsonbuilder::JsonBuilder;

fn main() {
    let mut size = 1;

    for i in 1..8 {
        println!("size: {}", size);
        let mut jb = JsonBuilder::try_new_object().unwrap();
        let mut buf = vec![0x61; size];
        jb.set_string_from_bytes("test", &buf).unwrap();
        size = size * 2;
    }
}

catenacyber · 2023-05-18T18:05:56Z

    /// than building onto the buffer.
    ///
-    /// TODO: Allocate memory in a fallible way.
+    /// TODO: Revisit this, would be nice to build directly onto the


Is it to do in this PR ?

In a way, that one todo had to be removed, and another way came to mind at the same time.

catenacyber · 2023-05-18T18:08:12Z

+    fn encode_base64(&mut self, val: &[u8]) -> Result<&mut Self, JsonError> {
+        // Before base64 encoding, make sure enough space is reserved.
+        if self.buf.capacity() < self.buf.len() + val.len() * 2 {
+            self.buf.try_reserve(val.len() * 2)?;


base64 is rather ((val.len()+2)/3)*4 no ?

like one byte gets encoded as 4 bytes

Ah yeah, mines too small. base64 has a function for this, will just use that.

catenacyber · 2023-05-18T18:11:18Z

Do not you need to check also vec![State::None, State::ObjectFirst] ?

catenacyber · 2023-05-18T18:11:59Z

And make push_state fallible

catenacyber · 2023-05-18T18:15:19Z

self.buf.push_str(&val.to_string()); in append_uint seems to be missing as well

catenacyber

Some nits to address ;-)

I wonder if some cases should do a big reserve before multiple push_str like open_array doing

        self.push('"')?;
        self.push_str(key)?;
        self.push_str("\":[")?;

jasonish · 2023-05-18T20:05:59Z

Some nits to address ;-)

I wonder if some cases should do a big reserve before multiple push_str like open_array doing
        self.push('"')?;
        self.push_str(key)?;
        self.push_str("\":[")?;

Maybe. But as the reserve behind these does 4k size, the first that needs it will allocate enough. My first iteration did try to estimate the size and reserve at once, but it did seem a little error prone. But not harm to estimate it, reserve once so the reserve shouldn't be triggered again during the call into jsonbuilder. Not sure if it'll have a noticeable affect other than fail a little earlier if it was going to fail.

jasonish · 2023-05-19T13:56:56Z

Commented addressed (well most of them) here: #8901

jasonish added 6 commits May 9, 2023 10:13

jsonbuilder: rustfmt

4ec17c5

Some very minor changes to formatting.

jsonbuilder: check allocation when growing buffer

26a7d5c

Provide a wrapper around "push" and "push_str" on the internal buffer that will "try_reserve" data before growing in an attempt to handle memory allocation errors. Ticket: OISF#6057

jsonbuilder: memory allocation handling in encode_string

4d59f9b

Convert encode_string() to use try_reserve to catch memory allocation errors.

jsonbuilder: reserve space before base64 encoding

6063d29

As base64 encoding is done by a 3rd party library directly into the buffer and there is no "try" style function, first reserve enough data to fit the complete base64 encoded string (and then some).

rust: update base64 crate to 0.21

4a8f3e4

Required minor updates to users of the base64 crate.

jasonish requested review from catenacyber and jufajardini as code owners May 9, 2023 22:16

jasonish mentioned this pull request May 9, 2023

(draft) jsonbuilder: better handling of memory allocation errors - v1 #8847

Closed

jasonish mentioned this pull request May 10, 2023

[draft] Example of catching panics in Rust #8757

Closed

catenacyber mentioned this pull request May 17, 2023

fuzz: make targets more resistant to allocation failures #8891

Merged

catenacyber self-assigned this May 18, 2023

catenacyber reviewed May 18, 2023

View reviewed changes

Comment thread rust/src/jsonbuilder.rs

catenacyber reviewed May 18, 2023

View reviewed changes

catenacyber requested changes May 18, 2023

View reviewed changes

jasonish mentioned this pull request May 19, 2023

jsonbuilder: attempt to handle memory allocation errors - v3 #8901

Closed

jasonish closed this May 19, 2023

jasonish mentioned this pull request May 24, 2023

jsonbuilder: attempt to handle memory allocation errors - v5 #8916

Closed

jasonish deleted the jsonbuilder-alloc/v2 branch May 31, 2023 21:29

Conversation

jasonish commented May 9, 2023

Uh oh!

codecov Bot commented May 10, 2023

Codecov Report

Uh oh!

suricata-qa commented May 10, 2023

Uh oh!

catenacyber commented May 12, 2023

Uh oh!

jasonish commented May 12, 2023

Uh oh!

catenacyber commented May 14, 2023

Uh oh!

catenacyber commented May 14, 2023

Uh oh!

jasonish commented May 16, 2023

Uh oh!

jasonish commented May 16, 2023

Uh oh!

catenacyber commented May 16, 2023

Uh oh!

jasonish commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

catenacyber May 18, 2023

Choose a reason for hiding this comment

Uh oh!

jasonish May 18, 2023

Choose a reason for hiding this comment

Uh oh!

catenacyber May 18, 2023

Choose a reason for hiding this comment

Uh oh!

catenacyber May 18, 2023

Choose a reason for hiding this comment

Uh oh!

jasonish May 18, 2023

Choose a reason for hiding this comment

Uh oh!

catenacyber commented May 18, 2023

Uh oh!

catenacyber commented May 18, 2023

Uh oh!

catenacyber commented May 18, 2023

Uh oh!

catenacyber left a comment

Choose a reason for hiding this comment

Uh oh!

jasonish commented May 18, 2023

Uh oh!

jasonish commented May 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

jasonish commented May 16, 2023 •

edited

Loading