Refactor: Expose const_cast by removing ByteBuffer::warp(const void*, size_t)

asynts · awesomekling · commit b3d1a05261b6 · 2020-08-06T10:33:16.000+02:00
This function did a const_cast internally which made the call side look
"safe". This method is removed completely and call sites are replaced
with ByteBuffer::wrap(const_cast&lt;void*&gt;(data), size) which makes the
behaviour obvious.
diff --git a/AK/ByteBuffer.h b/AK/ByteBuffer.h
@@ -42,7 +42,6 @@ class ByteBufferImpl : public RefCounted<ByteBufferImpl> {
     static NonnullRefPtr<ByteBufferImpl> create_zeroed(size_t);
     static NonnullRefPtr<ByteBufferImpl> copy(const void*, size_t);
     static NonnullRefPtr<ByteBufferImpl> wrap(void*, size_t);
-    static NonnullRefPtr<ByteBufferImpl> wrap(const void*, size_t);
     static NonnullRefPtr<ByteBufferImpl> adopt(void*, size_t);
 
     ~ByteBufferImpl() { clear(); }
@@ -135,7 +134,9 @@ class ByteBuffer {
     static ByteBuffer create_uninitialized(size_t size) { return ByteBuffer(ByteBufferImpl::create_uninitialized(size)); }
     static ByteBuffer create_zeroed(size_t size) { return ByteBuffer(ByteBufferImpl::create_zeroed(size)); }
     static ByteBuffer copy(const void* data, size_t size) { return ByteBuffer(ByteBufferImpl::copy(data, size)); }
-    static ByteBuffer wrap(const void* data, size_t size) { return ByteBuffer(ByteBufferImpl::wrap(data, size)); }
+    // The const version of this method was removed because it was misleading, suggesting copy on write
+    // functionality. If you really need the old behaviour, call ByteBuffer::wrap(const_cast<void*>(data), size)
+    // manually. Writing to such a byte buffer invokes undefined behaviour.
     static ByteBuffer wrap(void* data, size_t size) { return ByteBuffer(ByteBufferImpl::wrap(data, size)); }
     static ByteBuffer adopt(void* data, size_t size) { return ByteBuffer(ByteBufferImpl::adopt(data, size)); }
 
@@ -193,7 +194,7 @@ class ByteBuffer {
         // I cannot hand you a slice I don't have
         ASSERT(offset + size <= this->size());
 
-        return wrap(offset_pointer(offset), size);
+        return wrap(const_cast<u8*>(offset_pointer(offset)), size);
     }
 
     ByteBuffer slice(size_t offset, size_t size) const
@@ -302,11 +303,6 @@ inline NonnullRefPtr<ByteBufferImpl> ByteBufferImpl::wrap(void* data, size_t siz
     return ::adopt(*new ByteBufferImpl(data, size, Wrap));
 }
 
-inline NonnullRefPtr<ByteBufferImpl> ByteBufferImpl::wrap(const void* data, size_t size)
-{
-    return ::adopt(*new ByteBufferImpl(const_cast<void*>(data), size, Wrap));
-}
-
 inline NonnullRefPtr<ByteBufferImpl> ByteBufferImpl::adopt(void* data, size_t size)
 {
     return ::adopt(*new ByteBufferImpl(data, size, Adopt));
diff --git a/Kernel/FileSystem/ProcFS.cpp b/Kernel/FileSystem/ProcFS.cpp
@@ -315,7 +315,7 @@ Optional<KBuffer> procfs$pid_vm(InodeIdentifier identifier)
             region_object.add("cow_pages", region.cow_pages());
             region_object.add("name", region.name());
             region_object.add("vmobject", region.vmobject().class_name());
-	    
+
             StringBuilder pagemap_builder;
             for (size_t i = 0; i < region.page_count(); ++i) {
                 auto* page = region.physical_page(i);
@@ -736,8 +736,7 @@ Optional<KBuffer> procfs$cpuinfo(InodeIdentifier)
     KBufferBuilder builder;
     JsonArraySerializer array { builder };
     Processor::for_each(
-        [&](Processor& proc) -> IterationDecision
-        {
+        [&](Processor& proc) -> IterationDecision {
             auto& info = proc.info();
             auto obj = array.add_object();
             JsonArray features;
@@ -1399,7 +1398,7 @@ ssize_t ProcFSInode::write_bytes(off_t offset, ssize_t size, const u8* buffer, F
     ASSERT(is_persistent_inode(identifier()));
     // FIXME: Being able to write into ProcFS at a non-zero offset seems like something we should maybe support..
     ASSERT(offset == 0);
-    bool success = (*write_callback)(identifier(), ByteBuffer::wrap(buffer, size));
+    bool success = (*write_callback)(identifier(), ByteBuffer::wrap(const_cast<u8*>(buffer), size));
     ASSERT(success);
     return 0;
 }
diff --git a/Libraries/LibELF/Image.h b/Libraries/LibELF/Image.h
@@ -66,7 +66,7 @@ class Image {
         {
         }
 
-        ~Symbol() {}
+        ~Symbol() { }
 
         StringView name() const { return m_image.table_string(m_sym.st_name); }
         unsigned section_index() const { return m_sym.st_shndx; }
@@ -92,7 +92,7 @@ class Image {
             , m_program_header_index(program_header_index)
         {
         }
-        ~ProgramHeader() {}
+        ~ProgramHeader() { }
 
         unsigned index() const { return m_program_header_index; }
         u32 type() const { return m_program_header.p_type; }
@@ -122,7 +122,7 @@ class Image {
             , m_section_index(sectionIndex)
         {
         }
-        ~Section() {}
+        ~Section() { }
 
         StringView name() const { return m_image.section_header_table_string(m_section_header.sh_name); }
         unsigned type() const { return m_section_header.sh_type; }
@@ -132,7 +132,7 @@ class Image {
         unsigned entry_count() const { return !entry_size() ? 0 : size() / entry_size(); }
         u32 address() const { return m_section_header.sh_addr; }
         const char* raw_data() const { return m_image.raw_data(m_section_header.sh_offset); }
-        ByteBuffer wrapping_byte_buffer() { return ByteBuffer::wrap(reinterpret_cast<const u8*>(raw_data()), size()); }
+        ByteBuffer wrapping_byte_buffer() { return ByteBuffer::wrap(const_cast<char*>(raw_data()), size()); }
         bool is_undefined() const { return m_section_index == SHN_UNDEF; }
         const RelocationSection relocations() const;
         u32 flags() const { return m_section_header.sh_flags; }
@@ -166,7 +166,7 @@ class Image {
         {
         }
 
-        ~Relocation() {}
+        ~Relocation() { }
 
         unsigned offset() const { return m_rel.r_offset; }
         unsigned type() const { return ELF32_R_TYPE(m_rel.r_info); }
diff --git a/Libraries/LibGemini/GeminiJob.cpp b/Libraries/LibGemini/GeminiJob.cpp
@@ -95,7 +95,9 @@ void GeminiJob::read_while_data_available(Function<IterationDecision()> read)
 
 void GeminiJob::set_certificate(String certificate, String private_key)
 {
-    if (!m_socket->add_client_key(ByteBuffer::wrap(certificate.characters(), certificate.length()), ByteBuffer::wrap(private_key.characters(), private_key.length()))) {
+    if (!m_socket->add_client_key(
+            ByteBuffer::wrap(const_cast<char*>(certificate.characters()), certificate.length()),
+            ByteBuffer::wrap(const_cast<char*>(private_key.characters()), private_key.length()))) {
         dbg() << "LibGemini: Failed to set a client certificate";
         // FIXME: Do something about this failure
         ASSERT_NOT_REACHED();
diff --git a/Libraries/LibGfx/BMPLoader.cpp b/Libraries/LibGfx/BMPLoader.cpp
@@ -1104,7 +1104,7 @@ static bool decode_bmp_pixel_data(BMPLoadingContext& context)
         return false;
     }
 
-    auto buffer = ByteBuffer::wrap(context.data + context.data_offset, context.data_size);
+    auto buffer = ByteBuffer::wrap(const_cast<u8*>(context.data + context.data_offset), context.data_size);
 
     if (context.dib.info.compression == Compression::RLE4 || context.dib.info.compression == Compression::RLE8
         || context.dib.info.compression == Compression::RLE24) {
diff --git a/Libraries/LibGfx/GIFLoader.cpp b/Libraries/LibGfx/GIFLoader.cpp
@@ -345,7 +345,7 @@ bool load_gif_frame_descriptors(GIFLoadingContext& context)
     if (context.data_size < 32)
         return false;
 
-    auto buffer = ByteBuffer::wrap(context.data, context.data_size);
+    auto buffer = ByteBuffer::wrap(const_cast<u8*>(context.data), context.data_size);
     BufferStream stream(buffer);
 
     Optional<GIFFormat> format = decode_gif_header(stream);
@@ -546,7 +546,7 @@ GIFImageDecoderPlugin::GIFImageDecoderPlugin(const u8* data, size_t size)
     m_context->data_size = size;
 }
 
-GIFImageDecoderPlugin::~GIFImageDecoderPlugin() {}
+GIFImageDecoderPlugin::~GIFImageDecoderPlugin() { }
 
 IntSize GIFImageDecoderPlugin::size()
 {
@@ -591,7 +591,7 @@ bool GIFImageDecoderPlugin::set_nonvolatile()
 
 bool GIFImageDecoderPlugin::sniff()
 {
-    auto buffer = ByteBuffer::wrap(m_context->data, m_context->data_size);
+    auto buffer = ByteBuffer::wrap(const_cast<u8*>(m_context->data), m_context->data_size);
     BufferStream stream(buffer);
     return decode_gif_header(stream).has_value();
 }
diff --git a/Libraries/LibGfx/ICOLoader.cpp b/Libraries/LibGfx/ICOLoader.cpp
@@ -170,7 +170,7 @@ static size_t find_largest_image(const ICOLoadingContext& context)
     size_t max_area = 0;
     size_t index = 0;
     size_t largest_index = 0;
-    for(const auto& desc : context.images) {
+    for (const auto& desc : context.images) {
         if (desc.width * desc.height > max_area) {
             max_area = desc.width * desc.height;
             largest_index = index;
@@ -182,7 +182,7 @@ static size_t find_largest_image(const ICOLoadingContext& context)
 
 static bool load_ico_directory(ICOLoadingContext& context)
 {
-    auto buffer = ByteBuffer::wrap(context.data, context.data_size);
+    auto buffer = ByteBuffer::wrap(const_cast<u8*>(context.data), context.data_size);
     auto stream = BufferStream(buffer);
     auto image_count = decode_ico_header(stream);
     if (!image_count.has_value() || image_count.value() == 0) {
@@ -280,7 +280,7 @@ static bool load_ico_bmp(ICOLoadingContext& context, ImageDescriptor& desc)
     }
 
     // Mask is 1bpp, and each row must be 4-byte aligned
-    size_t mask_row_len = align_up_to(align_up_to(desc.width, 8)/8, 4);
+    size_t mask_row_len = align_up_to(align_up_to(desc.width, 8) / 8, 4);
     size_t required_len = desc.height * (desc.width * sizeof(BMP_ARGB) + mask_row_len);
     size_t available_len = desc.size - sizeof(info);
     if (required_len > available_len) {
@@ -408,7 +408,7 @@ bool ICOImageDecoderPlugin::set_nonvolatile()
 
 bool ICOImageDecoderPlugin::sniff()
 {
-    auto buffer = ByteBuffer::wrap(m_context->data, m_context->data_size);
+    auto buffer = ByteBuffer::wrap(const_cast<u8*>(m_context->data), m_context->data_size);
     BufferStream stream(buffer);
     return decode_ico_header(stream).has_value();
 }
diff --git a/Libraries/LibGfx/JPGLoader.cpp b/Libraries/LibGfx/JPGLoader.cpp
@@ -1146,7 +1146,7 @@ static bool scan_huffman_stream(BufferStream& stream, JPGLoadingContext& context
 
 static bool decode_jpg(JPGLoadingContext& context)
 {
-    ByteBuffer buffer = ByteBuffer::wrap(context.data, context.data_size);
+    ByteBuffer buffer = ByteBuffer::wrap(const_cast<u8*>(context.data), context.data_size);
     BufferStream stream(buffer);
     if (!parse_header(stream, context))
         return false;
diff --git a/Libraries/LibGfx/PNGLoader.cpp b/Libraries/LibGfx/PNGLoader.cpp
@@ -166,7 +166,7 @@ class Streamer {
     {
         if (m_size_remaining < count)
             return false;
-        buffer = ByteBuffer::wrap(m_data_ptr, count);
+        buffer = ByteBuffer::wrap(const_cast<u8*>(m_data_ptr), count);
         m_data_ptr += count;
         m_size_remaining -= count;
         return true;
diff --git a/Libraries/LibHTTP/HttpsJob.cpp b/Libraries/LibHTTP/HttpsJob.cpp
@@ -88,7 +88,10 @@ void HttpsJob::shutdown()
 
 void HttpsJob::set_certificate(String certificate, String private_key)
 {
-    if (!m_socket->add_client_key(ByteBuffer::wrap(certificate.characters(), certificate.length()), ByteBuffer::wrap(private_key.characters(), private_key.length()))) {
+    if (!m_socket->add_client_key(
+            ByteBuffer::wrap(const_cast<char*>(certificate.characters()), certificate.length()),
+            ByteBuffer::wrap(const_cast<char*>(private_key.characters()), private_key.length()))) {
+
         dbg() << "LibHTTP: Failed to set a client certificate";
         // FIXME: Do something about this failure
         ASSERT_NOT_REACHED();
diff --git a/Libraries/LibTLS/Handshake.cpp b/Libraries/LibTLS/Handshake.cpp
@@ -157,7 +157,7 @@ ByteBuffer TLSv12::build_finished()
     auto dummy = ByteBuffer::create_zeroed(0);
 
     auto digest = m_context.handshake_hash.digest();
-    auto hashbuf = ByteBuffer::wrap(digest.immutable_data(), m_context.handshake_hash.digest_size());
+    auto hashbuf = ByteBuffer::wrap(const_cast<u8*>(digest.immutable_data()), m_context.handshake_hash.digest_size());
     pseudorandom_function(outbuffer, m_context.master_key, (const u8*)"client finished", 15, hashbuf, dummy);
 
     builder.append(outbuffer);
diff --git a/Libraries/LibTLS/Record.cpp b/Libraries/LibTLS/Record.cpp
@@ -239,7 +239,7 @@ ssize_t TLSv12::handle_message(const ByteBuffer& buffer)
         memcpy(temp_buf, buffer.offset_pointer(0), 3);
         *(u16*)(temp_buf + 3) = convert_between_host_and_network(length);
         auto hmac = hmac_message(ByteBuffer::wrap(temp_buf, 5), decrypted, mac_size);
-        auto message_mac = ByteBuffer::wrap(message_hmac, mac_size);
+        auto message_mac = ByteBuffer::wrap(const_cast<u8*>(message_hmac), mac_size);
         if (hmac != message_mac) {
             dbg() << "integrity check failed (mac length " << length << ")";
             dbg() << "mac received:";
diff --git a/Libraries/LibTLS/TLSv12.cpp b/Libraries/LibTLS/TLSv12.cpp
@@ -265,7 +265,7 @@ static ssize_t _parse_asn1(const Context& context, Certificate& cert, const u8*
                     if (length == 1)
                         cert.version = buffer[position];
                 }
-                // print_buffer(ByteBuffer::wrap(buffer + position, length));
+                // print_buffer(ByteBuffer::wrap(const_cast<u8*>(buffer) + position, length));
                 break;
             case 0x03:
                 if (_asn1_is_field_present(fields, Constants::pk_id)) {
diff --git a/Libraries/LibWeb/Loader/ResourceLoader.cpp b/Libraries/LibWeb/Loader/ResourceLoader.cpp
@@ -117,7 +117,7 @@ void ResourceLoader::load(const URL& url, Function<void(const ByteBuffer&, const
     if (url.protocol() == "about") {
         dbg() << "Loading about: URL " << url;
         deferred_invoke([success_callback = move(success_callback)](auto&) {
-            success_callback(ByteBuffer::wrap(String::empty().characters(), 1), {});
+            success_callback(ByteBuffer::wrap(const_cast<char*>(String::empty().characters()), 1), {});
         });
         return;
     }
diff --git a/Userland/test-crypto.cpp b/Userland/test-crypto.cpp
@@ -189,12 +189,15 @@ void tls(const char* message, size_t len)
 
 void aes_cbc(const char* message, size_t len)
 {
-    auto buffer = ByteBuffer::wrap(message, len);
+    auto buffer = ByteBuffer::wrap(const_cast<char*>(message), len);
     // FIXME: Take iv as an optional parameter
     auto iv = ByteBuffer::create_zeroed(Crypto::Cipher::AESCipher::block_size());
 
     if (encrypting) {
-        Crypto::Cipher::AESCipher::CBCMode cipher(ByteBuffer::wrap(secret_key, strlen(secret_key)), key_bits, Crypto::Cipher::Intent::Encryption);
+        Crypto::Cipher::AESCipher::CBCMode cipher(
+            ByteBuffer::wrap(const_cast<char*>(secret_key), strlen(secret_key)),
+            key_bits,
+            Crypto::Cipher::Intent::Encryption);
 
         auto enc = cipher.create_aligned_buffer(buffer.size());
         (void)cipher.encrypt(buffer, enc, iv);
@@ -204,7 +207,10 @@ void aes_cbc(const char* message, size_t len)
         else
             print_buffer(enc, Crypto::Cipher::AESCipher::block_size());
     } else {
-        Crypto::Cipher::AESCipher::CBCMode cipher(ByteBuffer::wrap(secret_key, strlen(secret_key)), key_bits, Crypto::Cipher::Intent::Decryption);
+        Crypto::Cipher::AESCipher::CBCMode cipher(
+            ByteBuffer::wrap(const_cast<char*>(secret_key), strlen(secret_key)),
+            key_bits,
+            Crypto::Cipher::Intent::Decryption);
         auto dec = cipher.create_aligned_buffer(buffer.size());
         cipher.decrypt(buffer, dec, iv);
         printf("%.*s\n", (int)dec.size(), dec.data());

Original file line number	Diff line number	Diff line change
`@@ -66,7 +66,7 @@ class Image {`
`66`	`66`	`{`
`67`	`67`	`}`
`68`	`68`
`69`		`- ~Symbol() {}`
	`69`	`+ ~Symbol() { }`
`70`	`70`
`71`	`71`	`StringView name() const { return m_image.table_string(m_sym.st_name); }`
`72`	`72`	`unsigned section_index() const { return m_sym.st_shndx; }`
`@@ -92,7 +92,7 @@ class Image {`
`92`	`92`	`, m_program_header_index(program_header_index)`
`93`	`93`	`{`
`94`	`94`	`}`
`95`		`- ~ProgramHeader() {}`
	`95`	`+ ~ProgramHeader() { }`
`96`	`96`
`97`	`97`	`unsigned index() const { return m_program_header_index; }`
`98`	`98`	`u32 type() const { return m_program_header.p_type; }`
`@@ -122,7 +122,7 @@ class Image {`
`122`	`122`	`, m_section_index(sectionIndex)`
`123`	`123`	`{`
`124`	`124`	`}`
`125`		`- ~Section() {}`
	`125`	`+ ~Section() { }`
`126`	`126`
`127`	`127`	`StringView name() const { return m_image.section_header_table_string(m_section_header.sh_name); }`
`128`	`128`	`unsigned type() const { return m_section_header.sh_type; }`
`@@ -132,7 +132,7 @@ class Image {`
`132`	`132`	`unsigned entry_count() const { return !entry_size() ? 0 : size() / entry_size(); }`
`133`	`133`	`u32 address() const { return m_section_header.sh_addr; }`
`134`	`134`	`const char* raw_data() const { return m_image.raw_data(m_section_header.sh_offset); }`
`135`		`- ByteBuffer wrapping_byte_buffer() { return ByteBuffer::wrap(reinterpret_cast<const u8*>(raw_data()), size()); }`
	`135`	`+ ByteBuffer wrapping_byte_buffer() { return ByteBuffer::wrap(const_cast<char*>(raw_data()), size()); }`
`136`	`136`	`bool is_undefined() const { return m_section_index == SHN_UNDEF; }`
`137`	`137`	`const RelocationSection relocations() const;`
`138`	`138`	`u32 flags() const { return m_section_header.sh_flags; }`
`@@ -166,7 +166,7 @@ class Image {`
`166`	`166`	`{`
`167`	`167`	`}`
`168`	`168`
`169`		`- ~Relocation() {}`
	`169`	`+ ~Relocation() { }`
`170`	`170`
`171`	`171`	`unsigned offset() const { return m_rel.r_offset; }`
`172`	`172`	`unsigned type() const { return ELF32_R_TYPE(m_rel.r_info); }`
Original file line number	Diff line number	Diff line change
`@@ -1104,7 +1104,7 @@ static bool decode_bmp_pixel_data(BMPLoadingContext& context)`
`1104`	`1104`	`return false;`
`1105`	`1105`	`}`
`1106`	`1106`
`1107`		`- auto buffer = ByteBuffer::wrap(context.data + context.data_offset, context.data_size);`
	`1107`	`+ auto buffer = ByteBuffer::wrap(const_cast<u8*>(context.data + context.data_offset), context.data_size);`
`1108`	`1108`
`1109`	`1109`	`if (context.dib.info.compression == Compression::RLE4 \|\| context.dib.info.compression == Compression::RLE8`
`1110`	`1110`	`\|\| context.dib.info.compression == Compression::RLE24) {`
Original file line number	Diff line number	Diff line change
`@@ -345,7 +345,7 @@ bool load_gif_frame_descriptors(GIFLoadingContext& context)`
`345`	`345`	`if (context.data_size < 32)`
`346`	`346`	`return false;`
`347`	`347`
`348`		`- auto buffer = ByteBuffer::wrap(context.data, context.data_size);`
	`348`	`+ auto buffer = ByteBuffer::wrap(const_cast<u8*>(context.data), context.data_size);`
`349`	`349`	`BufferStream stream(buffer);`
`350`	`350`
`351`	`351`	`Optional<GIFFormat> format = decode_gif_header(stream);`
`@@ -546,7 +546,7 @@ GIFImageDecoderPlugin::GIFImageDecoderPlugin(const u8* data, size_t size)`
`546`	`546`	`m_context->data_size = size;`
`547`	`547`	`}`
`548`	`548`
`549`		`-GIFImageDecoderPlugin::~GIFImageDecoderPlugin() {}`
	`549`	`+GIFImageDecoderPlugin::~GIFImageDecoderPlugin() { }`
`550`	`550`
`551`	`551`	`IntSize GIFImageDecoderPlugin::size()`
`552`	`552`	`{`
`@@ -591,7 +591,7 @@ bool GIFImageDecoderPlugin::set_nonvolatile()`
`591`	`591`
`592`	`592`	`bool GIFImageDecoderPlugin::sniff()`
`593`	`593`	`{`
`594`		`- auto buffer = ByteBuffer::wrap(m_context->data, m_context->data_size);`
	`594`	`+ auto buffer = ByteBuffer::wrap(const_cast<u8*>(m_context->data), m_context->data_size);`
`595`	`595`	`BufferStream stream(buffer);`
`596`	`596`	`return decode_gif_header(stream).has_value();`
`597`	`597`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1146,7 +1146,7 @@ static bool scan_huffman_stream(BufferStream& stream, JPGLoadingContext& context`
`1146`	`1146`
`1147`	`1147`	`static bool decode_jpg(JPGLoadingContext& context)`
`1148`	`1148`	`{`
`1149`		`- ByteBuffer buffer = ByteBuffer::wrap(context.data, context.data_size);`
	`1149`	`+ ByteBuffer buffer = ByteBuffer::wrap(const_cast<u8*>(context.data), context.data_size);`
`1150`	`1150`	`BufferStream stream(buffer);`
`1151`	`1151`	`if (!parse_header(stream, context))`
`1152`	`1152`	`return false;`
Original file line number	Diff line number	Diff line change
`@@ -166,7 +166,7 @@ class Streamer {`
`166`	`166`	`{`
`167`	`167`	`if (m_size_remaining < count)`
`168`	`168`	`return false;`
`169`		`- buffer = ByteBuffer::wrap(m_data_ptr, count);`
	`169`	`+ buffer = ByteBuffer::wrap(const_cast<u8*>(m_data_ptr), count);`
`170`	`170`	`m_data_ptr += count;`
`171`	`171`	`m_size_remaining -= count;`
`172`	`172`	`return true;`