From 46d10dc7dcacd93ede56b481c66c29a31311abbc Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Tue, 3 Dec 2019 13:09:29 -0800 Subject: [PATCH] Fix "null passed as argument declared not null" Fix several such reports from ubsan E.g. /Users/anoyes/workspace/foundationdb/flow/Arena.h:794:16: runtime error: null pointer passed as argument 1, which is declared to never be null --- fdbclient/SnapshotCache.h | 8 ++-- fdbrpc/FlowTransport.actor.cpp | 4 +- fdbserver/DiskQueue.actor.cpp | 4 +- fdbserver/SkipList.cpp | 4 +- flow/Arena.h | 71 +++++++++++++++++++++++----------- flow/serialize.h | 12 ++++-- 6 files changed, 71 insertions(+), 32 deletions(-) diff --git a/fdbclient/SnapshotCache.h b/fdbclient/SnapshotCache.h index 9fca5cafdf..6b7bf69fd7 100644 --- a/fdbclient/SnapshotCache.h +++ b/fdbclient/SnapshotCache.h @@ -65,10 +65,12 @@ struct ExtStringRef { int size() const { return base.size() + extra_zero_bytes; } - int cmp( ExtStringRef const& rhs ) const { + int cmp(ExtStringRef const& rhs) const { int cbl = std::min(base.size(), rhs.base.size()); - int c = memcmp( base.begin(), rhs.base.begin(), cbl ); - if (c!=0) return c; + if (cbl > 0) { + int c = memcmp(base.begin(), rhs.base.begin(), cbl); + if (c != 0) return c; + } for(int i=cbl; i connectionReader( const int unproc_len = unprocessed_end - unprocessed_begin; const int len = getNewBufferSize(unprocessed_begin, unprocessed_end, peerAddress); uint8_t* const newBuffer = new (newArena) uint8_t[ len ]; - memcpy( newBuffer, unprocessed_begin, unproc_len ); + if (unproc_len > 0) { + memcpy(newBuffer, unprocessed_begin, unproc_len); + } arena = newArena; unprocessed_begin = newBuffer; unprocessed_end = newBuffer + unproc_len; diff --git a/fdbserver/DiskQueue.actor.cpp b/fdbserver/DiskQueue.actor.cpp index cfcbe091a0..023ca7e4cd 100644 --- a/fdbserver/DiskQueue.actor.cpp +++ b/fdbserver/DiskQueue.actor.cpp @@ -92,7 +92,9 @@ struct StringBuffer { uint8_t* p = (uint8_t*)(int64_t(b+alignment-1) & ~(alignment-1)); // first multiple of alignment greater than or equal to b ASSERT( p>=b && p+reserved<=e && int64_t(p)%alignment == 0 ); - memcpy(p, str.begin(), str.size()); + if (str.size() > 0) { + memcpy(p, str.begin(), str.size()); + } ref() = StringRef( p, str.size() ); } } diff --git a/fdbserver/SkipList.cpp b/fdbserver/SkipList.cpp index 62f22a66b9..0d2dc8b707 100644 --- a/fdbserver/SkipList.cpp +++ b/fdbserver/SkipList.cpp @@ -343,7 +343,9 @@ private: n->nPointers = level+1; n->valueLength = value.size(); - memcpy(n->value(), value.begin(), value.size()); + if (value.size() > 0) { + memcpy(n->value(), value.begin(), value.size()); + } return n; } diff --git a/flow/Arena.h b/flow/Arena.h index 3af189c8b4..45de490a9f 100644 --- a/flow/Arena.h +++ b/flow/Arena.h @@ -546,7 +546,9 @@ public: constexpr static FileIdentifier file_identifier = 13300811; StringRef() : data(0), length(0) {} StringRef( Arena& p, const StringRef& toCopy ) : data( new (p) uint8_t[toCopy.size()] ), length( toCopy.size() ) { - memcpy( (void*)data, toCopy.data, length ); + if (length > 0) { + memcpy((void*)data, toCopy.data, length); + } } StringRef( Arena& p, const std::string& toCopy ) : length( (int)toCopy.size() ) { UNSTOPPABLE_ASSERT( toCopy.size() <= std::numeric_limits::max()); @@ -554,7 +556,9 @@ public: if (length) memcpy( (void*)data, &toCopy[0], length ); } StringRef( Arena& p, const uint8_t* toCopy, int length ) : data( new (p) uint8_t[length] ), length(length) { - memcpy( (void*)data, toCopy, length ); + if (length > 0) { + memcpy((void*)data, toCopy, length); + } } StringRef( const uint8_t* data, int length ) : data(data), length(length) {} StringRef( const std::string& s ) : data((const uint8_t*)s.c_str()), length((int)s.size()) { @@ -573,17 +577,25 @@ public: bool startsWith( const StringRef& s ) const { return size() >= s.size() && !memcmp(begin(), s.begin(), s.size()); } bool endsWith( const StringRef& s ) const { return size() >= s.size() && !memcmp(end()-s.size(), s.begin(), s.size()); } - StringRef withPrefix( const StringRef& prefix, Arena& arena ) const { - uint8_t* s = new (arena) uint8_t[ prefix.size() + size() ]; - memcpy(s, prefix.begin(), prefix.size()); - memcpy(s+prefix.size(), begin(), size()); - return StringRef(s,prefix.size() + size()); + StringRef withPrefix(const StringRef& prefix, Arena& arena) const { + uint8_t* s = new (arena) uint8_t[prefix.size() + size()]; + if (prefix.size() > 0) { + memcpy(s, prefix.begin(), prefix.size()); + } + if (size() > 0) { + memcpy(s + prefix.size(), begin(), size()); + } + return StringRef(s, prefix.size() + size()); } StringRef withSuffix( const StringRef& suffix, Arena& arena ) const { uint8_t* s = new (arena) uint8_t[ suffix.size() + size() ]; - memcpy(s, begin(), size()); - memcpy(s+size(), suffix.begin(), suffix.size()); + if (size() > 0) { + memcpy(s, begin(), size()); + } + if (suffix.size() > 0) { + memcpy(s + size(), suffix.begin(), suffix.size()); + } return StringRef(s,suffix.size() + size()); } @@ -644,9 +656,11 @@ public: int expectedSize() const { return size(); } - int compare( StringRef const& other ) const { - int c = memcmp( begin(), other.begin(), std::min( size(), other.size() ) ); - if (c!=0) return c; + int compare(StringRef const& other) const { + if (std::min(size(), other.size()) > 0) { + int c = memcmp(begin(), other.begin(), std::min(size(), other.size())); + if (c != 0) return c; + } return size() - other.size(); } @@ -782,17 +796,24 @@ struct dynamic_size_traits : std::true_type { } }; -inline bool operator == (const StringRef& lhs, const StringRef& rhs ) { +inline bool operator==(const StringRef& lhs, const StringRef& rhs) { + if (lhs.size() == 0 && rhs.size() == 0) { + return true; + } return lhs.size() == rhs.size() && !memcmp(lhs.begin(), rhs.begin(), lhs.size()); } -inline bool operator < ( const StringRef& lhs, const StringRef& rhs ) { - int c = memcmp( lhs.begin(), rhs.begin(), std::min(lhs.size(), rhs.size()) ); - if (c!=0) return c<0; +inline bool operator<(const StringRef& lhs, const StringRef& rhs) { + if (std::min(lhs.size(), rhs.size()) > 0) { + int c = memcmp(lhs.begin(), rhs.begin(), std::min(lhs.size(), rhs.size())); + if (c != 0) return c < 0; + } return lhs.size() < rhs.size(); } -inline bool operator > ( const StringRef& lhs, const StringRef& rhs ) { - int c = memcmp( lhs.begin(), rhs.begin(), std::min(lhs.size(), rhs.size()) ); - if (c!=0) return c>0; +inline bool operator>(const StringRef& lhs, const StringRef& rhs) { + if (std::min(lhs.size(), rhs.size()) > 0) { + int c = memcmp(lhs.begin(), rhs.begin(), std::min(lhs.size(), rhs.size())); + if (c != 0) return c > 0; + } return lhs.size() > rhs.size(); } inline bool operator != (const StringRef& lhs, const StringRef& rhs ) { return !(lhs==rhs); } @@ -903,7 +924,9 @@ public: VectorRef(Arena& p, const VectorRef& toCopy, typename std::enable_if::value, int>::type = 0) : VPS(toCopy), data((T*)new (p) uint8_t[sizeof(T) * toCopy.size()]), m_size(toCopy.size()), m_capacity(toCopy.size()) { - memcpy(data, toCopy.data, m_size * sizeof(T)); + if (m_size > 0) { + memcpy(data, toCopy.data, m_size * sizeof(T)); + } } // Arena constructor for Ref types, which must have an Arena constructor @@ -997,7 +1020,9 @@ public: void append(Arena& p, const T* begin, int count) { if (m_size + count > m_capacity) reallocate(p, m_size + count); VPS::invalidate(); - memcpy(data + m_size, begin, sizeof(T) * count); + if (count > 0) { + memcpy(data + m_size, begin, sizeof(T) * count); + } m_size += count; } template @@ -1062,7 +1087,9 @@ private: requiredCapacity = std::max(m_capacity * 2, requiredCapacity); // SOMEDAY: Maybe we are right at the end of the arena and can expand cheaply T* newData = (T*)new (p) uint8_t[requiredCapacity * sizeof(T)]; - memcpy(newData, data, m_size * sizeof(T)); + if (m_size > 0) { + memcpy(newData, data, m_size * sizeof(T)); + } data = newData; m_capacity = requiredCapacity; } diff --git a/flow/serialize.h b/flow/serialize.h index 47c3a0ab79..d355ca90d4 100644 --- a/flow/serialize.h +++ b/flow/serialize.h @@ -324,9 +324,11 @@ public: serializeBytes(bytes.begin(), bytes.size()); } void serializeBytes(const void* data, int bytes) { - valgrindCheck( data, bytes, "serializeBytes" ); - void* p = writeBytes(bytes); - memcpy(p, data, bytes); + if (bytes > 0) { + valgrindCheck(data, bytes, "serializeBytes"); + void* p = writeBytes(bytes); + memcpy(p, data, bytes); + } } template void serializeBinaryItem( const T& t ) { @@ -456,7 +458,9 @@ private: } Arena newArena; uint8_t* newData = new ( newArena ) uint8_t[ allocated ]; - memcpy(newData, data, p); + if (p > 0) { + memcpy(newData, data, p); + } arena = newArena; data = newData; }