Backport #7982

rb_raise segfaults on %lli format with (0xffffffff + 1)

Added by Erik Chang over 2 years ago. Updated about 2 years ago.

[ruby-core:<unknown>]
Status:Closed
Priority:Normal
Assignee:Tomoyuki Chikanaga

Description

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.

t.c Magnifier - test extension to segfault (257 Bytes) Erik Chang, 02/28/2013 09:50 AM

extconf.rb Magnifier - extconf for test extension (36 Bytes) Erik Chang, 02/28/2013 09:50 AM

Associated revisions

Revision 40504
Added by Zachary Scott over 2 years ago

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

Revision 40505
Added by Zachary Scott over 2 years ago

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

Revision 40507
Added by Zachary Scott over 2 years ago

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

Revision 40511
Added by Nobuyoshi Nakada over 2 years ago

README.EXT: update note

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

Revision 40544
Added by Tomoyuki Chikanaga over 2 years ago

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.

History

#1 Updated by Jeremy Evans over 2 years ago

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]

#2 Updated by Heesob Park over 2 years ago

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';

#3 Updated by Nobuyoshi Nakada over 2 years ago

  • 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

#4 Updated by Jeremy Evans over 2 years ago

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

#5 Updated by Nobuyoshi Nakada over 2 years ago

  • 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

#6 Updated by Zachary Scott over 2 years ago

  • 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]

#7 Updated by Zachary Scott over 2 years ago

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

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

#8 Updated by Zachary Scott over 2 years ago

  • Backport set to 1.9.3: UNKNOWN, 2.0.0: UNKNOWN

#9 Updated by Zachary Scott over 2 years ago

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

#10 Updated by Zachary Scott over 2 years ago

  • 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]

#11 Updated by Zachary Scott over 2 years ago

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

How is this? We should backport this too

#12 Updated by Nobuyoshi Nakada over 2 years ago

  • % 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.

#13 Updated by Tomoyuki Chikanaga over 2 years ago

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

#14 Updated by Tomoyuki Chikanaga about 2 years ago

  • 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.

Also available in: Atom PDF