From aefa671eca2d514bfa685f385c5fa3e1a13305f4 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Sat, 25 Oct 2025 10:31:04 +0200 Subject: [PATCH] Fix concurrent usage of JSON::Coder#dump Fix: https://github.com/rails/rails/commit/90616277e3d8fc46c9cf35d6a7470ff1ea0092f7#r168784389 Because the `depth` counter is inside `JSON::State` it can't be used concurrently, and in case of a circular reference the counter may be left at the max value. The depth counter should be moved outside `JSON_Generator_State` and into `struct generate_json_data`, but it's a larger refactor. In the meantime, `JSON::Coder` calls `State#generate_new` so I changed that method so that it first copy the state on the stack. --- CHANGES.md | 3 +++ ext/json/ext/generator/generator.c | 39 +++++++++++++++++++++++---- java/src/json/ext/GeneratorState.java | 24 ++++++++++++++--- lib/json/truffle_ruby/generator.rb | 6 ++++- test/json/json_coder_test.rb | 10 +++++++ 5 files changed, 73 insertions(+), 9 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 1f0b7873..4434d815 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -2,6 +2,9 @@ ### Unreleased +* Fix `JSON::Coder` to have one dedicated depth counter per invocation. + After encountering a circular reference in `JSON::Coder#dump`, any further `#dump` call would raise `JSON::NestingError`. + ### 2025-10-07 (2.15.1) * Fix incorrect escaping in the JRuby extension when encoding shared strings. diff --git a/ext/json/ext/generator/generator.c b/ext/json/ext/generator/generator.c index 6a38cc60..f3f27b29 100644 --- a/ext/json/ext/generator/generator.c +++ b/ext/json/ext/generator/generator.c @@ -1124,7 +1124,7 @@ static inline long increase_depth(struct generate_json_data *data) JSON_Generator_State *state = data->state; long depth = ++state->depth; if (RB_UNLIKELY(depth > state->max_nesting && state->max_nesting)) { - rb_raise(eNestingError, "nesting of %ld is too deep", --state->depth); + rb_raise(eNestingError, "nesting of %ld is too deep. Did you try to serialize objects with circular references?", --state->depth); } return depth; } @@ -1491,10 +1491,39 @@ static VALUE cState_generate(int argc, VALUE *argv, VALUE self) rb_check_arity(argc, 1, 2); VALUE obj = argv[0]; VALUE io = argc > 1 ? argv[1] : Qnil; - VALUE result = cState_partial_generate(self, obj, generate_json, io); + return cState_partial_generate(self, obj, generate_json, io); +} + +static VALUE cState_generate_new(int argc, VALUE *argv, VALUE self) +{ + rb_check_arity(argc, 1, 2); + VALUE obj = argv[0]; + VALUE io = argc > 1 ? argv[1] : Qnil; + GET_STATE(self); - (void)state; - return result; + + JSON_Generator_State new_state; + MEMCPY(&new_state, state, JSON_Generator_State, 1); + + // FIXME: depth shouldn't be part of JSON_Generator_State, as that prevents it from being used concurrently. + new_state.depth = 0; + + char stack_buffer[FBUFFER_STACK_SIZE]; + FBuffer buffer = { + .io = RTEST(io) ? io : Qfalse, + }; + fbuffer_stack_init(&buffer, state->buffer_initial_length, stack_buffer, FBUFFER_STACK_SIZE); + + struct generate_json_data data = { + .buffer = &buffer, + .vstate = Qfalse, + .state = &new_state, + .obj = obj, + .func = generate_json + }; + rb_rescue(generate_json_try, (VALUE)&data, generate_json_rescue, (VALUE)&data); + + return fbuffer_finalize(&buffer); } static VALUE cState_initialize(int argc, VALUE *argv, VALUE self) @@ -2072,7 +2101,7 @@ void Init_generator(void) rb_define_method(cState, "buffer_initial_length", cState_buffer_initial_length, 0); rb_define_method(cState, "buffer_initial_length=", cState_buffer_initial_length_set, 1); rb_define_method(cState, "generate", cState_generate, -1); - rb_define_alias(cState, "generate_new", "generate"); // :nodoc: + rb_define_method(cState, "generate_new", cState_generate_new, -1); // :nodoc: rb_define_private_method(cState, "allow_duplicate_key?", cState_allow_duplicate_key_p, 0); diff --git a/java/src/json/ext/GeneratorState.java b/java/src/json/ext/GeneratorState.java index f9e8234e..fd1478b0 100644 --- a/java/src/json/ext/GeneratorState.java +++ b/java/src/json/ext/GeneratorState.java @@ -231,7 +231,7 @@ public IRubyObject initialize_copy(ThreadContext context, IRubyObject vOrig) { * the result. If no valid JSON document can be created this method raises * a GeneratorError exception. */ - @JRubyMethod(alias="generate_new") + @JRubyMethod public IRubyObject generate(ThreadContext context, IRubyObject obj, IRubyObject io) { IRubyObject result = Generator.generateJson(context, obj, this, io); RuntimeInfo info = RuntimeInfo.forRuntime(context.runtime); @@ -251,11 +251,25 @@ public IRubyObject generate(ThreadContext context, IRubyObject obj, IRubyObject return resultString; } - @JRubyMethod(alias="generate_new") + @JRubyMethod public IRubyObject generate(ThreadContext context, IRubyObject obj) { return generate(context, obj, context.nil); } + @JRubyMethod + public IRubyObject generate_new(ThreadContext context, IRubyObject obj, IRubyObject io) { + GeneratorState newState = (GeneratorState)dup(); + newState.resetDepth(); + return newState.generate(context, obj, io); + } + + @JRubyMethod + public IRubyObject generate_new(ThreadContext context, IRubyObject obj) { + GeneratorState newState = (GeneratorState)dup(); + newState.resetDepth(); + return newState.generate(context, obj, context.nil); + } + @JRubyMethod(name="[]") public IRubyObject op_aref(ThreadContext context, IRubyObject vName) { String name = vName.asJavaString(); @@ -478,6 +492,10 @@ public IRubyObject depth_set(IRubyObject vDepth) { return vDepth; } + public void resetDepth() { + depth = 0; + } + private ByteList prepareByteList(ThreadContext context, IRubyObject value) { RubyString str = value.convertToString(); if (str.getEncoding() != UTF8Encoding.INSTANCE) { @@ -610,7 +628,7 @@ public int decreaseDepth() { private void checkMaxNesting(ThreadContext context) { if (maxNesting != 0 && depth > maxNesting) { depth--; - throw Utils.newException(context, Utils.M_NESTING_ERROR, "nesting of " + depth + " is too deep"); + throw Utils.newException(context, Utils.M_NESTING_ERROR, "nesting of " + depth + " is too deep. Did you try to serialize objects with circular references?"); } } } diff --git a/lib/json/truffle_ruby/generator.rb b/lib/json/truffle_ruby/generator.rb index 2d01ad1b..703211cb 100644 --- a/lib/json/truffle_ruby/generator.rb +++ b/lib/json/truffle_ruby/generator.rb @@ -212,7 +212,7 @@ def check_max_nesting # :nodoc: return if @max_nesting.zero? current_nesting = depth + 1 current_nesting > @max_nesting and - raise NestingError, "nesting of #{current_nesting} is too deep" + raise NestingError, "nesting of #{current_nesting} is too deep. Did you try to serialize objects with circular references?" end # Returns true, if circular data structures are checked, @@ -347,6 +347,10 @@ def generate_new(obj, anIO = nil) # :nodoc: dup.generate(obj, anIO) end + private def initialize_copy(_orig) + @depth = 0 + end + # Handles @allow_nan, @buffer_initial_length, other ivars must be the default value (see above) private def generate_json(obj, buf) case obj diff --git a/test/json/json_coder_test.rb b/test/json/json_coder_test.rb index c7248353..fb9d7b30 100755 --- a/test/json/json_coder_test.rb +++ b/test/json/json_coder_test.rb @@ -66,4 +66,14 @@ def test_json_coder_dump_NaN_or_Infinity_loop end assert_include error.message, "NaN not allowed in JSON" end + + def test_nesting_recovery + coder = JSON::Coder.new + ary = [] + ary << ary + assert_raise JSON::NestingError do + coder.dump(ary) + end + assert_equal '{"a":1}', coder.dump({ a: 1 }) + end end