Skip to content

Packer#write_extension: interior pointer of a coerced temporary held across rb_raise (use-after-move) #401

Description

@jeremy

Summary

Packer_write_extension takes a char * into a String that rb_String has just created, and
passes it to rb_raise, which allocates while it formats. If a GC compaction runs during that
allocation the temporary can move, and %s then reads a vacated slot. This is a latent
memory-safety defect on the error path. It is present at HEAD (and in 1.8.4).

Location

ext/msgpack/packer_class.c (line numbers from 1.8.4, identical in 1.8.3):

static VALUE Packer_write_extension(VALUE self, VALUE obj)
{
    ...
    VALUE rb_ext_type = RSTRUCT_GET(obj, 0);
    if(!RB_TYPE_P(rb_ext_type, T_FIXNUM)) {
        rb_raise(rb_eRangeError, "integer %s too big to convert to `signed char'",
                 RSTRING_PTR(rb_String(rb_ext_type)));   /* :238 */
    }

When the type slot is not a Fixnum, rb_String(rb_ext_type) coerces it to a fresh String that
no argument slot holds (argv holds the ExtensionValue struct, not the coerced String).
RSTRING_PTR takes an interior pointer into that fresh String, and rb_raiserb_vsprintf
allocates the message buffer before it reads %s. A compaction in that window relocates the fresh
String; the read lands in the vacated, zero-filled slot.

Reproduction

Reproduces on arm64-darwin, ruby 3.4.7/3.4.10/4.0.6: an ExtensionValue whose type slot is a
non-Fixnum object with an embedded (movable) to_str, packed under GC.stress + GC.auto_compact,
raises RangeError whose %s field is empty"integer too big to convert to \signed
char'"— 200/200, versus the intact marker on the same run with the type slot already a String (0/200) or with GC off (0/200). Natural rate, noGC.stress`: ~35 corruptions in 300,000 packs.

It is codegen-dependent: on aarch64-linux with Debian gcc 12 the temporary stays in a
conservatively-scanned register across rb_raise and does not move (0/300 while a same-size witness
relocated 300/300), so this build is safe only by accident. A compiler, flag or arch change unpins
it.

Fix

Root the coerced value in a local before raising, so it is a named VALUE on the stack for the
duration of the rb_raise:

VALUE s = rb_String(rb_ext_type);
rb_raise(rb_eRangeError, "integer %"PRIsVALUE" too big to convert to `signed char'", s);

Using %"PRIsVALUE" with the VALUE (rather than %s with RSTRING_PTR) also removes the raw
pointer entirely.

Environment: msgpack 1.8.3 and 1.8.4, ruby 3.4.x, arm64-darwin (fires) and aarch64-linux (latent).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions