possible invalid usage of memcpy/memcmp
Dushistov opened this issue · 3 comments
At least for glibc
extern int memcmp (const void *__s1, const void *__s2, size_t __n)
__THROW __attribute_pure__ __nonnull ((1, 2));
extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
size_t __n) __THROW __nonnull ((1, 2));
so memcpy and memcmp expect nonnull arguments.
But I saw a log of warnings about possible misuse of this API when I run clang static analyzer against couchbase-lite-core:
In file included from /home/evgeniy/bigdisk1/projects/cpp-infra/couchbase-lite-core/LiteCore/Database/TreeDocument.cc:19:
In file included from /home/evgeniy/bigdisk1/projects/cpp-infra/couchbase-lite-core/LiteCore/Database/TreeDocument.hh:20:
In file included from /home/evgeniy/bigdisk1/projects/cpp-infra/couchbase-lite-core/LiteCore/Database/Document.hh:20:
In file included from /home/evgeniy/bigdisk1/projects/cpp-infra/couchbase-lite-core/C/c4Internal.hh:21:
In file included from /home/evgeniy/bigdisk1/projects/cpp-infra/couchbase-lite-core/LiteCore/Support/Base.hh:21:
/home/evgeniy/bigdisk1/projects/cpp-infra/couchbase-lite-core/vendor/fleece/API/fleece/slice.hh:125:66: warning: Null pointer passed to 2nd parameter expecting 'nonnull'
memcmp(buf, s.buf, size) == 0;}
^
/home/evgeniy/bigdisk1/projects/cpp-infra/couchbase-lite-core/LiteCore/Database/TreeDocument.cc:277:17: note: Assuming 'winningRev' is non-null
if (!winningRev || !losingRev)
^~~~~~~~~~~
/home/evgeniy/bigdisk1/projects/cpp-infra/couchbase-lite-core/LiteCore/Database/TreeDocument.cc:277:17: note: Left side of '||' is false
/home/evgeniy/bigdisk1/projects/cpp-infra/couchbase-lite-core/LiteCore/Database/TreeDocument.cc:277:32: note: Assuming 'losingRev' is non-null
if (!winningRev || !losingRev)
^~~~~~~~~~
/home/evgeniy/bigdisk1/projects/cpp-infra/couchbase-lite-core/LiteCore/Database/TreeDocument.cc:277:13: note: Taking false branch
if (!winningRev || !losingRev)
^
/home/evgeniy/bigdisk1/projects/cpp-infra/couchbase-lite-core/LiteCore/Database/TreeDocument.cc:279:17: note: Left side of '||' is false
if (!winningRev->isLeaf() || !losingRev->isLeaf())
the same for memcpy:
/home/evgeniy/bigdisk1/projects/cpp-infra/couchbase-lite-core/vendor/fleece/Fleece/API_Impl/Fleece.cc:57:5: warning: Null pointer passed to 2nd parameter expecting 'nonnull'
memcpy(cstr, json.buf, json.size);
^
/home/evgeniy/bigdisk1/projects/cpp-infra/couchbase-lite-core/vendor/fleece/Fleece/API_Impl/Fleece.cc:63:12: note: Calling 'FLDump'
return FLDump(Value::fromData(data));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/evgeniy/bigdisk1/projects/cpp-infra/couchbase-lite-core/vendor/fleece/Fleece/API_Impl/Fleece.cc:55:5: note: 'json.buf' initialized to a null pointer value
FLStringResult json = FLValue_ToJSON(v);
^~~~~~~~~~~~~~~~~~~
/home/evgeniy/bigdisk1/projects/cpp-infra/couchbase-lite-core/vendor/fleece/Fleece/API_Impl/Fleece.cc:57:5: note: Null pointer passed to 2nd parameter expecting 'nonnull'
memcpy(cstr, json.buf, json.size);
I've never seen that as a requirement for memcmp/memcpy before, not if the size is 0. The Clang UB sanitizer and address sanitizer do not warn about this.
My last comment in that thread:
I believe it is impossible for a correct implementation of memcpy/memmove/memcmp to dereference the source or destination pointer if the size is 0. Because those pointers could legally point to a boundary between mapped and unmapped memory.
I think the only reason the standard defines those pointers as non-null is because (a) NULL is never a valid address nowadays ... , (b) there are mechanisms to tell the compiler that a parameter must be non-null, and (c) there are not mechanisms to tell the compiler the full restriction that "these pointers must be non-null but only if this other parameter is nonzero."
And may be you missed the point that compiler also know about undefined behavior
Yeah, the fact that libstdc++ went as far as to declare the parameters nonnull means that the compiler can now do the kind of mis-optimization you pointed out. So I guess we have to fix this.