From 56bda9de677893f025adbfe96762031a99c08160 Mon Sep 17 00:00:00 2001 From: liuq19 Date: Thu, 29 Aug 2024 16:23:46 +0800 Subject: [PATCH 1/2] fix: memory leak in to_value --- .github/workflows/ci.yml | 2 -- Cargo.toml | 2 +- src/index.rs | 9 ++++++-- src/serde/de.rs | 6 ++--- src/value/node.rs | 7 ++++-- src/value/ser.rs | 47 ++++++++++++++++++++++++++++++++-------- src/value/shared.rs | 25 --------------------- 7 files changed, 54 insertions(+), 44 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b3324b4..b87db6e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,8 +2,6 @@ name: 'CI' on: pull_request: push: - branches: - - main env: RUST_BACKTRACE: 1 diff --git a/Cargo.toml b/Cargo.toml index e2aac58..7d11028 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,7 +18,7 @@ cfg-if = "1.0" serde = { version = "1.0", features = ["rc", "derive"] } itoa = "1.0" ryu = "1.0" -faststr = "0.2" +faststr = { version = "0.2", features = ["serde"] } smallvec = "1.11" bumpalo = "3.13" bytes = "1.4" diff --git a/src/index.rs b/src/index.rs index 14f90a8..cad5c00 100644 --- a/src/index.rs +++ b/src/index.rs @@ -66,7 +66,7 @@ impl std::ops::IndexMut for Value { /// /// ``` /// # use sonic_rs::json; - /// # + /// # use sonic_rs::object; /// let mut data = json!({ "x": 0, "z": null }); /// /// // replace an existing key @@ -84,11 +84,14 @@ impl std::ops::IndexMut for Value { /// //insert an key in a null value /// data["z"]["zz"] = json!("insert in null"); /// + /// data["z"]["zz1"] = object!{}.into(); + /// + /// /// assert_eq!(data, json!({ /// "x": 1, /// "y": [true, 2, 3], /// "a": { "b": {"c": {"d": true}}}, - /// "z": {"zz": "insert in null"} + /// "z": {"zz": "insert in null", "zz1": {}} /// })); /// ``` #[inline] @@ -182,6 +185,7 @@ macro_rules! impl_str_index { if shared.is_null() { shared = Shared::new_ptr(); *v = Value::new_object(shared, 8); + v.mark_root(); } else { unsafe { std::ptr::write(v, Value::new_object(shared, DEFAULT_OBJ_CAP)) }; } @@ -282,3 +286,4 @@ where (**self).as_key() } } + diff --git a/src/serde/de.rs b/src/serde/de.rs index aa70df6..37beda6 100644 --- a/src/serde/de.rs +++ b/src/serde/de.rs @@ -308,7 +308,7 @@ impl<'de, R: Reader<'de>> Deserializer { { let shared = self.parser.get_shared_inc_count(); let mut val = Value::new_null(shared.data_ptr()); - let val = if self.parser.read.index() == 0 { + let mut val = if self.parser.read.index() == 0 { // get n to check trailing characters in later let n = val.parse_with_padding(self.parser.read.as_u8_slice())?; self.parser.read.eat(n); @@ -321,10 +321,10 @@ impl<'de, R: Reader<'de>> Deserializer { }; // deserialize `Value` must be root node - debug_assert!(val.is_static() || val.is_root()); - if !val.shared_parts().is_null() { + if !val.is_scalar() { std::mem::forget(shared); } + val.mark_root(); let val = ManuallyDrop::new(val); // #Safety diff --git a/src/value/node.rs b/src/value/node.rs index c3933df..444d22f 100644 --- a/src/value/node.rs +++ b/src/value/node.rs @@ -1194,6 +1194,7 @@ impl Value { #[inline] pub(crate) fn grow(&mut self, capacity: usize) { + assert!(self.is_array() || self.is_object()); if self.is_static() { self.mark_shared(Shared::new_ptr()); self.mark_root(); @@ -1367,7 +1368,6 @@ impl Value { parser.parse_value(&mut visitor)?; self.data = visitor.nodes()[0].data; self.meta = visitor.nodes()[0].meta; - self.mark_root(); Ok(parser.read.index()) } @@ -1388,7 +1388,6 @@ impl Value { parser.parse_value_without_padding(&mut visitor)?; self.data = visitor.nodes()[0].data; self.meta = visitor.nodes()[0].meta; - self.mark_root(); Ok(()) } @@ -1416,6 +1415,10 @@ impl Value { unsafe { &*(self.data.sval) } } + pub(crate) fn is_scalar(&self) -> bool { + self.meta.tag() < STRING + } + pub(crate) fn str_len(&self) -> usize { debug_assert!(self.is_str()); let s = unsafe { &*self.data.sval }; diff --git a/src/value/ser.rs b/src/value/ser.rs index b9606fd..0eb6536 100644 --- a/src/value/ser.rs +++ b/src/value/ser.rs @@ -12,7 +12,6 @@ use crate::{ serde::ser::key_must_be_str_or_num, util::arc::Arc, value::node::Value, - JsonValueTrait, }; /// Convert a `T` into `sonic_rs::Value` which can represent any valid JSON data. @@ -64,16 +63,13 @@ pub fn to_value(value: &T) -> Result where T: ?Sized + Serialize, { - let shared = Arc::new(Shared::new()); - let mut value = to_value_in( - unsafe { NonNull::new_unchecked(shared.data_ptr() as *mut _) }, - value, - )?; - if value.is_number() { + let shared = Shared::new_ptr(); + let mut value = to_value_in(unsafe { NonNull::new_unchecked(shared as *mut _) }, value)?; + if value.is_scalar() { value.mark_shared(std::ptr::null()); + std::mem::drop(unsafe { Arc::from_raw(shared) }); } else { value.mark_root(); - std::mem::forget(shared); } Ok(value) } @@ -783,7 +779,9 @@ where mod test { use std::collections::HashMap; - use super::*; + use serde::{Deserialize, Serialize}; + + use crate::{to_value, JsonValueTrait, Value}; #[derive(Debug, serde::Serialize, Hash, Default, Eq, PartialEq)] struct User { @@ -825,4 +823,35 @@ mod test { println!("{:?}", got); assert!(got.is_err()); } + + #[derive(Default, Clone, Serialize, Deserialize, Debug)] + pub struct CommonArgs { + pub app_name: Option, + } + + #[derive(Default, Clone, Serialize, Deserialize, Debug)] + struct Foo { + a: i64, + b: Vec, + } + + #[test] + fn test_to_value2() { + let mut value = Value::default(); + + let args = CommonArgs { + app_name: Some("test".to_string()), + }; + let foo: Foo = + crate::from_str(r#"{"a": 1, "b":[123, "a", {}, [], {"a":null}, ["b"], 1.23]}"#) + .unwrap(); + + value["arg"] = to_value(&args).unwrap_or_default(); + value["bool"] = to_value(&true).unwrap_or_default(); + value["foo"] = to_value(&foo).unwrap_or_default(); + value["arr"] = to_value(&[1, 2, 3]).unwrap_or_default(); + value["arr"][2] = to_value(&args).unwrap_or_default(); + + assert_eq!(value["arr"][2]["app_name"].as_str(), Some("test")); + } } diff --git a/src/value/shared.rs b/src/value/shared.rs index a066e6f..6fd8fda 100644 --- a/src/value/shared.rs +++ b/src/value/shared.rs @@ -116,28 +116,3 @@ impl Drop for SharedCtxGuard { set_shared(self.old); } } - -pub(crate) struct CheckCtxGuard { - is_root: bool, -} - -impl CheckCtxGuard { - /// assign `new_shared` into SharedCtx - pub(crate) fn new() -> Self { - let old = get_shared(); - if old.is_null() { - set_shared(Shared::new_ptr()); - Self { is_root: true } - } else { - Self { is_root: false } - } - } -} - -impl Drop for CheckCtxGuard { - fn drop(&mut self) { - if self.is_root { - set_shared(std::ptr::null()); - } - } -} From 13e52f0bb766c6ada9a3f449db4a9c1d40d69b0e Mon Sep 17 00:00:00 2001 From: liuq19 Date: Thu, 29 Aug 2024 16:53:05 +0800 Subject: [PATCH 2/2] bump 0.3.12 --- Cargo.toml | 2 +- src/value/ser.rs | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 7d11028..de3007a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "sonic-rs" -version = "0.3.11" +version = "0.3.12" authors = ["Volo Team "] edition = "2021" description = "Sonic-rs is a fast Rust JSON library based on SIMD" diff --git a/src/value/ser.rs b/src/value/ser.rs index 0eb6536..b15757f 100644 --- a/src/value/ser.rs +++ b/src/value/ser.rs @@ -781,7 +781,7 @@ mod test { use serde::{Deserialize, Serialize}; - use crate::{to_value, JsonValueTrait, Value}; + use crate::{to_value, Value}; #[derive(Debug, serde::Serialize, Hash, Default, Eq, PartialEq)] struct User { @@ -836,7 +836,10 @@ mod test { } #[test] + #[cfg(not(feature = "arbitrary_precision"))] fn test_to_value2() { + use crate::prelude::*; + let mut value = Value::default(); let args = CommonArgs {