Open Bug 1347384 Opened 7 years ago Updated 2 years ago

Allow CheckedInt<T*>, or add CheckedPtr<T*>

Categories

(Core :: MFBT, enhancement)

53 Branch
enhancement

Tracking

()

People

(Reporter: mozbugz, Unassigned)

Details

It would be great to have a way to perform checked operations on pointers, most importantly to detect overflows.

It is already possible to do so with CheckedInt<uintptr_t>, but that requires using reinterpret_cast's to convert to/from pointers. And theoretically uintptr_t could be wider than a pointer, which could lead to truncations when converting back to a pointer.

So I am suggesting a couple of solutions:
- Simply allow CheckedInt<T*>, assuming Everything Just Works.
- Create a separate CheckedPtr<T*>. It's obviously more work, duplicate code, and maintenance cost; but a bit less confusing than the first option, and it could also offer pointer-friendly functions, e.g. operator->().
It seems to me this would only encourage people to do pointer operations when they should, instead, be using arrays or some such. Also, address space overflow, which would be the only thing this checks is far from the most prominent problem with pointer arithmetics.
Agreed that arrays/ranges/spans/etc. should be preferred over pointers in most cases.

Now, I don't think a CheckedPtr would "encourage" pointer usage. People who want to use pointer, will use pointers anyway! People who already use arrays will not suddenly jump to pointers when seeing CheckedPtr (I would hope!?)

But at least it would give pointer-loving devs an easy&safe&portable way to deal with potential address space overflows.

And more importantly, it would help us safely plug security holes in existing code, without having to rewrite everything around them.
It may not be a prominent problem, but it absolutely needs to be handled when working with externally-provided data that could contain arbitrarily-big file offsets (for an example of something I'm working on right now).
To bikeshed a little, I feel like "CheckedPtr" overpromises. (It sort of sounds like it might do bounds checking.)
Makes sense, Andrew. Suggestions?

Most verbose would be "OverflowCheckedPtr" or similar.

And maybe we could also have a real BoundCheckedPtr too!
(In reply to Gerald Squelart [:gerald] from comment #0)
> And theoretically uintptr_t could be wider than a pointer, which could lead to
> truncations when converting back to a pointer.

What is the theoretical concern here?  Any object pointer must be convertable to void* and back without loss of information, and void* is guaranteed to be convertible to uintptr_t and back without loss of information...

Why not RangedPtr (or Span, which hsivonen is about to land elsewhere)?  It seems like bounds-checking things appropriately would eliminate any concerns about overflow...
(In reply to Nathan Froyd [:froydnj] from comment #5)
> (In reply to Gerald Squelart [:gerald] from comment #0)
> > And theoretically uintptr_t could be wider than a pointer, which could lead to
> > truncations when converting back to a pointer.
> 
> What is the theoretical concern here?  Any object pointer must be
> convertable to void* and back without loss of information, and void* is
> guaranteed to be convertible to uintptr_t and back without loss of
> information...

Ah, but of course uintptr_t *could* be larger than void*, which you stated!  Reading comprehension fail.  Though any implementation with such a uintptr_t would be kind of unusual, and would probably break any amount of working code...
(In reply to Nathan Froyd [:froydnj] from comment #5)
> Why not RangedPtr (or Span, which hsivonen is about to land elsewhere)?  It
> seems like bounds-checking things appropriately would eliminate any concerns
> about overflow...

If there's already something usable, great. I'll have a look, thanks.

Bounds-checking is not strictly enough though: You could have an overflow that goes so far around the memory as to end up inside the bounds again! E.g.: I think `++ptr + UINTPTR_MAX` overflows but goes back to the same location. Though I'm not sure it would then qualify as a risk, if we're still inside the bounds!?
FWIW, s390 has 31-bits for addresses and 32-bits for data.
Looking at RangePtr (thank you for the suggestion, Mike and Nathan), I don't think it's what I needed when I opened this bug: RangePtr is more like a developer's tool to find coding errors early through crash reports from debug builds.

What I need is a user-friendlier range checker that will never crash, but give me ways to perform pointer operations, find out if they fail (in all release channels), and give me access to successful results -- Just like CheckedInt does. ;-)

So if there's a way we can reuse RangePtr, or somehow combine it with CheckedInt, that would be fine.
But if reusing RangePtr, we would need options to make the checks run in non-debug builds too, and to make the checks non-crashing but instead make them toggle some visible valid/invalid bit (or just use nullptr to indicate invalid pointers?)
(In reply to Mike Hommey [:glandium] from comment #8)
> FWIW, s390 has 31-bits for addresses and 32-bits for data.

And there are odder architectures out there. Remember segmented addressing in x86 protected mode: 48 bit pointers, but 32 bit integers?

On uintptr_t, the C14 Standard, s.7.20.1.4(1) (working paper n1570) says this:

-------
The following type designates an unsigned integer type with the property that any valid
pointer to void can be converted to this type, then converted back to pointer to void,
and the result will compare equal to the original pointer:

uintptr_t

These types [meaning intptr_t and uintptr_t] are optional.
-------

That standard s.6.3.2.3(1) also says:

-------
A pointer to void may be converted to or from a pointer to any object type. A pointer to
any object type may be converted to a pointer to void and back again; the result shall
compare equal to the original pointer.
-------

But, nothing in all this guarantees that you can modify a validly-created uintptr_t, convert it back to the original pointer type, and have it work. Or even that you can modify a validly-created uintptr_t and use it for bounds checking; this wouldn't work on some segmented architectures for sure.
Another thought: If we did implement the equivalent of CheckedInt for pointers, we could use it to perform the overflow checks in RangePtr's MOZ_ASSERTs.
This way we would concentrate the not-obvious-to-get-right overflow-checking code in one spot, meaning less maintenance cost; and possible overflow-check optimizations (e.g., using GCC's __builtin_add_overflow) could be done there and would automatically benefit all callers.
(In reply to Gerald Squelart [:gerald] from comment #7)
> (In reply to Nathan Froyd [:froydnj] from comment #5)
> > Why not RangedPtr (or Span, which hsivonen is about to land elsewhere)?  It
> > seems like bounds-checking things appropriately would eliminate any concerns
> > about overflow...
> 
> If there's already something usable, great. I'll have a look, thanks.
> 
> Bounds-checking is not strictly enough though: You could have an overflow
> that goes so far around the memory as to end up inside the bounds again!
> E.g.: I think `++ptr + UINTPTR_MAX` overflows but goes back to the same
> location. Though I'm not sure it would then qualify as a risk, if we're
> still inside the bounds!?

Oh yes it does. Imagine that you wrap a 32-bit pointer twice, using a calculation derived from the nsTArray::Length() values returned by the elements of:

    nsTArray<nsTArray<char>> foo;

then you iterate foo and append the individual char arrays into the buffer using the wrapped pointer. As soon as you pass the middle element, bang!
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.