From d7b6cc6421c48185f4dfafb24d2b093b86305860 Mon Sep 17 00:00:00 2001 From: Daniel Bertalan Date: Fri, 3 Sep 2021 19:11:51 +0200 Subject: [PATCH] Everywhere: Prevent risky implicit casts of (Nonnull)RefPtr Our existing implementation did not check the element type of the other pointer in the constructors and move assignment operators. This meant that some operations that would require explicit casting on raw pointers were done implicitly, such as: - downcasting a base class to a derived class (e.g. `Kernel::Inode` => `Kernel::ProcFSDirectoryInode` in Kernel/ProcFS.cpp), - casting to an unrelated type (e.g. `Promise` => `Promise` in LibIMAP/Client.cpp) This, of course, allows gross violations of the type system, and makes the need to type-check less obvious before downcasting. Luckily, while adding the `static_ptr_cast`s, only two truly incorrect usages were found; in the other instances, our casts just needed to be made explicit. --- AK/NonnullRefPtr.h | 14 ++++++------- AK/RefPtr.h | 20 +++++++++---------- Kernel/FileSystem/ProcFS.cpp | 2 +- Kernel/FileSystem/SysFSComponent.cpp | 4 ++-- Kernel/FileSystem/SysFSComponent.h | 4 ++-- Kernel/Graphics/GraphicsManagement.cpp | 2 +- Kernel/Syscalls/purge.cpp | 2 +- Tests/LibSQL/TestSqlDatabase.cpp | 4 ++-- .../Spreadsheet/SpreadsheetWidget.cpp | 2 +- Userland/Libraries/LibGL/Tex/TextureUnit.cpp | 2 +- Userland/Libraries/LibIMAP/Client.cpp | 2 +- Userland/Libraries/LibIMAP/Client.h | 2 +- Userland/Libraries/LibJS/Parser.cpp | 2 +- Userland/Libraries/LibJS/Parser.h | 2 +- Userland/Libraries/LibPDF/Document.cpp | 2 +- Userland/Libraries/LibPDF/Object.h | 2 +- Userland/Libraries/LibSQL/AST/Parser.cpp | 2 +- .../LibWeb/HTML/HTMLTableElement.cpp | 10 +++++----- .../Libraries/LibWeb/Layout/TreeBuilder.cpp | 2 +- Userland/Shell/Parser.cpp | 2 +- Userland/Shell/Parser.h | 2 +- 21 files changed, 43 insertions(+), 43 deletions(-) diff --git a/AK/NonnullRefPtr.h b/AK/NonnullRefPtr.h index 192149373cd..094d8bba440 100644 --- a/AK/NonnullRefPtr.h +++ b/AK/NonnullRefPtr.h @@ -58,7 +58,7 @@ public: const_cast(object).ref(); } template - ALWAYS_INLINE NonnullRefPtr(const U& object) + ALWAYS_INLINE NonnullRefPtr(const U& object) requires(IsConvertible) : m_bits((FlatPtr) static_cast(&object)) { VERIFY(!(m_bits & 1)); @@ -75,7 +75,7 @@ public: VERIFY(!(m_bits & 1)); } template - ALWAYS_INLINE NonnullRefPtr(NonnullRefPtr&& other) + ALWAYS_INLINE NonnullRefPtr(NonnullRefPtr&& other) requires(IsConvertible) : m_bits((FlatPtr)&other.leak_ref()) { VERIFY(!(m_bits & 1)); @@ -86,7 +86,7 @@ public: VERIFY(!(m_bits & 1)); } template - ALWAYS_INLINE NonnullRefPtr(const NonnullRefPtr& other) + ALWAYS_INLINE NonnullRefPtr(const NonnullRefPtr& other) requires(IsConvertible) : m_bits((FlatPtr)other.add_ref()) { VERIFY(!(m_bits & 1)); @@ -119,7 +119,7 @@ public: } template - NonnullRefPtr& operator=(const NonnullRefPtr& other) + NonnullRefPtr& operator=(const NonnullRefPtr& other) requires(IsConvertible) { assign(other.add_ref()); return *this; @@ -133,7 +133,7 @@ public: } template - NonnullRefPtr& operator=(NonnullRefPtr&& other) + NonnullRefPtr& operator=(NonnullRefPtr&& other) requires(IsConvertible) { assign(&other.leak_ref()); return *this; @@ -213,7 +213,7 @@ public: } template - void swap(NonnullRefPtr& other) + void swap(NonnullRefPtr& other) requires(IsConvertible) { // NOTE: swap is not atomic! U* other_ptr = other.exchange(nullptr); @@ -327,7 +327,7 @@ struct Formatter> : Formatter { }; template -inline void swap(NonnullRefPtr& a, NonnullRefPtr& b) +inline void swap(NonnullRefPtr& a, NonnullRefPtr& b) requires(IsConvertible) { a.swap(b); } diff --git a/AK/RefPtr.h b/AK/RefPtr.h index 5031b82fe4e..e054cc99371 100644 --- a/AK/RefPtr.h +++ b/AK/RefPtr.h @@ -154,18 +154,18 @@ public: { } template - ALWAYS_INLINE RefPtr(const NonnullRefPtr& other) + ALWAYS_INLINE RefPtr(const NonnullRefPtr& other) requires(IsConvertible) : m_bits(PtrTraits::as_bits(const_cast(other.add_ref()))) { } template - ALWAYS_INLINE RefPtr(NonnullRefPtr&& other) + ALWAYS_INLINE RefPtr(NonnullRefPtr&& other) requires(IsConvertible) : m_bits(PtrTraits::as_bits(&other.leak_ref())) { VERIFY(!is_null()); } template> - RefPtr(RefPtr&& other) + RefPtr(RefPtr&& other) requires(IsConvertible) : m_bits(PtrTraits::template convert_from(other.leak_ref_raw())) { } @@ -174,7 +174,7 @@ public: { } template> - RefPtr(const RefPtr& other) + RefPtr(const RefPtr& other) requires(IsConvertible) : m_bits(other.add_ref_raw()) { } @@ -203,7 +203,7 @@ public: } template> - void swap(RefPtr& other) + void swap(RefPtr& other) requires(IsConvertible) { // NOTE: swap is not atomic! FlatPtr other_bits = P::exchange(other.m_bits, P::default_null_value); @@ -219,14 +219,14 @@ public: } template> - ALWAYS_INLINE RefPtr& operator=(RefPtr&& other) + ALWAYS_INLINE RefPtr& operator=(RefPtr&& other) requires(IsConvertible) { assign_raw(PtrTraits::template convert_from(other.leak_ref_raw())); return *this; } template - ALWAYS_INLINE RefPtr& operator=(NonnullRefPtr&& other) + ALWAYS_INLINE RefPtr& operator=(NonnullRefPtr&& other) requires(IsConvertible) { assign_raw(PtrTraits::as_bits(&other.leak_ref())); return *this; @@ -239,7 +239,7 @@ public: } template - ALWAYS_INLINE RefPtr& operator=(const NonnullRefPtr& other) + ALWAYS_INLINE RefPtr& operator=(const NonnullRefPtr& other) requires(IsConvertible) { assign_raw(PtrTraits::as_bits(other.add_ref())); return *this; @@ -253,7 +253,7 @@ public: } template - ALWAYS_INLINE RefPtr& operator=(const RefPtr& other) + ALWAYS_INLINE RefPtr& operator=(const RefPtr& other) requires(IsConvertible) { assign_raw(other.add_ref_raw()); return *this; @@ -470,7 +470,7 @@ inline RefPtr static_ptr_cast(const RefPtr& ptr) } template -inline void swap(RefPtr& a, RefPtr& b) +inline void swap(RefPtr& a, RefPtr& b) requires(IsConvertible) { a.swap(b); } diff --git a/Kernel/FileSystem/ProcFS.cpp b/Kernel/FileSystem/ProcFS.cpp index fb5e51d068b..176bfb04f9f 100644 --- a/Kernel/FileSystem/ProcFS.cpp +++ b/Kernel/FileSystem/ProcFS.cpp @@ -55,7 +55,7 @@ KResult ProcFS::initialize() auto root_inode = ProcFSComponentRegistry::the().root_directory().to_inode(*this); if (root_inode.is_error()) return root_inode.error(); - m_root_inode = static_cast>(root_inode.release_value()); + m_root_inode = static_ptr_cast(root_inode.release_value()); return KSuccess; } diff --git a/Kernel/FileSystem/SysFSComponent.cpp b/Kernel/FileSystem/SysFSComponent.cpp index 78780221111..1b3697e4992 100644 --- a/Kernel/FileSystem/SysFSComponent.cpp +++ b/Kernel/FileSystem/SysFSComponent.cpp @@ -61,12 +61,12 @@ SysFSDirectory::SysFSDirectory(StringView name, SysFSDirectory const& parent_dir { } -NonnullRefPtr SysFSDirectory::to_inode(SysFS const& sysfs_instance) const +NonnullRefPtr SysFSDirectory::to_inode(SysFS const& sysfs_instance) const { return SysFSDirectoryInode::create(sysfs_instance, *this); } -NonnullRefPtr SysFSComponent::to_inode(SysFS const& sysfs_instance) const +NonnullRefPtr SysFSComponent::to_inode(SysFS const& sysfs_instance) const { return SysFSInode::create(sysfs_instance, *this); } diff --git a/Kernel/FileSystem/SysFSComponent.h b/Kernel/FileSystem/SysFSComponent.h index 3e29138a5f3..cefa342650e 100644 --- a/Kernel/FileSystem/SysFSComponent.h +++ b/Kernel/FileSystem/SysFSComponent.h @@ -26,7 +26,7 @@ public: virtual RefPtr lookup(StringView) { VERIFY_NOT_REACHED(); }; virtual KResultOr write_bytes(off_t, size_t, UserOrKernelBuffer const&, FileDescription*) { return EROFS; } - virtual NonnullRefPtr to_inode(SysFS const&) const; + virtual NonnullRefPtr to_inode(SysFS const&) const; InodeIndex component_index() const { return m_component_index; }; @@ -45,7 +45,7 @@ public: virtual KResult traverse_as_directory(unsigned, Function) const override; virtual RefPtr lookup(StringView name) override; - virtual NonnullRefPtr to_inode(SysFS const& sysfs_instance) const override final; + virtual NonnullRefPtr to_inode(SysFS const& sysfs_instance) const override final; protected: explicit SysFSDirectory(StringView name); diff --git a/Kernel/Graphics/GraphicsManagement.cpp b/Kernel/Graphics/GraphicsManagement.cpp index a3607dcbccd..2455cd74bfc 100644 --- a/Kernel/Graphics/GraphicsManagement.cpp +++ b/Kernel/Graphics/GraphicsManagement.cpp @@ -130,7 +130,7 @@ UNMAP_AFTER_INIT bool GraphicsManagement::determine_and_initialize_graphics_devi // Note: If no other VGA adapter is attached as m_vga_adapter, we should attach it then. if (!m_vga_adapter && PCI::is_io_space_enabled(address) && adapter->type() == GraphicsDevice::Type::VGACompatible) { dbgln("Graphics adapter @ {} is operating in VGA mode", address); - m_vga_adapter = adapter; + m_vga_adapter = static_ptr_cast(adapter); } return true; } diff --git a/Kernel/Syscalls/purge.cpp b/Kernel/Syscalls/purge.cpp index b13c099f8e6..72057dbafe8 100644 --- a/Kernel/Syscalls/purge.cpp +++ b/Kernel/Syscalls/purge.cpp @@ -27,7 +27,7 @@ KResultOr Process::sys$purge(int mode) if (vmobject.is_anonymous()) { // In the event that the append fails, only attempt to continue // the purge if we have already appended something successfully. - if (!vmobjects.try_append(vmobject) && vmobjects.is_empty()) { + if (!vmobjects.try_append(static_cast(vmobject)) && vmobjects.is_empty()) { result = ENOMEM; return IterationDecision::Break; } diff --git a/Tests/LibSQL/TestSqlDatabase.cpp b/Tests/LibSQL/TestSqlDatabase.cpp index 9c67267c5e1..e57b7bd8a5f 100644 --- a/Tests/LibSQL/TestSqlDatabase.cpp +++ b/Tests/LibSQL/TestSqlDatabase.cpp @@ -16,7 +16,7 @@ #include NonnullRefPtr setup_schema(SQL::Database&); -NonnullRefPtr setup_table(SQL::Database&); +NonnullRefPtr setup_table(SQL::Database&); void insert_into_table(SQL::Database&, int); void verify_table_contents(SQL::Database&, int); void insert_and_verify(int); @@ -28,7 +28,7 @@ NonnullRefPtr setup_schema(SQL::Database& db) return schema; } -NonnullRefPtr setup_table(SQL::Database& db) +NonnullRefPtr setup_table(SQL::Database& db) { auto schema = setup_schema(db); auto table = SQL::TableDef::construct(schema, "TestTable"); diff --git a/Userland/Applications/Spreadsheet/SpreadsheetWidget.cpp b/Userland/Applications/Spreadsheet/SpreadsheetWidget.cpp index 5d2abc435ec..fcad4a05e3c 100644 --- a/Userland/Applications/Spreadsheet/SpreadsheetWidget.cpp +++ b/Userland/Applications/Spreadsheet/SpreadsheetWidget.cpp @@ -220,7 +220,7 @@ void SpreadsheetWidget::setup_tabs(NonnullRefPtrVector new_sheets) }; m_tab_widget->on_context_menu_request = [&](auto& widget, auto& event) { - m_tab_context_menu_sheet_view = widget; + m_tab_context_menu_sheet_view = static_cast(widget); m_tab_context_menu->popup(event.screen_position()); }; } diff --git a/Userland/Libraries/LibGL/Tex/TextureUnit.cpp b/Userland/Libraries/LibGL/Tex/TextureUnit.cpp index 8b4f4870453..b8a180c447b 100644 --- a/Userland/Libraries/LibGL/Tex/TextureUnit.cpp +++ b/Userland/Libraries/LibGL/Tex/TextureUnit.cpp @@ -13,7 +13,7 @@ void TextureUnit::bind_texture_to_target(GLenum texture_target, const RefPtr(texture); m_currently_bound_texture = texture; m_currently_bound_target = GL_TEXTURE_2D; break; diff --git a/Userland/Libraries/LibIMAP/Client.cpp b/Userland/Libraries/LibIMAP/Client.cpp index 9b63733e148..7b1bef5a8ea 100644 --- a/Userland/Libraries/LibIMAP/Client.cpp +++ b/Userland/Libraries/LibIMAP/Client.cpp @@ -31,7 +31,7 @@ RefPtr> Client::connect() } if (!success) return {}; - m_connect_pending = Promise::construct(); + m_connect_pending = Promise::construct(); return m_connect_pending; } diff --git a/Userland/Libraries/LibIMAP/Client.h b/Userland/Libraries/LibIMAP/Client.h index 3fb9095dbdf..cea34fe37cb 100644 --- a/Userland/Libraries/LibIMAP/Client.h +++ b/Userland/Libraries/LibIMAP/Client.h @@ -69,7 +69,7 @@ private: // Not yet sent Vector m_command_queue {}; - RefPtr> m_connect_pending {}; + RefPtr> m_connect_pending {}; ByteBuffer m_buffer; Parser m_parser; diff --git a/Userland/Libraries/LibJS/Parser.cpp b/Userland/Libraries/LibJS/Parser.cpp index c2cf37f9dc9..7a3437915f9 100644 --- a/Userland/Libraries/LibJS/Parser.cpp +++ b/Userland/Libraries/LibJS/Parser.cpp @@ -1606,7 +1606,7 @@ NonnullRefPtr Parser::parse_identifier() token.value()); } -NonnullRefPtr Parser::parse_call_expression(NonnullRefPtr lhs) +NonnullRefPtr Parser::parse_call_expression(NonnullRefPtr lhs) { auto rule_start = push_start(); if (!m_state.allow_super_constructor_call && is(*lhs)) diff --git a/Userland/Libraries/LibJS/Parser.h b/Userland/Libraries/LibJS/Parser.h index 41b6cef2c86..b594b640408 100644 --- a/Userland/Libraries/LibJS/Parser.h +++ b/Userland/Libraries/LibJS/Parser.h @@ -85,7 +85,7 @@ public: NonnullRefPtr parse_string_literal(const Token& token, bool in_template_literal = false); NonnullRefPtr parse_template_literal(bool is_tagged); NonnullRefPtr parse_secondary_expression(NonnullRefPtr, int min_precedence, Associativity associate = Associativity::Right); - NonnullRefPtr parse_call_expression(NonnullRefPtr); + NonnullRefPtr parse_call_expression(NonnullRefPtr); NonnullRefPtr parse_new_expression(); NonnullRefPtr parse_class_declaration(); NonnullRefPtr parse_class_expression(bool expect_class_name); diff --git a/Userland/Libraries/LibPDF/Document.cpp b/Userland/Libraries/LibPDF/Document.cpp index 473b5f5a207..582a1c90f9b 100644 --- a/Userland/Libraries/LibPDF/Document.cpp +++ b/Userland/Libraries/LibPDF/Document.cpp @@ -146,7 +146,7 @@ Value Document::resolve(Value const& value) auto obj = value.as_object(); if (obj->is_indirect_value()) - return static_cast>(obj)->value(); + return static_ptr_cast(obj)->value(); return obj; } diff --git a/Userland/Libraries/LibPDF/Object.h b/Userland/Libraries/LibPDF/Object.h index 47d0794de67..d7673e8ca5b 100644 --- a/Userland/Libraries/LibPDF/Object.h +++ b/Userland/Libraries/LibPDF/Object.h @@ -248,7 +248,7 @@ template # undef ENUMERATE_TYPES #endif - return static_cast>(obj); + return static_ptr_cast(obj); } } diff --git a/Userland/Libraries/LibSQL/AST/Parser.cpp b/Userland/Libraries/LibSQL/AST/Parser.cpp index 6230a0f91c4..d7c728a9ff0 100644 --- a/Userland/Libraries/LibSQL/AST/Parser.cpp +++ b/Userland/Libraries/LibSQL/AST/Parser.cpp @@ -210,7 +210,7 @@ NonnullRefPtr Parser::parse_insert_statement(RefPtr 0) && (chained_expr->expressions().size() != column_names.size())) { syntax_error("Number of expressions does not match number of columns"); } else { - chained_expressions.append(chained_expression.release_nonnull()); + chained_expressions.append(static_ptr_cast(chained_expression.release_nonnull())); } } else { expected("Chained expression"); diff --git a/Userland/Libraries/LibWeb/HTML/HTMLTableElement.cpp b/Userland/Libraries/LibWeb/HTML/HTMLTableElement.cpp index 93bc19b5bf8..f84057cbec7 100644 --- a/Userland/Libraries/LibWeb/HTML/HTMLTableElement.cpp +++ b/Userland/Libraries/LibWeb/HTML/HTMLTableElement.cpp @@ -72,7 +72,7 @@ NonnullRefPtr HTMLTableElement::create_caption() auto caption = DOM::create_element(document(), TagNames::caption, Namespace::HTML); pre_insert(caption, first_child()); - return caption; + return static_ptr_cast(caption); } void HTMLTableElement::delete_caption() @@ -159,7 +159,7 @@ NonnullRefPtr HTMLTableElement::create_t_head() pre_insert(thead, child_to_append_after); - return thead; + return static_ptr_cast(thead); } void HTMLTableElement::delete_t_head() @@ -209,7 +209,7 @@ NonnullRefPtr HTMLTableElement::create_t_foot() auto tfoot = DOM::create_element(document(), TagNames::tfoot, Namespace::HTML); append_child(tfoot); - return tfoot; + return static_ptr_cast(tfoot); } void HTMLTableElement::delete_t_foot() @@ -248,7 +248,7 @@ NonnullRefPtr HTMLTableElement::create_t_body() pre_insert(tbody, child_to_append_after); - return tbody; + return static_ptr_cast(tbody); } NonnullRefPtr HTMLTableElement::rows() @@ -287,7 +287,7 @@ DOM::ExceptionOr> HTMLTableElement::insert_ro if (index < -1 || index >= (long)rows_length) { return DOM::IndexSizeError::create("Index is negative or greater than the number of rows"); } - auto tr = static_cast>(DOM::create_element(document(), TagNames::tr, Namespace::HTML)); + auto tr = static_ptr_cast(DOM::create_element(document(), TagNames::tr, Namespace::HTML)); if (rows_length == 0 && !has_child_of_type()) { auto tbody = DOM::create_element(document(), TagNames::tbody, Namespace::HTML); tbody->append_child(tr); diff --git a/Userland/Libraries/LibWeb/Layout/TreeBuilder.cpp b/Userland/Libraries/LibWeb/Layout/TreeBuilder.cpp index 74b52e9b41f..955416f9a93 100644 --- a/Userland/Libraries/LibWeb/Layout/TreeBuilder.cpp +++ b/Userland/Libraries/LibWeb/Layout/TreeBuilder.cpp @@ -166,7 +166,7 @@ void TreeBuilder::remove_irrelevant_boxes(NodeWithStyle& root) { // The following boxes are discarded as if they were display:none: - NonnullRefPtrVector to_remove; + NonnullRefPtrVector to_remove; // Children of a table-column. for_each_in_tree_with_display(root, [&](Box& table_column) { diff --git a/Userland/Shell/Parser.cpp b/Userland/Shell/Parser.cpp index 68ff39accf4..260fffbcdd2 100644 --- a/Userland/Shell/Parser.cpp +++ b/Userland/Shell/Parser.cpp @@ -1431,7 +1431,7 @@ RefPtr Parser::parse_variable_ref() return create(move(name)); // Variable Simple } -RefPtr Parser::parse_slice() +RefPtr Parser::parse_slice() { auto rule_start = push_start(); if (!next_is("[")) diff --git a/Userland/Shell/Parser.h b/Userland/Shell/Parser.h index 2c11d23a92c..37aac4097f9 100644 --- a/Userland/Shell/Parser.h +++ b/Userland/Shell/Parser.h @@ -85,7 +85,7 @@ private: RefPtr parse_string_inner(StringEndCondition); RefPtr parse_variable(); RefPtr parse_variable_ref(); - RefPtr parse_slice(); + RefPtr parse_slice(); RefPtr parse_evaluate(); RefPtr parse_history_designator(); RefPtr parse_comment();