From 5d65a9fa7b9cd0111a67c91f4ce3cc6fbd25a5d0 Mon Sep 17 00:00:00 2001 From: Soheil Hassas Yeganeh Date: Mon, 10 Jun 2019 15:07:39 -0400 Subject: [PATCH] Address comments from Vijay. --- src/core/ext/transport/chttp2/transport/frame_data.cc | 8 ++++---- src/core/lib/gpr/string.cc | 9 ++++++++- src/core/lib/gpr/string.h | 7 ++++++- src/core/lib/slice/slice.cc | 9 +++++++-- src/core/lib/slice/slice_internal.h | 2 ++ src/core/lib/slice/slice_string_helpers.cc | 9 +++++++++ src/core/lib/slice/slice_string_helpers.h | 2 ++ src/core/lib/surface/validate_metadata.cc | 3 +-- test/core/slice/slice_test.cc | 10 ++++++++++ 9 files changed, 49 insertions(+), 10 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/frame_data.cc b/src/core/ext/transport/chttp2/transport/frame_data.cc index c50d7e48d41..74c305b820f 100644 --- a/src/core/ext/transport/chttp2/transport/frame_data.cc +++ b/src/core/ext/transport/chttp2/transport/frame_data.cc @@ -137,10 +137,10 @@ grpc_error* grpc_deframe_unprocessed_incoming_frames( p->error = grpc_error_set_int(p->error, GRPC_ERROR_INT_STREAM_ID, static_cast(s->id)); gpr_free(msg); - p->error = grpc_error_set_str( - p->error, GRPC_ERROR_STR_RAW_BYTES, - grpc_slice_from_moved_string(grpc_core::UniquePtr( - grpc_dump_slice(*slice, GPR_DUMP_HEX | GPR_DUMP_ASCII)))); + p->error = + grpc_error_set_str(p->error, GRPC_ERROR_STR_RAW_BYTES, + grpc_dump_slice_to_slice( + *slice, GPR_DUMP_HEX | GPR_DUMP_ASCII)); p->error = grpc_error_set_int(p->error, GRPC_ERROR_INT_OFFSET, cur - beg); p->state = GRPC_CHTTP2_DATA_ERROR; diff --git a/src/core/lib/gpr/string.cc b/src/core/lib/gpr/string.cc index 31d5fdee5be..a39f56ef926 100644 --- a/src/core/lib/gpr/string.cc +++ b/src/core/lib/gpr/string.cc @@ -126,7 +126,8 @@ static void asciidump(dump_out* out, const char* buf, size_t len) { } } -char* gpr_dump(const char* buf, size_t len, uint32_t flags) { +char* gpr_dump_return_len(const char* buf, size_t len, uint32_t flags, + size_t* out_len) { dump_out out = dump_out_create(); if (flags & GPR_DUMP_HEX) { hexdump(&out, buf, len); @@ -135,9 +136,15 @@ char* gpr_dump(const char* buf, size_t len, uint32_t flags) { asciidump(&out, buf, len); } dump_out_append(&out, 0); + *out_len = out.length; return out.data; } +char* gpr_dump(const char* buf, size_t len, uint32_t flags) { + size_t unused; + return gpr_dump_return_len(buf, len, flags, &unused); +} + int gpr_parse_bytes_to_uint32(const char* buf, size_t len, uint32_t* result) { uint32_t out = 0; uint32_t new_val; diff --git a/src/core/lib/gpr/string.h b/src/core/lib/gpr/string.h index c5efcec3bb1..bf59db7abfe 100644 --- a/src/core/lib/gpr/string.h +++ b/src/core/lib/gpr/string.h @@ -32,9 +32,14 @@ #define GPR_DUMP_HEX 0x00000001 #define GPR_DUMP_ASCII 0x00000002 -/* Converts array buf, of length len, into a C string according to the flags. +/* Converts array buf, of length len, into a C string according to the flags. Result should be freed with gpr_free() */ char* gpr_dump(const char* buf, size_t len, uint32_t flags); +/* Converts array buf, of length len, into a C string according to the flags. + The length of the returned buffer is stored in out_len. + Result should be freed with gpr_free() */ +char* gpr_dump_return_len(const char* buf, size_t len, uint32_t flags, + size_t* out_len); /* Parses an array of bytes into an integer (base 10). Returns 1 on success, 0 on failure. */ diff --git a/src/core/lib/slice/slice.cc b/src/core/lib/slice/slice.cc index 0f8ea6ae018..eebf66bb882 100644 --- a/src/core/lib/slice/slice.cc +++ b/src/core/lib/slice/slice.cc @@ -216,8 +216,8 @@ grpc_slice grpc_slice_from_copied_string(const char* source) { return grpc_slice_from_copied_buffer(source, strlen(source)); } -grpc_slice grpc_slice_from_moved_string(grpc_core::UniquePtr p) { - const size_t len = strlen(p.get()); +grpc_slice grpc_slice_from_moved_buffer(grpc_core::UniquePtr p, + size_t len) { uint8_t* ptr = reinterpret_cast(p.get()); grpc_slice slice; if (len <= sizeof(slice.data.inlined.bytes)) { @@ -234,6 +234,11 @@ grpc_slice grpc_slice_from_moved_string(grpc_core::UniquePtr p) { return slice; } +grpc_slice grpc_slice_from_moved_string(grpc_core::UniquePtr p) { + const size_t len = strlen(p.get()); + return grpc_slice_from_moved_buffer(std::move(p), len); +} + namespace { class MallocRefCount { diff --git a/src/core/lib/slice/slice_internal.h b/src/core/lib/slice/slice_internal.h index 457b76700ed..c6943fe6563 100644 --- a/src/core/lib/slice/slice_internal.h +++ b/src/core/lib/slice/slice_internal.h @@ -303,6 +303,8 @@ inline uint32_t grpc_slice_hash_internal(const grpc_slice& s) { : grpc_slice_hash_refcounted(s); } +grpc_slice grpc_slice_from_moved_buffer(grpc_core::UniquePtr p, + size_t len); grpc_slice grpc_slice_from_moved_string(grpc_core::UniquePtr p); // Returns the memory used by this slice, not counting the slice structure diff --git a/src/core/lib/slice/slice_string_helpers.cc b/src/core/lib/slice/slice_string_helpers.cc index c2392fd392f..7887e0305fb 100644 --- a/src/core/lib/slice/slice_string_helpers.cc +++ b/src/core/lib/slice/slice_string_helpers.cc @@ -25,6 +25,7 @@ #include #include "src/core/lib/gpr/string.h" +#include "src/core/lib/gprpp/memory.h" #include "src/core/lib/slice/slice_internal.h" char* grpc_dump_slice(const grpc_slice& s, uint32_t flags) { @@ -32,6 +33,14 @@ char* grpc_dump_slice(const grpc_slice& s, uint32_t flags) { GRPC_SLICE_LENGTH(s), flags); } +grpc_slice grpc_dump_slice_to_slice(const grpc_slice& s, uint32_t flags) { + size_t len; + grpc_core::UniquePtr ptr( + gpr_dump_return_len(reinterpret_cast GRPC_SLICE_START_PTR(s), + GRPC_SLICE_LENGTH(s), flags, &len)); + return grpc_slice_from_moved_buffer(std::move(ptr), len); +} + /** Finds the initial (\a begin) and final (\a end) offsets of the next * substring from \a str + \a read_offset until the next \a sep or the end of \a * str. diff --git a/src/core/lib/slice/slice_string_helpers.h b/src/core/lib/slice/slice_string_helpers.h index cb1df658fa5..6551a6df75d 100644 --- a/src/core/lib/slice/slice_string_helpers.h +++ b/src/core/lib/slice/slice_string_helpers.h @@ -31,6 +31,8 @@ /* Calls gpr_dump on a slice. */ char* grpc_dump_slice(const grpc_slice& slice, uint32_t flags); +/* Calls gpr_dump on a slice and returns the result as a slice. */ +grpc_slice grpc_dump_slice_to_slice(const grpc_slice& slice, uint32_t flags); /** Split \a str by the separator \a sep. Results are stored in \a dst, which * should be a properly initialized instance. */ diff --git a/src/core/lib/surface/validate_metadata.cc b/src/core/lib/surface/validate_metadata.cc index 46554596235..0f65091333d 100644 --- a/src/core/lib/surface/validate_metadata.cc +++ b/src/core/lib/surface/validate_metadata.cc @@ -40,13 +40,12 @@ static grpc_error* conforms_to(const grpc_slice& slice, int byte = idx / 8; int bit = idx % 8; if ((legal_bits[byte] & (1 << bit)) == 0) { - char* dump = grpc_dump_slice(slice, GPR_DUMP_HEX | GPR_DUMP_ASCII); grpc_error* error = grpc_error_set_str( grpc_error_set_int(GRPC_ERROR_CREATE_FROM_COPIED_STRING(err_desc), GRPC_ERROR_INT_OFFSET, p - GRPC_SLICE_START_PTR(slice)), GRPC_ERROR_STR_RAW_BYTES, - grpc_slice_from_moved_string(grpc_core::UniquePtr(dump))); + grpc_dump_slice_to_slice(slice, GPR_DUMP_HEX | GPR_DUMP_ASCII)); return error; } } diff --git a/test/core/slice/slice_test.cc b/test/core/slice/slice_test.cc index 3c9c0572a43..4f824cbd5ad 100644 --- a/test/core/slice/slice_test.cc +++ b/test/core/slice/slice_test.cc @@ -311,6 +311,16 @@ static void test_moved_string_slice(void) { reinterpret_cast(large_ptr)); grpc_slice_unref(large); + // Moved buffer must respect the provided length not the actual length of the + // string. + large_ptr = strdup(kSLargeStr); + small = grpc_slice_from_moved_buffer(grpc_core::UniquePtr(large_ptr), + strlen(kSmallStr)); + GPR_ASSERT(GRPC_SLICE_LENGTH(small) == strlen(kSmallStr)); + GPR_ASSERT(GRPC_SLICE_START_PTR(small) != + reinterpret_cast(large_ptr)); + grpc_slice_unref(small); + grpc_shutdown(); }