string.c: compare the append source as an address, not as a pointer - #7051
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesString concatenation overlap handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This needs a rebase: #7043 landed as 0aeae4b and moved the containment test above The change itself I want, and the reasoning is right: After the rebase both changes apply to the same four lines and compose without interacting, since one moves the test and the other changes what it compares: /* comment from #7043 */
if ((uintptr_t)ptr >= (uintptr_t)RSTR_PTR(s) &&
(uintptr_t)ptr <= (uintptr_t)RSTR_PTR(s) + (size_t)RSTR_LEN(s)) {
off = ptr - RSTR_PTR(s);
}
mrb_int capa = str_modify_cat(mrb, s, (mrb_int)len);One thing to decide while you are in there, rather than a request: the |
`mrb_str_cat()` finds out whether `ptr` points into `s` by comparing it against `RSTR_PTR(s)`, and subtracts the two where it does. `ptr` is a parameter of a public API and may come from any object, so the two need not point into the same one, and C leaves both the relational comparison (C11 6.5.8p5) and the pointer subtraction (6.5.6p9) undefined when they do not. A containment test has no way of saying what it means in pointers alone; `uintptr_t` is where it can be said, since the integers are ordered across the whole range. Convert both operands and test the addresses. The answer is the same wherever the address space is flat, which is everywhere mruby builds, so nothing changes at runtime. `uintptr_t` is no new requirement either: `mrb_ptr_to_str()` in this file already uses it, as does `is_pool_memory()` in `mrbgems/mruby-bigint/core/bigint.c`, which writes its own containment test the same way.
cd8c891 to
bd55e53
Compare
|
Rebased onto 9233195. The conflict was the four lines you named: #7043 moved the test above On the subtraction: it was already off = (ptrdiff_t)(ptr_addr - str_addr);since the first push, so 6.5.6p9 is covered along with 6.5.8p5, and the sketch in your comment is the one place the old form still appears. /* ... comment from #7043 ...
`ptr` is allowed to come from anywhere, so it and `RSTR_PTR(s)` need not
point into the same object, and relational comparison and subtraction
between pointers that do not is undefined. Going through `uintptr_t`
leaves both on integers, where the whole range is ordered. */
uintptr_t ptr_addr = (uintptr_t)ptr;
uintptr_t str_addr = (uintptr_t)RSTR_PTR(s);
if (ptr_addr >= str_addr && ptr_addr <= str_addr + (uintptr_t)RSTR_LEN(s)) {
off = (ptrdiff_t)(ptr_addr - str_addr);
}
mrb_int capa = str_modify_cat(mrb, s, (mrb_int)len);Retested after the rebase: |
2 participants
Summary
mrb_str_cat()decides whetherptrpoints into the string it is appending to, so thatthe source can be followed if the buffer moves under it:
ptris a parameter of a public API and may point into any object. When it points intosomething other than
s, which is the ordinary case for a plain append, the two pointersare not into the same object, and C leaves the relational comparison undefined (C11
6.5.8p5). The subtraction on the line below is undefined on the same grounds (6.5.6p9),
though it only runs once the comparison has said the two are related.
That is the shape every pointer containment test has. There is no way to ask the question
in pointers alone: the operators that would ask it are defined only for operands already
known to be related.
The change
Convert both operands to
uintptr_tonce and compare the addresses. Integers are orderedacross the whole range, so the test means what it is written to mean.
Nothing changes at runtime. Wherever the address space is flat, and that is everywhere
mruby builds, the addresses order the same way the pointers did, so the same appends are
recognized as overlapping and the same offset comes out.
uintptr_tis no new requirement either:mrb_ptr_to_str()in this file already uses it,as does
is_pool_memory()inmrbgems/mruby-bigint/core/bigint.c, which writes its owncontainment test this way.
Scope
The comparison has stood since
54132e436(2014). No sanitizer flags it and no compiler Iam aware of exploits it, so this is a conformance change rather than a bug fix, which is
why it comes on its own rather than folded into anything.
Relation to #7043
#7043 moves these same lines above
str_modify_cat()without changing their form. The twodo not overlap in intent, only in position. Whichever lands first, I will rebase the other
onto it.
Testing
rake testwithbuild_config/default.rb,MRUBY_CONFIG=asan rake test(
address,undefined), andMRUBY_CONFIG=ci/gcc-clang rake test, which coversMRB_GC_STRESSwithMRB_USE_DEBUG_HOOK,MRB_GC_FIXED_ARENA, and the C++ ABI build.KO: 0andCrash: 0on all of them.No test comes with this. Undefined behavior that every current toolchain compiles into the
intended code is not something the suite can distinguish.
Summary by CodeRabbit