diff --git a/src/core/ext/transport/chttp2/transport/frame_data.cc b/src/core/ext/transport/chttp2/transport/frame_data.cc index 3734c0150b7..74c305b820f 100644 --- a/src/core/ext/transport/chttp2/transport/frame_data.cc +++ b/src/core/ext/transport/chttp2/transport/frame_data.cc @@ -109,7 +109,6 @@ grpc_error* grpc_deframe_unprocessed_incoming_frames( end = GRPC_SLICE_END_PTR(*slice); cur = beg; uint32_t message_flags; - char* msg; if (cur == end) { grpc_slice_buffer_remove_first(slices); @@ -132,15 +131,16 @@ grpc_error* grpc_deframe_unprocessed_incoming_frames( p->is_frame_compressed = true; /* GPR_TRUE */ break; default: + char* msg; gpr_asprintf(&msg, "Bad GRPC frame type 0x%02x", p->frame_type); p->error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(msg); p->error = grpc_error_set_int(p->error, GRPC_ERROR_INT_STREAM_ID, static_cast(s->id)); gpr_free(msg); - msg = grpc_dump_slice(*slice, GPR_DUMP_HEX | GPR_DUMP_ASCII); - p->error = grpc_error_set_str(p->error, GRPC_ERROR_STR_RAW_BYTES, - grpc_slice_from_copied_string(msg)); - gpr_free(msg); + 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/ext/transport/chttp2/transport/frame_rst_stream.cc b/src/core/ext/transport/chttp2/transport/frame_rst_stream.cc index ccde36cbc48..cda09c3dea1 100644 --- a/src/core/ext/transport/chttp2/transport/frame_rst_stream.cc +++ b/src/core/ext/transport/chttp2/transport/frame_rst_stream.cc @@ -26,6 +26,7 @@ #include #include "src/core/ext/transport/chttp2/transport/frame.h" +#include "src/core/lib/gprpp/memory.h" #include "src/core/lib/transport/http2_errors.h" grpc_slice grpc_chttp2_rst_stream_create(uint32_t id, uint32_t code, @@ -102,9 +103,9 @@ grpc_error* grpc_chttp2_rst_stream_parser_parse(void* parser, error = grpc_error_set_int( grpc_error_set_str(GRPC_ERROR_CREATE_FROM_STATIC_STRING("RST_STREAM"), GRPC_ERROR_STR_GRPC_MESSAGE, - grpc_slice_from_copied_string(message)), + grpc_slice_from_moved_string( + grpc_core::UniquePtr(message))), GRPC_ERROR_INT_HTTP2_ERROR, static_cast(reason)); - gpr_free(message); } grpc_chttp2_mark_stream_closed(t, s, true, true, 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/http/httpcli.cc b/src/core/lib/http/httpcli.cc index 8a8da8b1604..93bb1432b16 100644 --- a/src/core/lib/http/httpcli.cc +++ b/src/core/lib/http/httpcli.cc @@ -28,6 +28,7 @@ #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/gpr/string.h" +#include "src/core/lib/gprpp/memory.h" #include "src/core/lib/http/format_request.h" #include "src/core/lib/http/parser.h" #include "src/core/lib/iomgr/endpoint.h" @@ -112,12 +113,11 @@ static void append_error(internal_request* req, grpc_error* error) { GRPC_ERROR_CREATE_FROM_STATIC_STRING("Failed HTTP/1 client request"); } grpc_resolved_address* addr = &req->addresses->addrs[req->next_address - 1]; - char* addr_text = grpc_sockaddr_to_uri(addr); + grpc_core::UniquePtr addr_text(grpc_sockaddr_to_uri(addr)); req->overall_error = grpc_error_add_child( req->overall_error, grpc_error_set_str(error, GRPC_ERROR_STR_TARGET_ADDRESS, - grpc_slice_from_copied_string(addr_text))); - gpr_free(addr_text); + grpc_slice_from_moved_string(std::move(addr_text)))); } static void do_read(internal_request* req) { diff --git a/src/core/lib/slice/slice.cc b/src/core/lib/slice/slice.cc index 566ad94d705..eebf66bb882 100644 --- a/src/core/lib/slice/slice.cc +++ b/src/core/lib/slice/slice.cc @@ -102,18 +102,19 @@ class NewSliceRefcount { } NewSliceRefcount(void (*destroy)(void*), void* user_data) - : rc_(grpc_slice_refcount::Type::REGULAR, &refs_, Destroy, this, &rc_), + : base_(grpc_slice_refcount::Type::REGULAR, &refs_, Destroy, this, + &base_), user_destroy_(destroy), user_data_(user_data) {} GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE - grpc_slice_refcount* base_refcount() { return &rc_; } + grpc_slice_refcount* base_refcount() { return &base_; } private: ~NewSliceRefcount() { user_destroy_(user_data_); } - grpc_slice_refcount rc_; + grpc_slice_refcount base_; RefCount refs_; void (*user_destroy_)(void*); void* user_data_; @@ -141,7 +142,6 @@ grpc_slice grpc_slice_new(void* p, size_t len, void (*destroy)(void*)) { namespace grpc_core { /* grpc_slice_new_with_len support structures - we create a refcount object extended with the user provided data pointer & destroy function */ - class NewWithLenSliceRefcount { public: static void Destroy(void* arg) { @@ -150,25 +150,48 @@ class NewWithLenSliceRefcount { NewWithLenSliceRefcount(void (*destroy)(void*, size_t), void* user_data, size_t user_length) - : rc_(grpc_slice_refcount::Type::REGULAR, &refs_, Destroy, this, &rc_), + : base_(grpc_slice_refcount::Type::REGULAR, &refs_, Destroy, this, + &base_), user_data_(user_data), user_length_(user_length), user_destroy_(destroy) {} GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE - grpc_slice_refcount* base_refcount() { return &rc_; } + grpc_slice_refcount* base_refcount() { return &base_; } private: ~NewWithLenSliceRefcount() { user_destroy_(user_data_, user_length_); } - grpc_slice_refcount rc_; + grpc_slice_refcount base_; RefCount refs_; void* user_data_; size_t user_length_; void (*user_destroy_)(void*, size_t); }; +/** grpc_slice_from_moved_(string|buffer) ref count .*/ +class MovedStringSliceRefCount { + public: + MovedStringSliceRefCount(grpc_core::UniquePtr&& str) + : base_(grpc_slice_refcount::Type::REGULAR, &refs_, Destroy, this, + &base_), + str_(std::move(str)) {} + + GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE + + grpc_slice_refcount* base_refcount() { return &base_; } + + private: + static void Destroy(void* arg) { + Delete(static_cast(arg)); + } + + grpc_slice_refcount base_; + grpc_core::RefCount refs_; + grpc_core::UniquePtr str_; +}; + } // namespace grpc_core grpc_slice grpc_slice_new_with_len(void* p, size_t len, @@ -193,6 +216,29 @@ 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_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)) { + slice.refcount = nullptr; + slice.data.inlined.length = len; + memcpy(GRPC_SLICE_START_PTR(slice), ptr, len); + } else { + slice.refcount = + grpc_core::New(std::move(p)) + ->base_refcount(); + slice.data.refcounted.bytes = ptr; + slice.data.refcounted.length = len; + } + 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 23a2f6b81bc..c6943fe6563 100644 --- a/src/core/lib/slice/slice_internal.h +++ b/src/core/lib/slice/slice_internal.h @@ -28,6 +28,7 @@ #include #include "src/core/lib/gpr/murmur_hash.h" +#include "src/core/lib/gprpp/memory.h" #include "src/core/lib/gprpp/ref_counted.h" #include "src/core/lib/transport/static_metadata.h" @@ -302,6 +303,10 @@ 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 // itself. This means that inlined and slices from static strings will return // 0. All other slices will return the size of the allocated chars. 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 a92ab823a38..0f65091333d 100644 --- a/src/core/lib/surface/validate_metadata.cc +++ b/src/core/lib/surface/validate_metadata.cc @@ -24,6 +24,7 @@ #include #include +#include "src/core/lib/gprpp/memory.h" #include "src/core/lib/iomgr/error.h" #include "src/core/lib/slice/slice_internal.h" #include "src/core/lib/slice/slice_string_helpers.h" @@ -39,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_copied_string(dump)); - gpr_free(dump); + GRPC_ERROR_STR_RAW_BYTES, + 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 6ed02366fe2..4f824cbd5ad 100644 --- a/test/core/slice/slice_test.cc +++ b/test/core/slice/slice_test.cc @@ -27,6 +27,7 @@ #include #include +#include "src/core/lib/gprpp/memory.h" #include "src/core/lib/slice/slice_internal.h" #include "src/core/lib/transport/static_metadata.h" #include "test/core/util/test_config.h" @@ -285,6 +286,44 @@ static void test_static_slice_copy_interning(void) { grpc_shutdown(); } +static void test_moved_string_slice(void) { + LOG_TEST_NAME("test_moved_string_slice"); + + grpc_init(); + + // Small string should be inlined. + constexpr char kSmallStr[] = "hello12345"; + char* small_ptr = strdup(kSmallStr); + grpc_slice small = + grpc_slice_from_moved_string(grpc_core::UniquePtr(small_ptr)); + GPR_ASSERT(GRPC_SLICE_LENGTH(small) == strlen(kSmallStr)); + GPR_ASSERT(GRPC_SLICE_START_PTR(small) != + reinterpret_cast(small_ptr)); + grpc_slice_unref(small); + + // Large string should be move the reference. + constexpr char kSLargeStr[] = "hello123456789123456789123456789"; + char* large_ptr = strdup(kSLargeStr); + grpc_slice large = + grpc_slice_from_moved_string(grpc_core::UniquePtr(large_ptr)); + GPR_ASSERT(GRPC_SLICE_LENGTH(large) == strlen(kSLargeStr)); + GPR_ASSERT(GRPC_SLICE_START_PTR(large) == + 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(); +} + int main(int argc, char** argv) { unsigned length; grpc::testing::TestEnvironment env(argc, argv); @@ -302,6 +341,7 @@ int main(int argc, char** argv) { test_slice_interning(); test_static_slice_interning(); test_static_slice_copy_interning(); + test_moved_string_slice(); grpc_shutdown(); return 0; }