From 7c6b5ed16116355d608515de6ffff489650b89d8 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Wed, 4 Jan 2023 07:08:29 -0500 Subject: [PATCH] LibIPC+LibC: Add and use a helper to encode/decoder container sizes While refactoring the IPC encoders and decoders for fallibility, the inconsistency in which we transfer container sizes was a frequent thing to trip over. We currently transfer sizes as any of i32, u32, and u64. This adds a helper to transfer sizes in one consistent way. Two special cases here are DeprecatedString and Vector, whose encoding is depended upon by netdb, so that is also updated here. --- Userland/Libraries/LibC/netdb.cpp | 17 ++++++++--------- Userland/Libraries/LibIPC/Decoder.cpp | 25 ++++++++++++++----------- Userland/Libraries/LibIPC/Decoder.h | 15 +++++++-------- Userland/Libraries/LibIPC/Encoder.cpp | 19 ++++++++++++++----- Userland/Libraries/LibIPC/Encoder.h | 7 +++++-- 5 files changed, 48 insertions(+), 35 deletions(-) diff --git a/Userland/Libraries/LibC/netdb.cpp b/Userland/Libraries/LibC/netdb.cpp index 20ba73f0ae..e87ed4122d 100644 --- a/Userland/Libraries/LibC/netdb.cpp +++ b/Userland/Libraries/LibC/netdb.cpp @@ -122,20 +122,19 @@ hostent* gethostbyname(char const* name) close(fd); }); - size_t unsigned_name_length = strlen(name); - VERIFY(unsigned_name_length <= NumericLimits::max()); - i32 name_length = static_cast(unsigned_name_length); + auto name_length = strlen(name); + VERIFY(name_length <= NumericLimits::max()); struct [[gnu::packed]] { u32 message_size; u32 endpoint_magic; i32 message_id; - i32 name_length; + u32 name_length; } request_header = { (u32)(sizeof(request_header) - sizeof(request_header.message_size) + name_length), lookup_server_endpoint_magic, 1, - name_length, + static_cast(name_length), }; if (auto nsent = write(fd, &request_header, sizeof(request_header)); nsent < 0) { h_errno = TRY_AGAIN; @@ -148,7 +147,7 @@ hostent* gethostbyname(char const* name) if (auto nsent = write(fd, name, name_length); nsent < 0) { h_errno = TRY_AGAIN; return nullptr; - } else if (nsent != name_length) { + } else if (static_cast(nsent) != name_length) { h_errno = NO_RECOVERY; return nullptr; } @@ -158,7 +157,7 @@ hostent* gethostbyname(char const* name) u32 endpoint_magic; i32 message_id; i32 code; - u64 addresses_count; + u32 addresses_count; } response_header; if (auto nreceived = read(fd, &response_header, sizeof(response_header)); nreceived < 0) { @@ -270,7 +269,7 @@ hostent* gethostbyaddr(void const* addr, socklen_t addr_size, int type) u32 endpoint_magic; i32 message_id; i32 code; - i32 name_length; + u32 name_length; } response_header; if (auto nreceived = read(fd, &response_header, sizeof(response_header)); nreceived < 0) { @@ -293,7 +292,7 @@ hostent* gethostbyaddr(void const* addr, socklen_t addr_size, int type) if (auto nreceived = read(fd, buffer, response_header.name_length); nreceived < 0) { h_errno = TRY_AGAIN; return nullptr; - } else if (nreceived != response_header.name_length) { + } else if (static_cast(nreceived) != response_header.name_length) { h_errno = NO_RECOVERY; return nullptr; } diff --git a/Userland/Libraries/LibIPC/Decoder.cpp b/Userland/Libraries/LibIPC/Decoder.cpp index 50decffea4..bfe6a1493c 100644 --- a/Userland/Libraries/LibIPC/Decoder.cpp +++ b/Userland/Libraries/LibIPC/Decoder.cpp @@ -5,6 +5,7 @@ */ #include +#include #include #include #include @@ -16,19 +17,24 @@ namespace IPC { +ErrorOr Decoder::decode_size() +{ + return static_cast(TRY(decode())); +} + template<> ErrorOr decode(Decoder& decoder) { - auto length = TRY(decoder.decode()); - if (length < 0) + auto length = TRY(decoder.decode_size()); + if (length == NumericLimits::max()) return DeprecatedString {}; if (length == 0) return DeprecatedString::empty(); char* text_buffer = nullptr; - auto text_impl = StringImpl::create_uninitialized(static_cast(length), text_buffer); + auto text_impl = StringImpl::create_uninitialized(length, text_buffer); - Bytes bytes { text_buffer, static_cast(length) }; + Bytes bytes { text_buffer, length }; TRY(decoder.decode_into(bytes)); return DeprecatedString { *text_impl }; @@ -37,8 +43,8 @@ ErrorOr decode(Decoder& decoder) template<> ErrorOr decode(Decoder& decoder) { - auto length = TRY(decoder.decode()); - if (length <= 0) + auto length = TRY(decoder.decode_size()); + if (length == 0) return ByteBuffer {}; auto buffer = TRY(ByteBuffer::create_uninitialized(length)); @@ -65,10 +71,7 @@ ErrorOr decode(Decoder& decoder) template<> ErrorOr decode(Decoder& decoder) { - auto size = TRY(decoder.decode()); - if (size >= NumericLimits::max()) - VERIFY_NOT_REACHED(); - + auto size = TRY(decoder.decode_size()); Dictionary dictionary {}; for (size_t i = 0; i < size; ++i) { @@ -99,7 +102,7 @@ ErrorOr decode(Decoder& decoder) if (auto valid = TRY(decoder.decode()); !valid) return Core::AnonymousBuffer {}; - auto size = TRY(decoder.decode()); + auto size = TRY(decoder.decode_size()); auto anon_file = TRY(decoder.decode()); return Core::AnonymousBuffer::create_from_anon_fd(anon_file.take_fd(), size); diff --git a/Userland/Libraries/LibIPC/Decoder.h b/Userland/Libraries/LibIPC/Decoder.h index ad636f1b8c..1dec4958ac 100644 --- a/Userland/Libraries/LibIPC/Decoder.h +++ b/Userland/Libraries/LibIPC/Decoder.h @@ -50,6 +50,8 @@ public: return {}; } + ErrorOr decode_size(); + Core::Stream::LocalSocket& socket() { return m_socket; } private: @@ -96,11 +98,9 @@ ErrorOr decode(Decoder&); template ErrorOr decode(Decoder& decoder) { - auto size = TRY(decoder.decode()); - if (size > NumericLimits::max()) - return Error::from_string_literal("IPC: Invalid Vector size"); - T vector; + + auto size = TRY(decoder.decode_size()); TRY(vector.try_ensure_capacity(size)); for (size_t i = 0; i < size; ++i) { @@ -114,12 +114,11 @@ ErrorOr decode(Decoder& decoder) template ErrorOr decode(Decoder& decoder) { - auto size = TRY(decoder.decode()); - if (size > NumericLimits::max()) - return Error::from_string_literal("IPC: Invalid HashMap size"); - T hashmap; + auto size = TRY(decoder.decode_size()); + TRY(hashmap.try_ensure_capacity(size)); + for (size_t i = 0; i < size; ++i) { auto key = TRY(decoder.decode()); auto value = TRY(decoder.decode()); diff --git a/Userland/Libraries/LibIPC/Encoder.cpp b/Userland/Libraries/LibIPC/Encoder.cpp index 25b8921eff..f0259caaca 100644 --- a/Userland/Libraries/LibIPC/Encoder.cpp +++ b/Userland/Libraries/LibIPC/Encoder.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -21,6 +22,13 @@ namespace IPC { +ErrorOr Encoder::encode_size(size_t size) +{ + if (static_cast(size) > static_cast(NumericLimits::max())) + return Error::from_string_literal("Container exceeds the maximum allowed size"); + return encode(static_cast(size)); +} + template<> ErrorOr encode(Encoder& encoder, float const& value) { @@ -43,10 +51,11 @@ ErrorOr encode(Encoder& encoder, StringView const& value) template<> ErrorOr encode(Encoder& encoder, DeprecatedString const& value) { + // NOTE: Do not change this encoding without also updating LibC/netdb.cpp. if (value.is_null()) - return encoder.encode(-1); + return encoder.encode(NumericLimits::max()); - TRY(encoder.encode(static_cast(value.length()))); + TRY(encoder.encode_size(value.length())); TRY(encoder.encode(value.view())); return {}; } @@ -54,7 +63,7 @@ ErrorOr encode(Encoder& encoder, DeprecatedString const& value) template<> ErrorOr encode(Encoder& encoder, ByteBuffer const& value) { - TRY(encoder.encode(static_cast(value.size()))); + TRY(encoder.encode_size(value.size())); TRY(encoder.append(value.data(), value.size())); return {}; } @@ -74,7 +83,7 @@ ErrorOr encode(Encoder& encoder, URL const& value) template<> ErrorOr encode(Encoder& encoder, Dictionary const& dictionary) { - TRY(encoder.encode(static_cast(dictionary.size()))); + TRY(encoder.encode_size(dictionary.size())); TRY(dictionary.try_for_each_entry([&](auto const& key, auto const& value) -> ErrorOr { TRY(encoder.encode(key)); @@ -109,7 +118,7 @@ ErrorOr encode(Encoder& encoder, Core::AnonymousBuffer const& buffer) TRY(encoder.encode(buffer.is_valid())); if (buffer.is_valid()) { - TRY(encoder.encode(static_cast(buffer.size()))); + TRY(encoder.encode_size(buffer.size())); TRY(encoder.encode(IPC::File { buffer.fd() })); } diff --git a/Userland/Libraries/LibIPC/Encoder.h b/Userland/Libraries/LibIPC/Encoder.h index 334c2b14c8..c96646e0d7 100644 --- a/Userland/Libraries/LibIPC/Encoder.h +++ b/Userland/Libraries/LibIPC/Encoder.h @@ -57,6 +57,8 @@ public: return m_buffer.fds.try_append(move(auto_fd)); } + ErrorOr encode_size(size_t size); + private: void encode_u32(u32); void encode_u64(u64); @@ -134,7 +136,8 @@ ErrorOr encode(Encoder&, Empty const&); template ErrorOr encode(Encoder& encoder, T const& vector) { - TRY(encoder.encode(static_cast(vector.size()))); + // NOTE: Do not change this encoding without also updating LibC/netdb.cpp. + TRY(encoder.encode_size(vector.size())); for (auto const& value : vector) TRY(encoder.encode(value)); @@ -145,7 +148,7 @@ ErrorOr encode(Encoder& encoder, T const& vector) template ErrorOr encode(Encoder& encoder, T const& hashmap) { - TRY(encoder.encode(static_cast(hashmap.size()))); + TRY(encoder.encode_size(hashmap.size())); for (auto it : hashmap) { TRY(encoder.encode(it.key));