Bug #22323
closedIncorrect nonnull contract changes rb_typeddata_inherited_p(NULL, NULL) behavior
Description
rb_typeddata_inherited_p() documents that both arguments may be NULL,
and its implementation returns true for (NULL, NULL). However, both the
exported declaration and the inline implementation are marked with
RBIMPL_ATTR_NONNULL(()), which tells GCC and Clang that all pointer
arguments are nonnull.
This contradictory contract can change observable behavior under
optimization.
Relevant code¶
The declaration is annotated as follows:
RBIMPL_ATTR_NONNULL(())
/**
* Checks for the domestic relationship between the two.
*
* ...
*
* You can pass NULL to both arguments, don't know what that means though.
*/
int rb_typeddata_inherited_p(const rb_data_type_t *child,
const rb_data_type_t *parent);
The inline implementation has the same annotation:
RBIMPL_ATTR_NONNULL(())
static inline bool
rbimpl_typeddata_inherited_p_inline(const rb_data_type_t *child,
const rb_data_type_t *parent)
{
do {
if (RB_LIKELY(child == parent)) return true;
} while ((child = child->parent) != NULL);
return false;
}
#define rb_typeddata_inherited_p rbimpl_typeddata_inherited_p_inline
Because the equality check occurs before child is dereferenced,
rb_typeddata_inherited_p(NULL, NULL) returns true according to the
implementation.
The relevant header is:
https://github.com/ruby/ruby/blob/e60ce574f0/include/ruby/internal/core/rtypeddata.h
Reproducer¶
I reproduced the issue with the following minimal C extension:
#include <ruby.h>
#include <stdint.h>
static VALUE
typeddata_null(VALUE self, VALUE use_nonnull)
{
const rb_data_type_t *type = RTEST(use_nonnull)
? (const rb_data_type_t *)(uintptr_t)1
: NULL;
int inherited = rb_typeddata_inherited_p(type, type);
/*
* Returning 2 means the compiler selected the type != NULL branch.
* When use_nonnull is false, the expected result is inherited == 1.
*/
return INT2NUM(type != NULL ? 2 : inherited);
}
void
Init_typeddata_null(void)
{
VALUE module = rb_define_module("TypeddataNull");
rb_define_singleton_method(module, "check", typeddata_null, 1);
}
With this extconf.rb:
I built the extension with Clang 18 at -O0 and -O2, then ran:
The results were:
The argument to check() is false, so type is NULL at runtime.
The expected result is therefore 1: the two pointers are equal and
rb_typeddata_inherited_p(NULL, NULL) returns true.
The value 2 is not a return value from rb_typeddata_inherited_p().
It is a marker showing that Clang selected the later type != NULL
branch. The nonnull annotation allows Clang to assume that type
cannot be null after it has been passed to the function, even though
the documented and implemented (NULL, NULL) case was used.
Clang 14 generated the same optimized control flow. GCC and Clang also
emit -Wnonnull diagnostics when the two null arguments are written
directly.
Possible fix¶
If (NULL, NULL) is intended to remain supported, I believe the
incompatible RBIMPL_ATTR_NONNULL(()) annotations should be removed
from both the exported declaration and
rbimpl_typeddata_inherited_p_inline().
Removing only the annotation from the exported declaration is not
sufficient because extension calls are redirected to the annotated
inline implementation by the macro.
After removing both annotations, the Clang 18 -O2 extension reports:
The complete patched Ruby tree also passes make test.
Could you confirm whether (NULL, NULL) is an intended input for this
function? If it is not intended, the documentation and the explicit
equality behavior may instead need to be updated.