CTSRD-CHERI / clang

DO NOT USE. Use llvm-project instead
Other
9 stars 8 forks source link

Should modulo operation on uintcap_t be an error by default? #191

Closed arichardson closed 6 years ago

arichardson commented 6 years ago

I just ran into the following assertion when running qt unit tests:

void QRandomGenerator::_fillRange(void *buffer, void *bufferEnd)
{
    // Verify that the pointers are properly aligned for 32-bit
    Q_ASSERT(quintptr(buffer) % sizeof(quint32) == 0);
    Q_ASSERT(quintptr(bufferEnd) % sizeof(quint32) == 0);

I am quite surprised that this triggered since it means that offset != 0, i.e. it has an unaligned base. Changing this to qvaddr instead of quintptr fixed the assertion. (ideally this would use __builtin_is_aligned() but it is also used when building the host tools so that is not available.)

Does it ever make sense to use modulo operations on a uintcap_t?

I have a warning for this but I am not sure whether to also make it DefaultError.

brooksdavis commented 6 years ago

It usually doesn't make sense, but if you have enough knowledge of the buffer allocation it's sometimes it's not wrong. If you have malloced storage or the underlying buffer is fairly strongly aligned then you can generally assume that base has a reasonable alignment (anything malloced that is larger than sizeof(void * __capability) is capability aligned.) Subject to dealing with any fallout, I think it would be ok to make into an error.

davidchisnall commented 6 years ago

In the general case, I don't think we want to prevent it. intptr_t may be used for storing non-pointer-derived integers. In particular, it can be used as a storage type for pointers but an arithmetic type for integers. Ideally, we want a static analyser that will tell you if you do a cast from an pointer to a __[u]intcap_t and then do a modulo operation on it (or, basically, any arithmetic other than plus and minus).