Open
Bug 1347384
Opened 7 years ago
Updated 2 years ago
Allow CheckedInt<T*>, or add CheckedPtr<T*>
Categories
(Core :: MFBT, enhancement)
Tracking
()
NEW
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->().
Comment 1•7 years ago
|
||
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.
Reporter | ||
Comment 2•7 years ago
|
||
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).
Comment 3•7 years ago
|
||
To bikeshed a little, I feel like "CheckedPtr" overpromises. (It sort of sounds like it might do bounds checking.)
Reporter | ||
Comment 4•7 years ago
|
||
Makes sense, Andrew. Suggestions? Most verbose would be "OverflowCheckedPtr" or similar. And maybe we could also have a real BoundCheckedPtr too!
Comment 5•7 years ago
|
||
(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...
Comment 6•7 years ago
|
||
(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...
Reporter | ||
Comment 7•7 years ago
|
||
(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!?
Comment 8•7 years ago
|
||
FWIW, s390 has 31-bits for addresses and 32-bits for data.
Reporter | ||
Comment 9•7 years ago
|
||
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?)
Comment 10•7 years ago
|
||
(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.
Reporter | ||
Comment 11•7 years ago
|
||
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.
Comment 12•7 years ago
|
||
(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!
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•