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.
Updated by nobu (Nobuyoshi Nakada) 4 days ago
Thank you for the report.
yqtian (Yongqiang Tian) wrote:
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.
I don't think it is intended.
What do you mean by "the explicit equality behavior"?
Updated by yqtian (Yongqiang Tian) 4 days ago
Thank you. By “the explicit equality behavior,” I meant this check at the
beginning of the inline implementation:
The current header therefore gives three conflicting signals about the
(NULL, NULL) case:
- The documentation explicitly says that both arguments may be NULL.
- The implementation compares the arguments before dereferencing child, so
(NULL, NULL) returns true through the equality check. - RBIMPL_ATTR_NONNULL(()) tells the compiler that neither argument can ever
be NULL.
The reproducer demonstrates the consequence of this inconsistency. At -O0,
Clang follows the apparent documented and implemented behavior:
rb_typeddata_inherited_p(NULL, NULL) returns true. At -O2, however, the
nonnull contract allows Clang to assume that the argument cannot be NULL.
That assumption affects the caller after the call, so the later
type != NULL branch is optimized as true even though type is NULL at
runtime.
In other words, the value 2 in the reproducer is not returned by
rb_typeddata_inherited_p(). It shows that the nonnull contract has allowed
the compiler to derive a conclusion that conflicts with the documented
(NULL, NULL) use.
Updated by nobu (Nobuyoshi Nakada) 4 days ago
Neither argument is intended to be NULL. The nonnull annotation was added in 72627d85e337e5d8c7fa5738dc4ec7f253f0738e, which also annotated RTYPEDDATA_TYPE with RBIMPL_ATTR_RETURNS_NONNULL(). The expected child argument is the return value of RTYPEDDATA_TYPE, and the expected parent argument is the address of a constant type descriptor.
The equality comparison with parent has been present since the function was introduced and is part of its purpose. It does not imply that (NULL, NULL) is a supported input. The original while (child) guard was merely defensive, not intentional support for NULL arguments.
The documentation stating otherwise should be corrected.
Updated by nobu (Nobuyoshi Nakada) 4 days ago
- Status changed from Open to Closed
Applied in changeset git|b9d4fd7108b353fd58717c556757f0bce746c4cc.
[Bug #22323] [DOC] Remove a stale internal note for typed data
Neither argument to rb_typeddata_inherited_p is intended to be NULL.
Updated by yqtian (Yongqiang Tian) 4 days ago
Oh I see the point. Thank you for the clarification and commit!