Backport #7982: rb_raise segfaults on %lli format with (0xffffffff + 1)

• Status: Closed
• Assignee: nagachika (Tomoyuki Chikanaga)

[erik.s.chang (Erik Chang)]

This was not a problem on 1.9.3

Run attached "t" extension with:
ruby -rt -e '"".blowup(0xffffffff + 1)'

to reproduce. Using %lld works around the issue.

[jeremyevans0 (Jeremy Evans)]

I don't think this is just the %lli format. It is probably at least the %li format as well. home_run's test suite fails on 2.0.0-p0 (working fine on 1.8.7 and 1.9.3), with the following backtrace:

#0 0x0000031c9da03eea in kill () at :2
#1 0x0000031c9da6a6ca in abort () at /usr/src/lib/libc/stdlib/abort.c:70
#2 0x0000031c931a82ad in rb_bug () from /usr/local/lib/libruby20.so.0.0
#3 0x0000031c932514bd in sigsegv () from /usr/local/lib/libruby20.so.0.0
#4
#5 0x0000031c9325efd0 in rb_obj_as_string () from /usr/local/lib/libruby20.so.0.0
#6 0x0000031c93254be8 in ruby__sfvextra () from /usr/local/lib/libruby20.so.0.0
#7 0x0000031c9325310f in BSD_vfprintf () from /usr/local/lib/libruby20.so.0.0
#8 0x0000031c93254eef in rb_enc_vsprintf () from /usr/local/lib/libruby20.so.0.0
#9 0x0000031c931a6f0f in rb_raise () from /usr/local/lib/libruby20.so.0.0
#10 0x0000031c9e6d582b in rhrd_s_civil (argc=3, argv=0x31c8ffd0358, klass=3421292953680) at ../../../../ext/date_ext/date_ext.c:1565
1565 rb_raise(rb_eArgError, "invalid date (year: %li, month: %li, day: %li)", year, month, day);

Changing %li to %ld makes it no longer fail at that location (it fails at the next location using %li instead). This is on:

ruby 2.0.0p0 (2013-02-24 revision 39474) [x86_64-openbsd]

[phasis68 (Heesob Park)]

Here is a patch:
diff --git a/vsnprintf.c b/vsnprintf.c.new
index 1e4cdc5..29bdfda 100644
--- a/vsnprintf.c
+++ b/vsnprintf.c.new
@@ -809,10 +809,10 @@ reswitch: switch (ch) {
if (fp->vextra && (flags & INTPTR_MASK) == INTPTR_FLAG) {
FLUSH();
#if defined HAVE_SANE_QUAD && SIZEOF_VOIDP == SIZEOF_LONG_LONG

•   		uqval = va_arg(ap, u_quad_t);
  

•   		uqval = LL2NUM(va_arg(ap, u_quad_t));
    		cp = (*fp->vextra)(fp, sizeof(uqval), &uqval, &fieldsz, sign);
  

#else

•   		ulval = va_arg(ap, u_long);
  

•   		ulval = LONG2NUM(va_arg(ap, u_long));
    		cp = (*fp->vextra)(fp, sizeof(ulval), &ulval, &fieldsz, sign);
  

#endif
sign = '\0';

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Rejected

=begin
Sorry, you can't use (({%i})) for (({rb_raise()})), (({rb_warn()})), and so on.
It's now used for (({Object}))s.

Use (({%d})) instead.
=end

On 02/28 02:59, nobu (Nobuyoshi Nakada) wrote:

Issue #7982 has been updated by nobu (Nobuyoshi Nakada).

Status changed from Open to Rejected

=begin
Sorry, you can't use (({%i})) for (({rb_raise()})), (({rb_warn()})), and so on.
It's now used for (({Object}))s.

Use (({%d})) instead.
=end

The Changelog entry for this is:

• vsnprintf.c (BSD_vfprintf): [EXPERIMENTAL] object representation in
  rb_enc_vsprintf(). [Feature #5896]

There is no indication that %i is now handled differently than before.
For a C programmer, overriding %i which has historically been used for
integers is very confusing and error prone. It is true that ruby is
not C, but this is a C API, and rb_raise's documentation in README.EXT
states:

void rb_raise(rb_eRuntimeError, const char *fmt, ...) ::

Raises RuntimeError. The fmt is a format string just like printf().

I content that the format string syntax in ruby 2.0.0 is not "just
like printf()". Maybe "similar to printf(), with the following
exceptions: ..."?

As this is still marked EXPERIMENTAL, it would be better if another
letter that isn't already supported by printf(3) was used instead.
At the very least, if you plan to continue the use of %i for object
representation, there should be specific notes in the documentation.

Thanks,
Jeremy

[nobu (Nobuyoshi Nakada)]

• Category set to doc
• Status changed from Rejected to Open

=begin
: jeremyevans (Jeremy Evans) wrote:
I content that the format string syntax in ruby 2.0.0 is not "just
like printf()". Maybe "similar to printf(), with the following
exceptions: ..."?

Thank you, I'll add it later.
=end

[zzak (zzak _)]

• Status changed from Open to Closed
• % Done changed from 0 to 100

This issue was solved with changeset r40504.
Erik, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

---

• README.EXT: Add note to warn use of %i in Exceptions [Bug #7982]

[zzak (zzak _)]

• Status changed from Closed to Assigned
• Assignee set to nobu (Nobuyoshi Nakada)
• % Done changed from 100 to 50

@nobu (Nobuyoshi Nakada), how does this look? Any other places we should add this? Are there any more formats that should be avoided?

[zzak (zzak _)]

• Backport set to 1.9.3: UNKNOWN, 2.0.0: UNKNOWN

[zzak (zzak _)]

• Backport changed from 1.9.3: UNKNOWN, 2.0.0: UNKNOWN to 2.0.0: UNKNOWN

[zzak (zzak _)]

• Status changed from Assigned to Closed
• % Done changed from 50 to 100

This issue was solved with changeset r40507.
Erik, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

---

• README.EXT: Copy note from r40505 for rb_sprintf() [Bug #7982]

[zzak (zzak _)]

• Status changed from Closed to Assigned
• Assignee changed from nobu (Nobuyoshi Nakada) to nagachika (Tomoyuki Chikanaga)
• % Done changed from 100 to 50

How is this? We should backport this too

[nobu (Nobuyoshi Nakada)]

• % Done changed from 50 to 100
• Status changed from Assigned to Closed

This issue was solved with changeset r40511.
Erik, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

---

README.EXT: update note

• README.EXT: correct method name to be used. [Bug #7982]
• README.EXT.ja: add notes too.

[nagachika (Tomoyuki Chikanaga)]

• Tracker changed from Bug to Backport
• Project changed from Ruby master to Backport200
• Category deleted (doc)
• Status changed from Closed to Assigned
• Target version deleted (2.1.0)

[nagachika (Tomoyuki Chikanaga)]

• Status changed from Assigned to Closed

This issue was solved with changeset r40544.
Erik, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

---

merge revision(s) 40504,40505,40507,40511: [Backport #7982]

* README.EXT: Add note to warn use of %i in Exceptions [Bug #7982]

* README.EXT: Update note from r40504, by Jeremy Evans [Bug #7982]

* README.EXT: Copy note from r40505 for rb_sprintf() [Bug #7982]

* README.EXT: correct method name to be used.  [Bug #7982]

* README.EXT.ja: add notes too.