Skip to content

Fix edgecase segfault of BigDecimal#remainder#349

Merged
tompng merged 1 commit intoruby:masterfrom
tompng:fix_remainder_edgecase_segfault
Jun 10, 2025
Merged

Fix edgecase segfault of BigDecimal#remainder#349
tompng merged 1 commit intoruby:masterfrom
tompng:fix_remainder_edgecase_segfault

Conversation

@tompng
Copy link
Copy Markdown
Member

@tompng tompng commented Jun 10, 2025

There is a possibility where coerced remainder returns Qnil.
NIL_P(BigDecimal_divremain(self, r, &d, &rv)) doesn't mean d and rv has a calculated value.

This pull request is kind of a refactoring, but also fixes this segmentation fault.

obj = Object.new
def obj.coerce(*)=[self, self]
def obj.remainder(*) = nil
BigDecimal(1).remainder(obj) #=>  [BUG] Segmentation fault at 0x0000000000000020

There is a possibility where coerced remainder returns Qnil.
NIL_P(BigDecimal_divremain(self, r, &d, &rv)) doesn't mean d and rv has a calculated value.
@tompng tompng force-pushed the fix_remainder_edgecase_segfault branch from 2ae530f to ba4e15d Compare June 10, 2025 14:06
Comment on lines +2091 to +2094
if (BigDecimal_divremain(self, r, &d, &rv)) {
return VpCheckGetValue(rv);
}
return DoSomeOne(self, r, rb_intern("remainder"));
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same interface as BigDecimal_DoDivmod

        if (BigDecimal_DoDivmod(self, b, &div, &mod)) {
            return BigDecimal_to_i(VpCheckGetValue(div));
        }
        return DoSomeOne(self, b, rb_intern("div"));

}

if (!b) return DoSomeOne(self, r, rb_intern("remainder"));
if (!b) return Qfalse;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think using Qfalse and Qtrue instead of false and true, but this pattern is frequently used in bigdecimal.c even if the function name starts with BigDecimal_.
But this pattern is used in is_zero, is_one and BigDecimal_DoDivmod.
I think it's better to refactor in a separate pull request

@tompng tompng merged commit 2f9a424 into ruby:master Jun 10, 2025
168 of 170 checks passed
@tompng tompng deleted the fix_remainder_edgecase_segfault branch June 10, 2025 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant