Fix two instances of undefined behavior in codec crate #7751

brson · 2020-05-07T02:40:11Z

What problem does this PR solve?

Fix two instances of undefined behavior in codec crate, one in MemComparableBytesCodec::try_decode_first_internal and one in its tests.

Case 1: This function constructs a pointer called src_ptr_next to the next place it is going to need to look for data to decode. In the error case, this pointer points beyond its allocation, which is not allowed, and is undefined behavior. This patch changes that code path to use the offset_from function to instead calculate whether the difference from the current, valid pointer, to the next location, will be in-bounds.

Case 2: A test case is constructing two overlapping slices, one of them mutable, in order to test the behavior of the decoder for overlapping slices. This is undefined behavior. I've changed the test to use the in_place versions of the functions, which don't require overlapping slices.

Release note

No release note

It is UB to construct out-of-bounds pointers. Signed-off-by: Brian Anderson <[email protected]>

Previous code creates aliasing mutable slices. Signed-off-by: Brian Anderson <[email protected]>

BusyJay · 2020-05-07T02:55:39Z

components/codec/src/byte.rs

 return Err(ErrorInner::eof().into());
 }

+ let src_ptr_next = src_ptr.add(MEMCMP_GROUP_SIZE + 1);


I don't get it. How can it be an undefined behavior? I think accessing the data it points to is undefined, but get the pointer address is pure a math calculation.

I don't understand the rules enough to say for sure, but here's what miri says about this code: https://gist.github.com/brson/26d2111ce4c1a555d0e9a817c37ff646

I take this to mean that even creating the pointer is UB.

I can't find language in https://doc.rust-lang.org/beta/reference/behavior-considered-undefined.html to support the idea that even doing pointer math to create out-of-bounds pointers is undefined. The strongest it says is that dereferencing dangling pointers is undefined, which is obvious to me.

cc @oli-obk @RalfJung sorry for pinging you so much, but maybe you can explain why it's not ok to do pointer math that produces a bad pointer here.

Yeah doing pointer arithmetic on out of bounds pointers is UB in LLVM, see for example this blog post: https://blog.regehr.org/archives/1518 - "it is UB to create a pointer that lies more than one element outside of an allocated object. It is UB merely to create such a pointer, it does not need to be dereferenced. Also, it is still UB even if the overflowed pointer happens to refer to some other allocated object.". This is about C, but applies equally to Rust

Get it. So it's an undefined behavior because the calculation can produce an overflow value.

I can't find language in https://doc.rust-lang.org/beta/reference/behavior-considered-undefined.html to support the idea that even doing pointer math to create out-of-bounds pointers is undefined.

That list only enumerates UB caused by language primitives. When you call a method, the method docs mention the UB conditions for that method. In the case of add, it says:

Both the starting and resulting pointer must be either in bounds or one byte past the end of the same allocated object. Note that in Rust, every (stack-allocated) variable is considered a separate allocated object.

So no, it's not just overflow that is a problem.

If you are asking "why does add say this is UB", the short answer is "because this is what LLVM requires for getelementptr inbounds, and the long answer is that this helps the compiler analyze which pointers could potentially point where.

Maybe we should report targetted (to the intrinsic) errors instead of generic errors when they are caused by the intrinsic's shim?

The intrinsic is an unstable internal implementation detail though -- how it is called and which intrinsic is used for what can change any time. Seems a bit strange to surface that in error messages?

Oh right :/ the calling function's docs are important, not the bare intrinsic. Yea not sure what we can do to improve the miri error then.

Thanks for the help @RalfJung @oli-obk

Signed-off-by: Brian Anderson <[email protected]>

brson · 2020-05-07T02:57:46Z

This also marks the try_decode_first_internal function as unsafe.

nrc

lgtm, two comments about comments inline

components/codec/src/byte.rs

Signed-off-by: Brian Anderson <[email protected]>

sre-bot · 2020-05-10T20:38:57Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

brson · 2020-05-10T20:41:05Z

/rebuild

brson · 2020-05-10T21:43:38Z

/merge

sre-bot · 2020-05-10T21:43:41Z

Your auto merge job has been accepted, waiting for:

7737

sre-bot · 2020-05-10T21:46:36Z

/run-all-tests

sre-bot · 2020-05-10T22:02:35Z

@brson merge failed.

brson · 2020-05-10T22:24:51Z

/merge

sre-bot · 2020-05-10T22:25:04Z

/run-all-tests

sre-bot · 2020-05-10T22:31:34Z

@brson merge failed.

brson · 2020-05-11T23:11:43Z

/merge

sre-bot · 2020-05-11T23:14:08Z

/run-all-tests

BusyJay · 2020-05-12T07:27:10Z

components/codec/src/byte.rs

@@ -1048,12 +1071,11 @@ mod tests {
 let output_len = unsafe {
 let src_ptr = buffer.as_mut_ptr().add(encoded_prefix_len);
 let slice_len = buffer.len() - encoded_prefix_len;
- let src = std::slice::from_raw_parts(src_ptr, slice_len);
- let dest = std::slice::from_raw_parts_mut(src_ptr, slice_len);
+ let buf = std::slice::from_raw_parts_mut(src_ptr, slice_len);


I think this is on purpose as L1069 says. There should either be some misunderstanding or the case should be removed.

/cc @breeswish

Indeed, how about calling try_decode_first_internal instead?

brson · 2020-05-18T00:06:06Z

Waiting on further review from @breeswish

CLAassistant · 2020-12-16T03:36:00Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ sre-bot
❌ brson
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

ti-chi-bot · 2023-04-23T02:02:50Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

brson added 2 commits May 7, 2020 02:24

codec: Fix undefined behavior in try_decode_first_internal

06c4f7d

It is UB to construct out-of-bounds pointers. Signed-off-by: Brian Anderson <[email protected]>

codec: Fix undefined behavior in try_decode_first tests

69b602a

Previous code creates aliasing mutable slices. Signed-off-by: Brian Anderson <[email protected]>

brson added the needs-cherry-pick-release-4.0 Type: Need cherry pick to release 4.0 label May 7, 2020

brson requested a review from breezewish May 7, 2020 02:40

BusyJay reviewed May 7, 2020

View reviewed changes

codec: Mark MemComparableByteCodec::try_decode_first_internal unsafe

1f4afe4

Signed-off-by: Brian Anderson <[email protected]>

nrc approved these changes May 7, 2020

View reviewed changes

components/codec/src/byte.rs Show resolved Hide resolved

components/codec/src/byte.rs Show resolved Hide resolved

zhangjinpeng87 added this to In progress in Question and Bug Reports May 7, 2020

zhangjinpeng87 moved this from In progress to Review in progress in Question and Bug Reports May 7, 2020

zhangjinpeng87 removed this from In Progress: root cause located in Question and Bug Reports May 7, 2020

codec: Add safety doc comment sto try_decode_first_internal

868f1e3

Signed-off-by: Brian Anderson <[email protected]>

sre-bot added the status/can-merge Status: Can merge to base branch label May 10, 2020

Merge branch 'master' into codec-ub

6190b28

Merge branch 'master' into codec-ub

26cba0a

BusyJay reviewed May 12, 2020

View reviewed changes

brson removed the status/can-merge Status: Can merge to base branch label May 18, 2020

ti-chi-bot bot added the needs-rebase label Apr 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix two instances of undefined behavior in codec crate #7751

Fix two instances of undefined behavior in codec crate #7751

brson commented May 7, 2020 •

edited

BusyJay May 7, 2020

brson May 7, 2020

nrc May 7, 2020

BusyJay May 7, 2020

RalfJung May 7, 2020 •

edited

RalfJung May 7, 2020 •

edited

oli-obk May 7, 2020

RalfJung May 7, 2020

oli-obk May 7, 2020

brson May 10, 2020

brson commented May 7, 2020

nrc left a comment

sre-bot commented May 10, 2020

brson commented May 10, 2020

brson commented May 10, 2020

sre-bot commented May 10, 2020

sre-bot commented May 10, 2020

sre-bot commented May 10, 2020

brson commented May 10, 2020

sre-bot commented May 10, 2020

sre-bot commented May 10, 2020

brson commented May 11, 2020

sre-bot commented May 11, 2020

BusyJay May 12, 2020

breezewish Jun 2, 2020

brson commented May 18, 2020

CLAassistant commented Dec 16, 2020 •

edited

ti-chi-bot bot commented Apr 23, 2023

Fix two instances of undefined behavior in codec crate #7751

Are you sure you want to change the base?

Fix two instances of undefined behavior in codec crate #7751

Conversation

brson commented May 7, 2020 • edited

What problem does this PR solve?

Release note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung May 7, 2020 • edited

Choose a reason for hiding this comment

RalfJung May 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brson commented May 7, 2020

nrc left a comment

Choose a reason for hiding this comment

sre-bot commented May 10, 2020

brson commented May 10, 2020

brson commented May 10, 2020

sre-bot commented May 10, 2020

sre-bot commented May 10, 2020

sre-bot commented May 10, 2020

brson commented May 10, 2020

sre-bot commented May 10, 2020

sre-bot commented May 10, 2020

brson commented May 11, 2020

sre-bot commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brson commented May 18, 2020

CLAassistant commented Dec 16, 2020 • edited

ti-chi-bot bot commented Apr 23, 2023

brson commented May 7, 2020 •

edited

RalfJung May 7, 2020 •

edited

RalfJung May 7, 2020 •

edited

CLAassistant commented Dec 16, 2020 •

edited