Skip to content

Commit

Permalink
Calling std::memcpy() with a null pointer is UB even with zero size.
Browse files Browse the repository at this point in the history
Apparently. Or at least UBSan tells me so. I never had a problem with
this in practice, it's just bullying. It also seems to complain for
std::memcmp() and std::memchr(), which I refuse to fix at this point.

Come on C, WHY do you think forcing users to do one extra branch in ALL
their code is beneficial in any way? It only adds extra testing burden
and expands surface for potential bugs.
  • Loading branch information
mosra committed Jan 10, 2021
1 parent ee9c721 commit 9ee4415
Show file tree
Hide file tree
Showing 6 changed files with 31 additions and 8 deletions.
4 changes: 4 additions & 0 deletions doc/corrade-changelog.dox
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,10 @@ namespace Corrade {
function, which changed signature in 2.0.5 and caused runtime failures
when `-s ASSERTIONS` was enabled. A public stable API is now used instead,
see [mosra/corrade#106](https://github.com/mosra/corrade/pull/106).
- @ref Containers::Array growable utilities, @ref Containers::String,
@ref Utility::copy(), @ref Utility::format() and @ref Utility::Sha1 was
fixed to not call @ref std::memcpy() with @cpp nullptr @ce when size is
zero (see [mosra/corrade#102](https://github.com/mosra/corrade/issues/102))

@subsection corrade-changelog-latest-deprecated Deprecated APIs

Expand Down
12 changes: 9 additions & 3 deletions src/Corrade/Containers/GrowableArray.h
Original file line number Diff line number Diff line change
Expand Up @@ -958,7 +958,9 @@ template<class T> inline void arrayMoveConstruct(T* const src, T* const dst, con
IsTriviallyCopyableOnOldGcc<T>::value
#endif
>::type* = nullptr) {
std::memcpy(dst, src, count*sizeof(T));
/* Apparently memcpy() can't be called with null pointers, even if size is
zero. I call that bullying. */
if(count) std::memcpy(dst, src, count*sizeof(T));
}

template<class T> inline void arrayMoveConstruct(T* src, T* dst, const std::size_t count, typename std::enable_if<!
Expand Down Expand Up @@ -986,7 +988,9 @@ template<class T> inline void arrayMoveAssign(T* const src, T* const dst, const
IsTriviallyCopyableOnOldGcc<T>::value
#endif
>::type* = nullptr) {
std::memcpy(dst, src, count*sizeof(T));
/* Apparently memcpy() can't be called with null pointers, even if size is
zero. I call that bullying. */
if(count) std::memcpy(dst, src, count*sizeof(T));
}

template<class T> inline void arrayMoveAssign(T* src, T* dst, const std::size_t count, typename std::enable_if<!
Expand All @@ -1009,7 +1013,9 @@ template<class T> inline void arrayCopyConstruct(const T* const src, T* const ds
IsTriviallyCopyableOnOldGcc<T>::value
#endif
>::type* = nullptr) {
std::memcpy(dst, src, count*sizeof(T));
/* Apparently memcpy() can't be called with null pointers, even if size is
zero. I call that bullying. */
if(count) std::memcpy(dst, src, count*sizeof(T));
}

template<class T> inline void arrayCopyConstruct(const T* src, T* dst, const std::size_t count, typename std::enable_if<!
Expand Down
8 changes: 6 additions & 2 deletions src/Corrade/Containers/String.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@ inline void String::construct(const char* data, std::size_t size) {
/* If the size is small enough for SSO, use that. Not using <= because we
need to store the null terminator as well. */
if(size < Implementation::SmallStringSize) {
std::memcpy(_small.data, data, size);
/* Apparently memcpy() can't be called with null pointers, even if size
is zero. I call that bullying. */
if(size) std::memcpy(_small.data, data, size);
_small.data[size] = '\0';
_small.size = size | SmallSize;

Expand Down Expand Up @@ -144,7 +146,9 @@ String::String(AllocatedInitT, const char* const data, const std::size_t size)
"Containers::String: received a null string of size" << size, );

_large.data = new char[size+1];
std::memcpy(_large.data, data, size);
/* Apparently memcpy() can't be called with null pointers, even if size is
zero. I call that bullying. */
if(size) std::memcpy(_large.data, data, size);
_large.data[size] = '\0';
_large.size = size;
_large.deleter = nullptr;
Expand Down
4 changes: 3 additions & 1 deletion src/Corrade/Utility/Algorithms.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ void copy(const Containers::ArrayView<const void>& src, const Containers::ArrayV
CORRADE_ASSERT(srcSize == dstSize,
"Utility::Algorithms::copy(): sizes" << srcSize << "and" << dstSize << "don't match", );

std::memcpy(dst.data(), src.data(), srcSize);
/* Apparently memcpy() can't be called with null pointers, even if size is
zero. I call that bullying. */
if(srcSize) std::memcpy(dst.data(), src.data(), srcSize);
}

void copy(const Containers::StridedArrayView1D<const char>& src, const Containers::StridedArrayView1D<char>& dst) {
Expand Down
7 changes: 6 additions & 1 deletion src/Corrade/Utility/Format.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,9 @@ std::size_t Formatter<Containers::StringView>::format(const Containers::ArrayVie
static_cast<void>(type);
#endif
/* strncpy() would stop on \0 characters */
if(buffer) std::memcpy(buffer, value.data(), size);
/* Apparently memcpy() can't be called with null pointers, even if size is
zero. I call that bullying. */
if(buffer && size) std::memcpy(buffer, value.data(), size);
return size;
}
void Formatter<Containers::StringView>::format(std::FILE* const file, const Containers::StringView value, const int precision, const FormatType type) {
Expand Down Expand Up @@ -394,6 +396,9 @@ std::size_t formatInto(const Containers::ArrayView<char>& buffer, const char* co
CORRADE_ASSERT(data.size() <= buffer.size(),
"Utility::formatInto(): buffer too small, expected at least" << bufferOffset + data.size() << "but got" << bufferOffset + buffer.size(), );
/* strncpy() would stop on \0 characters */
/* data.size() can't be 0 because that would make the above assert
fail, thus data can't be nullptr either and so we don't need to
check anything to avoid calling memcpy() with a null pointer */
std::memcpy(buffer + bufferOffset, data, data.size());
}
bufferOffset += data.size();
Expand Down
4 changes: 3 additions & 1 deletion src/Corrade/Utility/Sha1.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@ Sha1& Sha1::operator<<(Containers::ArrayView<const char> data) {
if(_bufferSize != 0) {
/* Not large enough, try it next time */
if(data.size() + _bufferSize < 64) {
std::memcpy(_buffer + _bufferSize, data.data(), data.size());
/* Apparently memcpy() can't be called with null pointers, even if
size is zero. I call that bullying. */
if(data.size()) std::memcpy(_buffer + _bufferSize, data.data(), data.size());
_bufferSize += data.size();
_dataSize += data.size();
return *this;
Expand Down

0 comments on commit 9ee4415

Please sign in to comment.