Repository navigation
RCBC-586: Raise backend argument errors without skipping C++ frames - #280
Conversation
Motivation
----------
Backend methods build argument errors with rb_sprintf("%+" PRIsVALUE),
which calls the argument's inspect. An inspect that raises leaves by
longjmp, skipping the destructors of every C++ frame up to Ruby. The
arguments already copied leak: a mutate_in with a 4 MiB spec followed
by such a counter delta leaks the spec on each call.
Modifications
-------------
cb_sprintf() runs rb_vsprintf() under cb_protect. A raise in inspect
or to_s continues as ruby_jump, and the frames unwind before cb_method
resumes it. Every rb_sprintf() call in the backend uses it.
Results
-------
The exception raised by inspect reaches the caller unchanged, and the
copied arguments are freed. A test bounds RSS over repeated mutate_in
calls whose counter delta has a raising inspect.
Motivation ---------- NUM2ULL, NUM2LL, NUM2UINT, FIX2INT and FIX2UINT raise RangeError for a value outside the target type. They raise by longjmp from inside the backend method, skipping the C++ frames up to Ruby. A mutate_in with a 4 MiB spec followed by a counter delta of 2**64 leaks the spec on each call. Modifications ------------- cb_num2ull(), cb_num2ll(), cb_num2uint() and cb_num2int() run the conversion under cb_protect. Methods registered through cb_method, and the helpers they call, use them. The HDR histogram methods and Backend.leb128_encode are registered without cb_method and keep the macros. Results ------- An out-of-range Integer raises the same RangeError, and the frames unwind first. A test bounds RSS over repeated mutate_in calls with an out-of-range counter delta.
Motivation ---------- Backend methods registered through cb_method called rb_raise for an empty spec list, an empty ID/CAS tuple list, an unknown DNS SRV service and an unknown view namespace. rb_raise leaves by longjmp and skips the C++ destructors in the method, the cluster reference among them. The namespace check also passed the Symbol's ID where the message format expects an object. A static Symbol put a number in the message, and a dynamic Symbol crashed the process. Modifications ------------- These checks throw ruby_exception, and cb_method raises it after the frames unwind. The namespace message formats the Symbol itself. Results ------- view_query and the view index manager raise ArgumentError naming an unknown namespace, static or dynamic, and a test covers both. The other checks raise the same errors as before.
Motivation ---------- Backend methods registered through cb_method checked argument types with Check_Type, which raises TypeError by longjmp and skips the C++ destructors in the method, the cluster reference among them. Modifications ------------- These methods, and cb_extract_expiry, which only they call, use cb_check_type, which throws ruby_exception with the message Check_Type builds. Methods registered without cb_method keep Check_Type. Results ------- A wrong argument type raises the same TypeError and message as before, and a test pins the message for a non-String document id.
There was a problem hiding this comment.
🔵 Needs a closer look
The cross-cutting Ruby/C++ exception-boundary changes need human review and native-runtime validation of unwinding, leak prevention, and error compatibility.
No open issues 🎉
What changed in this PR
Addresses RCBC-586 by routing backend argument errors through C++ exceptions so destructors run before Ruby raises.
Changes:
- Adds protected formatting and numeric conversions, and replaces direct raising calls across backend operations.
- Fixes unknown view namespace formatting to use the Symbol rather than its ID.
- Adds regression tests for leaks, argument errors, and dynamic Symbols.
| File | Description |
|---|---|
| test/operation_unwind_test.rb | Adds leak and argument-error regression tests. |
| ext/rcb_views.cxx | Fixes namespace formatting and type checks. |
| ext/rcb_utils.hxx | Adds protected formatting and numeric helpers. |
| ext/rcb_utils.cxx | Applies protected helpers to argument extraction. |
| ext/rcb_users.cxx | Updates user and group validation. |
| ext/rcb_search.cxx | Updates search validation and numeric conversions. |
| ext/rcb_range_scan.cxx | Updates scan validation and token conversions. |
| ext/rcb_query.cxx | Updates query validation and numeric conversions. |
| ext/rcb_multi.cxx | Updates batch-operation argument errors. |
| ext/rcb_logger.cxx | Updates logger-level type validation. |
| ext/rcb_extras.cxx | Updates manifest and DNS argument errors. |
| ext/rcb_diagnostics.cxx | Updates diagnostics and ping validation. |
| ext/rcb_crud.cxx | Updates document-operation argument handling. |
| ext/rcb_collections.cxx | Updates collection validation and expiry conversions. |
| ext/rcb_buckets.cxx | Updates bucket settings validation and conversions. |
| ext/rcb_backend.cxx | Updates connection and lifecycle argument handling. |
| ext/rcb_analytics.cxx | Updates analytics argument validation. |
Review effort: Balanced
|
🤖 This review was produced with Claude Code and posted on my behalf. The conversions look complete to me, and I didn't find anything blocking. There's one more spot in the same class of bug that might be worth handling here or in a follow-up. Unchecked
Ruby lets the value through. A few lines further down, the same functions already use if (TYPE(val) != T_SYMBOL) {
throw ruby_exception(exc_invalid_argument(),
cb_sprintf("replicate_to must be a Symbol, but given %+" PRIsVALUE, val));
}Another option is a This predates the PR, so I'm fine with it being a follow-up ticket instead. |
Motivation ---------- rb_sym2id raises TypeError by longjmp for a value that is not a Symbol. The legacy durability options replicate_to and persist_to, durability_level in the same extractors, a range scan's type and an analytics link's type reach it unchecked, after the backend method has copied its arguments. An upsert with replicate_to: "one" leaks the document body on each call. Modifications ------------- cb_sym2id() checks the type with cb_check_type, which throws the same TypeError, before calling rb_sym2id. The unchecked call sites use it. Results ------- A non-Symbol in these options raises the same TypeError and message, after the frames unwind. A test bounds RSS over repeated upserts with a non-Symbol replicate_to and persist_to.
Fixed in 3db1663, in this PR.
The other
|
Fixes RCBC-586.
Problem
Some backend errors are raised by
longjmpfrom inside C++ frames, which skips their destructors. Amutate_inwhose last spec is rejected leaks every spec value copied before it. An unknown view namespace given as a dynamic Symbol crashes the process.Change
Each commit moves one kind of raising call onto the throwing path, so the frames unwind before
cb_methodraises in Ruby.rb_sprintf, whosePRIsVALUEarguments runinspectorto_scb_sprintf,rb_vsprintfundercb_protectNUM2ULL,NUM2LL,NUM2UINT,FIX2INT,FIX2UINTcb_num2ull,cb_num2ll,cb_num2uint,cb_num2intrb_raisethrow ruby_exceptionCheck_Typecb_check_typerb_sym2idon a value not checked to be a Symbolcb_sym2id, which checks withcb_check_typefirstCommit 3 also formats the unknown view namespace as the Symbol, where it passed the Symbol's
ID.Error classes and messages are otherwise unchanged.
Known gaps
rb_define_methodkeep the raising calls, because a C++ exception cannot leave them. These are theHdrHistogramCmethods,Backend.set_log_level,Backend.enable_protocol_logger_to_save_network_traffic_to_file, andBackend.leb128_*,snappy_*,form_encode,query_escape,path_escapeandparse_connection_string.cb_backend_to_public_api_clusterstill callsrb_raise. #277 replaces that call.Check_Typeandrb_raise.