From afc0e461e123489808930e5433f0d8b3f95d9256 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Fri, 13 Jan 2023 15:48:09 -0500 Subject: [PATCH] AK+Everywhere: Disallow returning a reference from a fallible expression This will silently make a copy. Rather than masking this behavior, let's explicitly disallow it. --- AK/Try.h | 38 +++++++++++-------- Kernel/Net/Socket.h | 14 ++++--- .../LibLocale/GenerateLocaleData.cpp | 14 ++++--- Userland/Libraries/LibAudio/LoaderError.h | 14 ++++--- Userland/Libraries/LibJS/Runtime/Completion.h | 2 + .../LibJS/Runtime/PromiseCapability.h | 6 +++ Userland/Libraries/LibVideo/DecoderError.h | 20 +++++----- .../Libraries/LibVideo/PlaybackManager.cpp | 2 + .../LibWeb/Fetch/Fetching/Fetching.cpp | 14 ++++--- .../LibWeb/WebDriver/ExecuteScript.cpp | 14 ++++--- Userland/Utilities/matroska.cpp | 2 + 11 files changed, 86 insertions(+), 54 deletions(-) diff --git a/AK/Try.h b/AK/Try.h index 772f0d6608..81b0fca9f3 100644 --- a/AK/Try.h +++ b/AK/Try.h @@ -15,22 +15,30 @@ // on statement expressions [1]. This is known to be implemented // by at least clang and gcc. // [1] https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html +// +// If the static_assert below is triggered, it means you tried to return a reference +// from a fallible expression. This will not do what you want; the statement expression +// will create a copy regardless, so it is explicitly disallowed. -#define TRY(expression) \ - ({ \ - /* Ignore -Wshadow to allow nesting the macro. */ \ - AK_IGNORE_DIAGNOSTIC("-Wshadow", \ - auto _temporary_result = (expression)); \ - if (_temporary_result.is_error()) [[unlikely]] \ - return _temporary_result.release_error(); \ - _temporary_result.release_value(); \ +#define TRY(expression) \ + ({ \ + /* Ignore -Wshadow to allow nesting the macro. */ \ + AK_IGNORE_DIAGNOSTIC("-Wshadow", \ + auto _temporary_result = (expression)); \ + static_assert(!IsLvalueReference, \ + "Do not return a reference from a fallible expression"); \ + if (_temporary_result.is_error()) [[unlikely]] \ + return _temporary_result.release_error(); \ + _temporary_result.release_value(); \ }) -#define MUST(expression) \ - ({ \ - /* Ignore -Wshadow to allow nesting the macro. */ \ - AK_IGNORE_DIAGNOSTIC("-Wshadow", \ - auto _temporary_result = (expression)); \ - VERIFY(!_temporary_result.is_error()); \ - _temporary_result.release_value(); \ +#define MUST(expression) \ + ({ \ + /* Ignore -Wshadow to allow nesting the macro. */ \ + AK_IGNORE_DIAGNOSTIC("-Wshadow", \ + auto _temporary_result = (expression)); \ + static_assert(!IsLvalueReference, \ + "Do not return a reference from a fallible expression"); \ + VERIFY(!_temporary_result.is_error()); \ + _temporary_result.release_value(); \ }) diff --git a/Kernel/Net/Socket.h b/Kernel/Net/Socket.h index 63f92aa270..b5bc494a77 100644 --- a/Kernel/Net/Socket.h +++ b/Kernel/Net/Socket.h @@ -184,12 +184,14 @@ private: }; // This is a special variant of TRY() that also updates the socket's SO_ERROR field on error. -#define SOCKET_TRY(expression) \ - ({ \ - auto result = (expression); \ - if (result.is_error()) \ - return set_so_error(result.release_error()); \ - result.release_value(); \ +#define SOCKET_TRY(expression) \ + ({ \ + auto result = (expression); \ + if (result.is_error()) \ + return set_so_error(result.release_error()); \ + static_assert(!IsLvalueReference, \ + "Do not return a reference from a fallible expression"); \ + result.release_value(); \ }) } diff --git a/Meta/Lagom/Tools/CodeGenerators/LibLocale/GenerateLocaleData.cpp b/Meta/Lagom/Tools/CodeGenerators/LibLocale/GenerateLocaleData.cpp index b2f6e78cae..11059aa9ae 100644 --- a/Meta/Lagom/Tools/CodeGenerators/LibLocale/GenerateLocaleData.cpp +++ b/Meta/Lagom/Tools/CodeGenerators/LibLocale/GenerateLocaleData.cpp @@ -231,12 +231,14 @@ struct CLDR { // Some parsing is expected to fail. For example, the CLDR contains language mappings // with locales such as "en-GB-oed" that are canonically invalid locale IDs. -#define TRY_OR_DISCARD(expression) \ - ({ \ - auto _temporary_result = (expression); \ - if (_temporary_result.is_error()) \ - return; \ - _temporary_result.release_value(); \ +#define TRY_OR_DISCARD(expression) \ + ({ \ + auto _temporary_result = (expression); \ + if (_temporary_result.is_error()) \ + return; \ + static_assert(!IsLvalueReference, \ + "Do not return a reference from a fallible expression"); \ + _temporary_result.release_value(); \ }) static ErrorOr parse_language_mapping(CLDR& cldr, StringView key, StringView alias) diff --git a/Userland/Libraries/LibAudio/LoaderError.h b/Userland/Libraries/LibAudio/LoaderError.h index 2464d5cb64..e659050f38 100644 --- a/Userland/Libraries/LibAudio/LoaderError.h +++ b/Userland/Libraries/LibAudio/LoaderError.h @@ -66,10 +66,12 @@ struct LoaderError { } // Convenience TRY-like macro to convert an Error to a LoaderError -#define LOADER_TRY(expression) \ - ({ \ - auto _temporary_result = (expression); \ - if (_temporary_result.is_error()) \ - return LoaderError(_temporary_result.release_error()); \ - _temporary_result.release_value(); \ +#define LOADER_TRY(expression) \ + ({ \ + auto _temporary_result = (expression); \ + if (_temporary_result.is_error()) \ + return LoaderError(_temporary_result.release_error()); \ + static_assert(!IsLvalueReference, \ + "Do not return a reference from a fallible expression"); \ + _temporary_result.release_value(); \ }) diff --git a/Userland/Libraries/LibJS/Runtime/Completion.h b/Userland/Libraries/LibJS/Runtime/Completion.h index 0e36f3b8a0..fed974f7b6 100644 --- a/Userland/Libraries/LibJS/Runtime/Completion.h +++ b/Userland/Libraries/LibJS/Runtime/Completion.h @@ -25,6 +25,8 @@ namespace JS { VERIFY(_temporary_result.error().code() == ENOMEM); \ return vm.throw_completion(JS::ErrorType::OutOfMemory); \ } \ + static_assert(!IsLvalueReference, \ + "Do not return a reference from a fallible expression"); \ _temporary_result.release_value(); \ }) diff --git a/Userland/Libraries/LibJS/Runtime/PromiseCapability.h b/Userland/Libraries/LibJS/Runtime/PromiseCapability.h index c2cee43deb..fac53cadb3 100644 --- a/Userland/Libraries/LibJS/Runtime/PromiseCapability.h +++ b/Userland/Libraries/LibJS/Runtime/PromiseCapability.h @@ -53,6 +53,9 @@ private: return (capability)->promise(); \ } \ \ + static_assert(!IsLvalueReference, \ + "Do not return a reference from a fallible expression"); \ + \ /* 2. Else if value is a Completion Record, set value to value.[[Value]]. */ \ _temporary_try_or_reject_result.release_value(); \ }) @@ -76,6 +79,9 @@ private: return Value { (capability)->promise() }; \ } \ \ + static_assert(!IsLvalueReference, \ + "Do not return a reference from a fallible expression"); \ + \ /* 2. Else if value is a Completion Record, set value to value.[[Value]]. */ \ _temporary_try_or_reject_result.release_value(); \ }) diff --git a/Userland/Libraries/LibVideo/DecoderError.h b/Userland/Libraries/LibVideo/DecoderError.h index 402b9c8c22..94a7b73993 100644 --- a/Userland/Libraries/LibVideo/DecoderError.h +++ b/Userland/Libraries/LibVideo/DecoderError.h @@ -77,15 +77,17 @@ private: DeprecatedString m_description; }; -#define DECODER_TRY(category, expression) \ - ({ \ - auto _result = ((expression)); \ - if (_result.is_error()) [[unlikely]] { \ - auto _error_string = _result.release_error().string_literal(); \ - return DecoderError::from_source_location( \ - ((category)), _error_string, SourceLocation::current()); \ - } \ - _result.release_value(); \ +#define DECODER_TRY(category, expression) \ + ({ \ + auto _result = ((expression)); \ + if (_result.is_error()) [[unlikely]] { \ + auto _error_string = _result.release_error().string_literal(); \ + return DecoderError::from_source_location( \ + ((category)), _error_string, SourceLocation::current()); \ + } \ + static_assert(!IsLvalueReference, \ + "Do not return a reference from a fallible expression"); \ + _result.release_value(); \ }) #define DECODER_TRY_ALLOC(expression) DECODER_TRY(DecoderErrorCategory::Memory, expression) diff --git a/Userland/Libraries/LibVideo/PlaybackManager.cpp b/Userland/Libraries/LibVideo/PlaybackManager.cpp index 77d881a8fb..acb14feefb 100644 --- a/Userland/Libraries/LibVideo/PlaybackManager.cpp +++ b/Userland/Libraries/LibVideo/PlaybackManager.cpp @@ -273,6 +273,8 @@ bool PlaybackManager::decode_and_queue_one_sample() m_present_timer->start(0); \ return false; \ } \ + static_assert(!IsLvalueReference, \ + "Do not return a reference from a fallible expression"); \ _temporary_result.release_value(); \ }) diff --git a/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp b/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp index d0117b3c2c..b3a8c92401 100644 --- a/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp +++ b/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp @@ -42,12 +42,14 @@ namespace Web::Fetch::Fetching { -#define TRY_OR_IGNORE(expression) \ - ({ \ - auto _temporary_result = (expression); \ - if (_temporary_result.is_error()) \ - return; \ - _temporary_result.release_value(); \ +#define TRY_OR_IGNORE(expression) \ + ({ \ + auto _temporary_result = (expression); \ + if (_temporary_result.is_error()) \ + return; \ + static_assert(!IsLvalueReference, \ + "Do not return a reference from a fallible expression"); \ + _temporary_result.release_value(); \ }) // https://fetch.spec.whatwg.org/#concept-fetch diff --git a/Userland/Libraries/LibWeb/WebDriver/ExecuteScript.cpp b/Userland/Libraries/LibWeb/WebDriver/ExecuteScript.cpp index 1fa2b62263..2c414b47aa 100644 --- a/Userland/Libraries/LibWeb/WebDriver/ExecuteScript.cpp +++ b/Userland/Libraries/LibWeb/WebDriver/ExecuteScript.cpp @@ -31,12 +31,14 @@ namespace Web::WebDriver { -#define TRY_OR_JS_ERROR(expression) \ - ({ \ - auto _temporary_result = (expression); \ - if (_temporary_result.is_error()) [[unlikely]] \ - return ExecuteScriptResultType::JavaScriptError; \ - _temporary_result.release_value(); \ +#define TRY_OR_JS_ERROR(expression) \ + ({ \ + auto _temporary_result = (expression); \ + if (_temporary_result.is_error()) [[unlikely]] \ + return ExecuteScriptResultType::JavaScriptError; \ + static_assert(!IsLvalueReference, \ + "Do not return a reference from a fallible expression"); \ + _temporary_result.release_value(); \ }) static ErrorOr internal_json_clone_algorithm(JS::Realm&, JS::Value, HashTable& seen); diff --git a/Userland/Utilities/matroska.cpp b/Userland/Utilities/matroska.cpp index 716250bbea..ac551a4d86 100644 --- a/Userland/Utilities/matroska.cpp +++ b/Userland/Utilities/matroska.cpp @@ -17,6 +17,8 @@ outln("Encountered a parsing error: {}", _temporary_result.error().string_literal()); \ return Error::from_string_literal("Failed to parse :("); \ } \ + static_assert(!IsLvalueReference, \ + "Do not return a reference from a fallible expression"); \ _temporary_result.release_value(); \ })