From 48649e46253035a6ed1ce4ba18dd2b151f7cc4d8 Mon Sep 17 00:00:00 2001 From: shyouhei Date: Wed, 7 Nov 2018 07:16:50 +0000 Subject: [PATCH] insns.def: avoid integer overflow In these expressions `1` is of type `signed int` (cf: ISO/IEC 9899:1990 section 6.1.3.2). The variable (e.g. `num`) is of type `rb_num_t`, which is in fact `unsigned long`. These two expressions then exercises the "usual arithmetic conversions" (cf: ISO/IEC 9899:1990 section 6.2.1.5) and both eventually become `unsigned long`. The two unsigned expressions are then subtracted to generate another unsigned integer expression (cf: ISO/IEC 9899:1990 section 6.3.6). This is where integer overflows can occur. OTOH the left hand side of the assignments are `rb_snum_t` which is `signed long`. The assignments exercise the "implicit conversion" of "an unsigned integer is converted to its corresponding signed integer" case (cf: ISO/IEC 9899:1990 section 6.2.1.2), which is "implementation-defined" (read: not portable). Casts are the proper way to avoid this problem. Because all expressions are converted to some integer types before any binary operations are performed, the assignments now have fully defined behaviour. These values can never exceed LONG_MAX so the casts must not lose any information. See also: https://travis-ci.org/ruby/ruby/jobs/451726874#L4357 git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@65595 b2dd03c8-39d4-4d8f-98ff-823fe69b080e --- insns.def | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/insns.def b/insns.def index 747e6ef8d4..b92e10ae9a 100644 --- a/insns.def +++ b/insns.def @@ -380,7 +380,7 @@ concatstrings (rb_num_t num) (...) (VALUE val) -// attr rb_snum_t sp_inc = 1 - num; +// attr rb_snum_t sp_inc = 1 - (rb_snum_t)num; { val = rb_str_concat_literals(num, STACK_ADDR_FROM_TOP(num)); } @@ -416,7 +416,7 @@ toregexp /* This instruction has StringValue(), which is a method call. But it * seems that path is never covered. */ // attr bool leaf = true; /* yes it is */ -// attr rb_snum_t sp_inc = 1 - cnt; +// attr rb_snum_t sp_inc = 1 - (rb_snum_t)cnt; { const VALUE ary = rb_ary_tmp_new_from_values(0, cnt, STACK_ADDR_FROM_TOP(cnt)); val = rb_reg_new_ary(ary, (int)opt); @@ -439,7 +439,7 @@ newarray (rb_num_t num) (...) (VALUE val) -// attr rb_snum_t sp_inc = 1 - num; +// attr rb_snum_t sp_inc = 1 - (rb_snum_t)num; { val = rb_ary_new4(num, STACK_ADDR_FROM_TOP(num)); } @@ -503,7 +503,7 @@ newhash (...) (VALUE val) // attr bool leaf = false; /* has rb_hash_key_str() */ -// attr rb_snum_t sp_inc = 1 - num; +// attr rb_snum_t sp_inc = 1 - (rb_snum_t)num; { RUBY_DTRACE_CREATE_HOOK(HASH, num); @@ -804,7 +804,7 @@ opt_newarray_max * necessary. No way to detect such method calls beforehand. We * cannot but mark it being not leaf. */ // attr bool leaf = false; /* has rb_funcall() */ -// attr rb_snum_t sp_inc = 1 - num; +// attr rb_snum_t sp_inc = 1 - (rb_snum_t)num; { val = vm_opt_newarray_max(num, STACK_ADDR_FROM_TOP(num)); } @@ -816,7 +816,7 @@ opt_newarray_min (VALUE val) /* Same discussion as opt_newarray_max. */ // attr bool leaf = false; /* has rb_funcall() */ -// attr rb_snum_t sp_inc = 1 - num; +// attr rb_snum_t sp_inc = 1 - (rb_snum_t)num; { val = vm_opt_newarray_min(num, STACK_ADDR_FROM_TOP(num)); }