From 3c05261611ff210e26d14aabc75882b5dee23a61 Mon Sep 17 00:00:00 2001 From: creator1creeper1 Date: Sat, 8 Jan 2022 20:06:03 +0100 Subject: [PATCH] AK+Everywhere: Make FixedArray OOM-safe FixedArray now doesn't expose any infallible constructors anymore. Rather, it exposes fallible methods. Therefore, it can be used for OOM-safe code. This commit also converts the rest of the system to use the new API. However, as an example, VMObject can't take advantage of this yet, as we would have to endow VMObject with a fallible static construction method, which would require a very fundamental change to VMObject's whole inheritance hierarchy. --- AK/FixedArray.h | 86 ++++++++++++++-------- Kernel/Memory/VMObject.cpp | 4 +- Tests/AK/TestFixedArray.cpp | 2 +- Userland/Libraries/LibAudio/Buffer.h | 2 +- Userland/Libraries/LibAudio/FlacLoader.cpp | 2 +- 5 files changed, 62 insertions(+), 34 deletions(-) diff --git a/AK/FixedArray.h b/AK/FixedArray.h index d2a631fdd0..a3485d85b3 100644 --- a/AK/FixedArray.h +++ b/AK/FixedArray.h @@ -1,11 +1,13 @@ /* - * Copyright (c) 2018-2021, Andreas Kling + * Copyright (c) 2018-2022, Andreas Kling + * Copyright (c) 2022, the SerenityOS developers. * * SPDX-License-Identifier: BSD-2-Clause */ #pragma once +#include #include #include #include @@ -16,40 +18,60 @@ template class FixedArray { public: FixedArray() = default; - explicit FixedArray(size_t size) - : m_size(size) + + static ErrorOr> try_create(size_t size) { - if (m_size != 0) { - m_elements = static_cast(kmalloc_array(m_size, sizeof(T))); - for (size_t i = 0; i < m_size; ++i) - new (&m_elements[i]) T(); - } + if (size == 0) + return FixedArray(); + T* elements = static_cast(kmalloc_array(size, sizeof(T))); + if (!elements) + return Error::from_errno(ENOMEM); + for (size_t i = 0; i < size; ++i) + new (&elements[i]) T(); + return FixedArray(size, elements); } + + static FixedArray must_create_but_fixme_should_propagate_errors(size_t size) + { + return MUST(try_create(size)); + } + + ErrorOr> try_clone() const + { + if (m_size == 0) + return FixedArray(); + T* elements = static_cast(kmalloc_array(m_size, sizeof(T))); + if (!elements) + return Error::from_errno(ENOMEM); + for (size_t i = 0; i < m_size; ++i) + new (&elements[i]) T(m_elements[i]); + return FixedArray(m_size, elements); + } + + FixedArray must_clone_but_fixme_should_propagate_errors() const + { + return MUST(try_clone()); + } + + // NOTE: Nobody can ever use these functions, since it would be impossible to make them OOM-safe due to their signatures. We just explicitly delete them. + FixedArray(FixedArray const&) = delete; + FixedArray& operator=(FixedArray const&) = delete; + + FixedArray(FixedArray&& other) + : m_size(other.m_size) + , m_elements(other.m_elements) + { + other.m_size = 0; + other.m_elements = nullptr; + } + // NOTE: Nobody uses this function, so we just explicitly delete it. + FixedArray& operator=(FixedArray&&) = delete; + ~FixedArray() { clear(); } - FixedArray(FixedArray const& other) - : m_size(other.m_size) - { - if (m_size != 0) { - m_elements = static_cast(kmalloc_array(m_size, sizeof(T))); - for (size_t i = 0; i < m_size; ++i) - new (&m_elements[i]) T(other[i]); - } - } - - FixedArray& operator=(FixedArray const& other) - { - FixedArray array(other); - swap(array); - return *this; - } - - FixedArray(FixedArray&&) = delete; - FixedArray& operator=(FixedArray&&) = delete; - void clear() { if (!m_elements) @@ -86,7 +108,7 @@ public: return false; } - void swap(FixedArray& other) + void swap(FixedArray& other) { ::swap(m_elements, other.m_elements); ::swap(m_size, other.m_size); @@ -105,6 +127,12 @@ public: Span span() { return { data(), size() }; } private: + FixedArray(size_t size, T* elements) + : m_size(size) + , m_elements(elements) + { + } + size_t m_size { 0 }; T* m_elements { nullptr }; }; diff --git a/Kernel/Memory/VMObject.cpp b/Kernel/Memory/VMObject.cpp index 7ccb723180..78db6f2d1b 100644 --- a/Kernel/Memory/VMObject.cpp +++ b/Kernel/Memory/VMObject.cpp @@ -18,13 +18,13 @@ SpinlockProtected& VMObject::all_instances() } VMObject::VMObject(VMObject const& other) - : m_physical_pages(other.m_physical_pages) + : m_physical_pages(other.m_physical_pages.must_clone_but_fixme_should_propagate_errors()) { all_instances().with([&](auto& list) { list.append(*this); }); } VMObject::VMObject(size_t size) - : m_physical_pages(ceil_div(size, static_cast(PAGE_SIZE))) + : m_physical_pages(FixedArray>::must_create_but_fixme_should_propagate_errors(ceil_div(size, static_cast(PAGE_SIZE)))) { all_instances().with([&](auto& list) { list.append(*this); }); } diff --git a/Tests/AK/TestFixedArray.cpp b/Tests/AK/TestFixedArray.cpp index 6b33818c62..1558619f79 100644 --- a/Tests/AK/TestFixedArray.cpp +++ b/Tests/AK/TestFixedArray.cpp @@ -16,7 +16,7 @@ TEST_CASE(construct) TEST_CASE(ints) { - FixedArray ints(3); + FixedArray ints = FixedArray::must_create_but_fixme_should_propagate_errors(3); ints[0] = 0; ints[1] = 1; ints[2] = 2; diff --git a/Userland/Libraries/LibAudio/Buffer.h b/Userland/Libraries/LibAudio/Buffer.h index 9d88e22496..1863fcfe06 100644 --- a/Userland/Libraries/LibAudio/Buffer.h +++ b/Userland/Libraries/LibAudio/Buffer.h @@ -89,7 +89,7 @@ public: static NonnullRefPtr create_empty() { // If we can't allocate an empty buffer, things are in a very bad state. - return MUST(adopt_nonnull_ref_or_enomem(new (nothrow) Buffer(FixedArray {}))); + return MUST(adopt_nonnull_ref_or_enomem(new (nothrow) Buffer(FixedArray()))); } Sample const* samples() const { return (const Sample*)data(); } diff --git a/Userland/Libraries/LibAudio/FlacLoader.cpp b/Userland/Libraries/LibAudio/FlacLoader.cpp index 03a0d83b94..463dd4dd0c 100644 --- a/Userland/Libraries/LibAudio/FlacLoader.cpp +++ b/Userland/Libraries/LibAudio/FlacLoader.cpp @@ -197,7 +197,7 @@ LoaderSamples FlacLoaderPlugin::get_more_samples(size_t max_bytes_to_read_from_i return Buffer::create_empty(); size_t samples_to_read = min(max_bytes_to_read_from_input, remaining_samples); - auto samples = FixedArray(samples_to_read); + auto samples = FixedArray::must_create_but_fixme_should_propagate_errors(samples_to_read); size_t sample_index = 0; if (m_unread_data.size() > 0) {