From 60624b569283e534ef9936744dfb02021e3ced7d Mon Sep 17 00:00:00 2001 From: Clemens Hammacher Date: Tue, 17 Sep 2019 13:59:59 +0200 Subject: [PATCH] Disallow nullptr arguments for {CopyChars} This allows to remove special casing for the {count == 0} case, which was needed because {memmove} does not accept {nullptr} arguments even if the {count} is zero. R=leszeks@chromium.org Bug: v8:9396 Change-Id: Iaef3cdbbffa74c2ba1c4e4501dafd943282cbcd9 Cq-Include-Trybots: luci.v8.try:v8_linux64_ubsan_rel_ng Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1807366 Reviewed-by: Leszek Swirski Reviewed-by: Ulan Degenbaev Commit-Queue: Clemens Hammacher Cr-Commit-Position: refs/heads/master@{#63838} --- include/v8config.h | 13 +++++++++++++ src/api/api.cc | 2 +- src/objects/string.cc | 4 ++-- src/strings/uri.cc | 12 ++++++++---- src/utils/memcopy.h | 10 +++++----- 5 files changed, 29 insertions(+), 12 deletions(-) diff --git a/include/v8config.h b/include/v8config.h index 7670c0e449..b6664ecf7b 100644 --- a/include/v8config.h +++ b/include/v8config.h @@ -170,6 +170,7 @@ // V8_HAS_ATTRIBUTE_ALWAYS_INLINE - __attribute__((always_inline)) // supported // V8_HAS_ATTRIBUTE_DEPRECATED - __attribute__((deprecated)) supported +// V8_HAS_ATTRIBUTE_NONNULL - __attribute__((nonnull)) supported // V8_HAS_ATTRIBUTE_NOINLINE - __attribute__((noinline)) supported // V8_HAS_ATTRIBUTE_UNUSED - __attribute__((unused)) supported // V8_HAS_ATTRIBUTE_VISIBILITY - __attribute__((visibility)) supported @@ -210,6 +211,7 @@ # define V8_HAS_ATTRIBUTE_DEPRECATED (__has_attribute(deprecated)) # define V8_HAS_ATTRIBUTE_DEPRECATED_MESSAGE \ (__has_extension(attribute_deprecated_with_message)) +# define V8_HAS_ATTRIBUTE_NONNULL (__has_attribute(nonnull)) # define V8_HAS_ATTRIBUTE_NOINLINE (__has_attribute(noinline)) # define V8_HAS_ATTRIBUTE_UNUSED (__has_attribute(unused)) # define V8_HAS_ATTRIBUTE_VISIBILITY (__has_attribute(visibility)) @@ -309,6 +311,17 @@ # define V8_ASSUME_ALIGNED(ptr) (ptr) #endif + +// A macro to mark specific arguments as non-null. +// Use like: +// int add(int* x, int y, int* z) V8_NONNULL(1, 3) { return *x + y + *z; } +#if V8_HAS_ATTRIBUTE_NONNULL +# define V8_NONNULL(...) __attribute__((nonnull(__VA_ARGS__))) +#else +# define V8_NONNULL(...) /* NOT SUPPORTED */ +#endif + + // A macro used to tell the compiler to never inline a particular function. // Don't bother for debug builds. // Use like: diff --git a/src/api/api.cc b/src/api/api.cc index dd022c34b8..8af1d7488b 100644 --- a/src/api/api.cc +++ b/src/api/api.cc @@ -5289,7 +5289,7 @@ static inline int WriteHelper(i::Isolate* isolate, const String* string, int end = start + length; if ((length == -1) || (length > str->length() - start)) end = str->length(); if (end < 0) return 0; - i::String::WriteToFlat(*str, buffer, start, end); + if (start < end) i::String::WriteToFlat(*str, buffer, start, end); if (!(options & String::NO_NULL_TERMINATION) && (length == -1 || end - start < length)) { buffer[end - start] = '\0'; diff --git a/src/objects/string.cc b/src/objects/string.cc index 88882f70ea..c8c85f3793 100644 --- a/src/objects/string.cc +++ b/src/objects/string.cc @@ -601,9 +601,8 @@ void String::WriteToFlat(String src, sinkchar* sink, int f, int t) { String source = src; int from = f; int to = t; - while (true) { + while (from < to) { DCHECK_LE(0, from); - DCHECK_LE(from, to); DCHECK_LE(to, source.length()); switch (StringShape(source).full_representation_tag()) { case kOneByteStringTag | kExternalStringTag: { @@ -681,6 +680,7 @@ void String::WriteToFlat(String src, sinkchar* sink, int f, int t) { break; } } + DCHECK_EQ(from, to); } template diff --git a/src/strings/uri.cc b/src/strings/uri.cc index 430c8dd0eb..de4e339b39 100644 --- a/src/strings/uri.cc +++ b/src/strings/uri.cc @@ -195,10 +195,14 @@ MaybeHandle Uri::Decode(Isolate* isolate, Handle uri, String); DisallowHeapAllocation no_gc; - CopyChars(result->GetChars(no_gc), one_byte_buffer.data(), - one_byte_buffer.size()); - CopyChars(result->GetChars(no_gc) + one_byte_buffer.size(), - two_byte_buffer.data(), two_byte_buffer.size()); + uc16* chars = result->GetChars(no_gc); + if (!one_byte_buffer.empty()) { + CopyChars(chars, one_byte_buffer.data(), one_byte_buffer.size()); + chars += one_byte_buffer.size(); + } + if (!two_byte_buffer.empty()) { + CopyChars(chars, two_byte_buffer.data(), two_byte_buffer.size()); + } return result; } diff --git a/src/utils/memcopy.h b/src/utils/memcopy.h index fb6ddc6692..2464daf8bd 100644 --- a/src/utils/memcopy.h +++ b/src/utils/memcopy.h @@ -200,16 +200,16 @@ inline void MemsetPointer(T** dest, U* value, size_t counter) { // Copy from 8bit/16bit chars to 8bit/16bit chars. Values are zero-extended if // needed. Ranges are not allowed to overlap unless the type sizes match (hence // {memmove} is used internally). +// The separate declaration is needed for the V8_NONNULL, which is not allowed +// on a definition. +template +void CopyChars(DstType* dst, const SrcType* src, size_t count) V8_NONNULL(1, 2); + template void CopyChars(DstType* dst, const SrcType* src, size_t count) { STATIC_ASSERT(std::is_integral::value); STATIC_ASSERT(std::is_integral::value); - // This special case for {count == 0} allows to pass {nullptr} as {dst} or - // {src} in that case. - // TODO(clemensh): Remove this and make {dst} and {src} nonnull. - if (count == 0) return; - // If the size of {SrcType} and {DstType} matches, we switch to the more // general and potentially faster {memmove}. Note that this decision is made // statically at compile time.