Skip to content

Commit 3d38d43

Browse files
authored
free the engine's C string copy in to_string_lossy (#1311)
* free the engine's C string copy in to_string_lossy * return an error from to_string_lossy when the conversion fails
1 parent b49fa3b commit 3d38d43

4 files changed

Lines changed: 69 additions & 9 deletions

File tree

‎crates/javy/CHANGELOG.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,14 @@ Versioning](https://semver.org/spec/v2.0.0.html).
1515
offset and length against the backing `ArrayBuffer` and return an error for an
1616
out-of-range or overflowing range instead of panicking. `readSync` no longer
1717
builds its mutable slice through raw FFI.
18+
- `to_string_lossy` now returns `rquickjs::Result<String>` and returns
19+
`Error::Exception` if the engine runs out of memory converting the string.
20+
21+
### Fixed
22+
23+
- `to_string_lossy` no longer leaks the engine-allocated copy of the string it
24+
converts, and no longer constructs a slice from a null pointer when that
25+
allocation fails.
1826

1927
## [8.1.0] - 2026-07-30
2028

‎crates/javy/src/apis/text_encoding/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ fn encode(args: Args<'_>) -> Result<Value<'_>> {
106106
// This is the fast path.
107107
// The string is already utf-8.
108108
.to_string()
109-
.unwrap_or_else(|error| to_string_lossy(&cx, js_string, error));
109+
.or_else(|error| to_string_lossy(&cx, js_string, error))?;
110110

111111
Ok(TypedArray::new(cx, encoded.into_bytes())?
112112
.as_value()

‎crates/javy/src/lib.rs‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -153,11 +153,25 @@ pub fn to_js_error(cx: Ctx, e: Error) -> JSError {
153153

154154
/// Converts the JavaScript value to a string, replacing any invalid UTF-8 sequences with the
155155
/// Unicode replacement character (U+FFFD).
156+
///
157+
/// Returns [`JSError::Exception`] if the engine runs out of memory while
158+
/// converting the string; the out-of-memory exception is left pending on the
159+
/// context.
156160
// TODO: Upstream this?
157-
pub fn to_string_lossy<'js>(cx: &Ctx<'js>, string: &JSString<'js>, error: JSError) -> String {
161+
pub fn to_string_lossy<'js>(
162+
cx: &Ctx<'js>,
163+
string: &JSString<'js>,
164+
error: JSError,
165+
) -> rquickjs::Result<String> {
158166
let mut len: qjs::size_t = 0;
159167
let ptr =
160168
unsafe { qjs::JS_ToCStringLen2(cx.as_raw().as_ptr(), &mut len, string.as_raw(), false) };
169+
if ptr.is_null() {
170+
// The conversion only fails when the engine cannot allocate the
171+
// buffer for the converted copy, in which case it has thrown an
172+
// out-of-memory exception.
173+
return Err(JSError::Exception);
174+
}
161175
let buffer = unsafe { std::slice::from_raw_parts(ptr as *const u8, len as usize) };
162176

163177
// The error here *must* be a Utf8 error; the `JSString::to_string()` may
@@ -203,7 +217,10 @@ pub fn to_string_lossy<'js>(cx: &Ctx<'js>, string: &JSString<'js>, error: JSErro
203217
Err(e) => utf8_error = e,
204218
}
205219
}
206-
res
220+
// The engine allocates a new reference-counted string for the converted
221+
// copy on every call; release it now that `res` owns the decoded contents.
222+
unsafe { qjs::JS_FreeCString(cx.as_raw().as_ptr(), ptr) };
223+
Ok(res)
207224
}
208225

209226
/// Retrieves the string representation of a JavaScript value.
@@ -212,17 +229,16 @@ pub fn val_to_string<'js>(this: &Ctx<'js>, val: Value<'js>) -> Result<String> {
212229
if let Some(description) = symbol.description()?.into_string() {
213230
let description = description
214231
.to_string()
215-
.unwrap_or_else(|e| to_string_lossy(this, &description, e));
232+
.or_else(|e| to_string_lossy(this, &description, e))?;
216233
Ok(format!("Symbol({description})"))
217234
} else {
218235
Ok("Symbol()".into())
219236
}
220237
} else {
221-
let stringified = <convert::Coerced<JSString>>::from_js(this, val).map(|string| {
222-
string
223-
.to_string()
224-
.unwrap_or_else(|e| to_string_lossy(this, &string.0, e))
225-
})?;
238+
let string = <convert::Coerced<JSString>>::from_js(this, val)?;
239+
let stringified = string
240+
.to_string()
241+
.or_else(|e| to_string_lossy(this, &string.0, e))?;
226242
Ok(stringified)
227243
}
228244
}

‎crates/javy/tests/misc.rs‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@
22
use anyhow::Result;
33
#[cfg(feature = "json")]
44
use javy::{Config, Runtime, quickjs::context::EvalOptions};
5+
use javy::{
6+
quickjs::{Context, Runtime as QuickJSRuntime, String as JSString},
7+
to_string_lossy,
8+
};
59

610
#[cfg(feature = "json")]
711
#[test]
@@ -40,3 +44,35 @@ fn json_stringify_cycle_checks() -> Result<()> {
4044

4145
Ok(())
4246
}
47+
48+
#[test]
49+
fn to_string_lossy_frees_engine_cstring() -> anyhow::Result<()> {
50+
let runtime = QuickJSRuntime::new()?;
51+
let context = Context::full(&runtime)?;
52+
53+
let baseline = runtime.memory_usage().malloc_size;
54+
context.with(|this| {
55+
// A long string ending in a lone surrogate takes the lossy path.
56+
let string: JSString = this.eval(r#""a".repeat(65536) + "\uD800""#)?;
57+
for _ in 0..64 {
58+
let error = string
59+
.to_string()
60+
.expect_err("conversion should fail on the lone surrogate");
61+
let lossy = to_string_lossy(&this, &string, error)?;
62+
assert!(lossy.ends_with(char::REPLACEMENT_CHARACTER));
63+
}
64+
Ok::<_, anyhow::Error>(())
65+
})?;
66+
67+
// Each call makes the engine allocate a ~192 KiB UTF-8 copy of the string.
68+
// The copies are reference counted, so if `to_string_lossy` does not free
69+
// them, not even a GC pass reclaims them and the ~12 MiB they amount to
70+
// here stays allocated.
71+
runtime.run_gc();
72+
let growth = runtime.memory_usage().malloc_size - baseline;
73+
assert!(
74+
growth < 1024 * 1024,
75+
"engine heap grew by {growth} bytes; the C strings were not freed"
76+
);
77+
Ok(())
78+
}

0 commit comments

Comments
 (0)