Overflow checks and unsafe code

[oln]

I think Integer overflows/underflows may be another thing that it would be a good idea watch out for in unsafe code, particularly when say indexing or working with pointers. Now they may not be quite as risky as in say C, as they will panic in debug mode, and in safe code there are generally extra guards that prevents them from being exploited excluding logic bugs. However, they could still lead to issues in unsafe code that isn’t checked and fuzzed well enough hitting an edge case in production use.

[nikomatsakis]

Indeed! However, while they can be the source of bugs, I don’t think that integer overflows/underflows (in Rust, anyway) are directly the source of undefined behavior, since Rust declares that they will have a 2’s complement result (in optimized builds; they panic in debug builds). That said, it’s really easy to overlook overflows, and I’ve sometimes though about enabling the checks by default e.g. in Rayon – or at least measuring the impact of doing so.

[fweimer]

• Niko Matsakis:

Indeed! However, while they can be the source of bugs, I don’t think that integer overflows/underflows (in Rust, anyway) are directly the source of undefined behavior, since Rust declares that they will have a 2’s complement result (in optimized builds; they panic in debug builds). That said, it’s really easy to overlook overflows, and I’ve sometimes though about enabling the checks by default e.g. in Rayon – or at least measuring the impact of doing so.

Overflows could invalidate bounds checks in unsafe code, though. When writing unsafe code, it would be nice to be able to write something like this:

  if header_size + nelems * element_size > usize::max_value() {
    ... // Report memory allocation failure
  }

That is, the compiler would evaluate the condition with arbitrary precision and return the exact result.

It is possible to emulate this in various ways (using checked or saturating arithmetic), of course, but the result is rather verbose, and therefore difficult to review.

As you said, if you ensure memory safety elsewhere, then the precise definition of integer arithmetic does not matter for memory safety. So maybe the answer is that up-front size checks such as the one above are doomed anyway, and you should (even in unsafe code) perform size checks when allocating from the buffer (against the allocated size, potentially computed incorrectly).

[arielb1]

This is the “unsafe code guidelines” thread. We are trying to figure out a reasonable definition for “what is UB”. Broken bounds checking, or broken allocation sizes, do not cause direct UB in Rust.

Of course, if the broken bounds checking causes OOB accesses, these might be UB for rather obvious reasons, but there’s nothing to be discussed from the “what is defined” perspective.

[nikomatsakis]

Next steps for the unsafe code guidelines

Overflows could invalidate bounds checks in unsafe code, though.

Yes, definitely! What I meant by my post was not that overflow is not a concern, but that I don’t think there’s anything that the unsafe code guidelines in particular need to say about it. Indexing an array out of its bounds (or adjusting a pointer improperly, and so forth) are definitely things we would want to talk about, and it seems like in any final document or guide to writing unsafe code, there is definitely room for a chapter on overflow – but it seems to me that having code which (incorrectly) assumes the absence of overflow is “just a bug”, from the POV of the language spec.

Put another way, having a bounds check which fails due to an overflow you were not expecting is not different from code that simply forgot to do the bounds check altogether from the POV of the rest of the system.

[fweimer]

Next steps for the unsafe code guidelines

Put another way, having a bounds check which fails due to an overflow you were not expecting is not different from code that simply forgot to do the bounds check altogether from the POV of the rest of the system.

Sorry for being unclear. I meant hand-written overflow checks in unsafe code. For some tasks, this is hardly unusual, and I don’t think Rust currently has a good answer to these issues.

[RalfJung]

Next steps for the unsafe code guidelines

Sorry for being unclear. I meant hand-written overflow checks in unsafe code. For some tasks, this is hardly unusual, and I don’t think Rust currently has a good answer to these issues.

I think it does – don’t hand-write these checks! The standard library provides methods to perform checked arithmetic.

[fweimer]

Next steps for the unsafe code guidelines

I think it does – don’t hand-write these checks! The standard library provides methods to perform checked arithmetic.

Sorry again, not what I meant. Let’s assume you need to compute the size of a memory allocation, like this C code I recently wrote:

  void *ptr;
  struct alloc_buffer buffer = alloc_buffer_allocate
    (sizeof (struct resolv_conf)
     + init->nameserver_list_size * sizeof (init->nameserver_list[0])
     + address_space
     + init->search_list_size * sizeof (init->search_list[0])
     + string_space,
     &ptr);

Let’s assume I would like to write this with proper overflow checks. (In the example above, they are not needed because the incoming data acts as a witness that overflow is not possible, and alloc_buffer performs its own checks internally anyway.) With the checked_* functions, the code would look like this:

  let size = match init.nameserver_list_size.checked_mul(
            size_of::<*const sockaddr>())
        .and_then(|x| x.checked_add(address_space))
        .and_then(|x| init.search_list_size.checked_mul(
                size_of::<*const c_char>())
        	.and_then(|y| x.checked_add(y)))
        .and_then(|x| x.checked_add(string_space)) {
    None => { return }
    Some(s) => s
  };
  let (mut buffer, ptr) = alloc_buffer::allocate(size);

Unfortunately, this obscures what is being computed.

[RalfJung]

Once ? works with Option, you could write something more like

let server_list_size = init.nameserver_list_size.checked_mul(size_of::<*const sockaddr>())?;
let search_list_size = init.search_list_size.checked_mul(size_of::<*const sockaddr>())?;
let size = server_list_size
  .checked_add(address_space)?
  .checked_add(search_list_size)?
  .checked_add(string_space)?;
let (mut buffer, ptr) = alloc_buffer::allocate(size);

Not too bad, IMHO – it’s not like this kind of code comes up all the time, does it?

To make things more convenient, you could define a wrapper type for (i32, bool) that overloads the arithmetic operators, that internally keeps track of whether an overflow happened (always using disjunction and the overflowing operations). I drafted something here: https://play.rust-lang.org/?gist=184268945e2ada9cc2a39a7731afb666&version=stable&backtrace=0.

The could would then look like

let size = Overflowing::new(init.nameserver_list_size) * Overflowing::new(size_of::<*const sockaddr>());
let search_list_size = Overflowing::new(init.search_list_size) * Overflowing::new(size_of::<*const sockaddr>()));
let size = server_list_size + Overflowing::new(address_space) + search_list_space + Overflowing::new(string_space);
let (mut buffer, ptr) = alloc_buffer::allocate(size.extract()?);

[sanxiyn]

Not too bad, IMHO

I actually consider this too bad, but I think it can be solved externally by a procedural macro. Basically, you implement checked! macro (name taken from C#) and checked! { a + b * c } expands to { let _1 = b.checked_mul(c)?; let _2 = a.checked_add(_1)?; _2 } or something like that. IMHO, forcing people to manually do this macro expansion is bad.

[system]

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.