From bdd3c6fe4dfa4e6ddd3792de1efa0778a26972fe Mon Sep 17 00:00:00 2001 From: Pablo Andres Fuente Date: Wed, 9 Oct 2024 23:23:04 -0300 Subject: [PATCH] Fix `StreamPeerGZIP::finish()` internal buffer size usage Fixes #97201 Instead of using and arbitrary fixed size for the internal buffer, the remaining available bytes of the internal `RingBuffer` is used. Also add unit tests for `StreamPeerGZIP` --- core/io/stream_peer_gzip.cpp | 6 +- doc/classes/StreamPeerGZIP.xml | 3 +- tests/core/io/test_stream_peer_gzip.h | 198 ++++++++++++++++++++++++++ tests/test_main.cpp | 1 + 4 files changed, 204 insertions(+), 4 deletions(-) create mode 100644 tests/core/io/test_stream_peer_gzip.h diff --git a/core/io/stream_peer_gzip.cpp b/core/io/stream_peer_gzip.cpp index 514bcf59b8e..ed247eaffbc 100644 --- a/core/io/stream_peer_gzip.cpp +++ b/core/io/stream_peer_gzip.cpp @@ -195,12 +195,12 @@ int StreamPeerGZIP::get_available_bytes() const { Error StreamPeerGZIP::finish() { ERR_FAIL_COND_V(!ctx || !compressing, ERR_UNAVAILABLE); // Ensure we have enough space in temporary buffer. - if (buffer.size() < 1024) { - buffer.resize(1024); // 1024 should be more than enough. + if (buffer.size() < get_available_bytes()) { + buffer.resize(get_available_bytes()); // get_available_bytes() is what we can store in RingBuffer. } int consumed = 0; int to_write = 0; - Error err = _process(buffer.ptrw(), 1024, nullptr, 0, consumed, to_write, true); // compress + Error err = _process(buffer.ptrw(), buffer.size(), nullptr, 0, consumed, to_write, true); // compress if (err != OK) { return err; } diff --git a/doc/classes/StreamPeerGZIP.xml b/doc/classes/StreamPeerGZIP.xml index e3f8441d195..01ca7d9077c 100644 --- a/doc/classes/StreamPeerGZIP.xml +++ b/doc/classes/StreamPeerGZIP.xml @@ -19,7 +19,8 @@ - Finalizes the stream, compressing or decompressing any buffered chunk left. + Finalizes the stream, compressing any buffered chunk left. + You must call it only when you are compressing. diff --git a/tests/core/io/test_stream_peer_gzip.h b/tests/core/io/test_stream_peer_gzip.h new file mode 100644 index 00000000000..9918ddefe93 --- /dev/null +++ b/tests/core/io/test_stream_peer_gzip.h @@ -0,0 +1,198 @@ +/**************************************************************************/ +/* test_stream_peer_gzip.h */ +/**************************************************************************/ +/* This file is part of: */ +/* GODOT ENGINE */ +/* https://godotengine.org */ +/**************************************************************************/ +/* Copyright (c) 2014-present Godot Engine contributors (see AUTHORS.md). */ +/* Copyright (c) 2007-2014 Juan Linietsky, Ariel Manzur. */ +/* */ +/* Permission is hereby granted, free of charge, to any person obtaining */ +/* a copy of this software and associated documentation files (the */ +/* "Software"), to deal in the Software without restriction, including */ +/* without limitation the rights to use, copy, modify, merge, publish, */ +/* distribute, sublicense, and/or sell copies of the Software, and to */ +/* permit persons to whom the Software is furnished to do so, subject to */ +/* the following conditions: */ +/* */ +/* The above copyright notice and this permission notice shall be */ +/* included in all copies or substantial portions of the Software. */ +/* */ +/* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, */ +/* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF */ +/* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. */ +/* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY */ +/* CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, */ +/* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE */ +/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ +/**************************************************************************/ + +#ifndef TEST_STREAM_PEER_GZIP_H +#define TEST_STREAM_PEER_GZIP_H + +#include "core/io/stream_peer_gzip.h" +#include "tests/test_macros.h" + +namespace TestStreamPeerGZIP { + +const String hello = "Hello World!!!"; + +TEST_CASE("[StreamPeerGZIP] Initialization") { + Ref spgz; + spgz.instantiate(); + CHECK_EQ(spgz->get_available_bytes(), 0); +} + +TEST_CASE("[StreamPeerGZIP] Compress/Decompress") { + Ref spgz; + spgz.instantiate(); + + bool is_deflate = false; + + SUBCASE("GZIP") { + is_deflate = false; + } + + SUBCASE("DEFLATE") { + is_deflate = true; + } + + CHECK_EQ(spgz->start_compression(is_deflate), Error::OK); + CHECK_EQ(spgz->put_data(hello.to_ascii_buffer().ptr(), hello.to_ascii_buffer().size()), Error::OK); + CHECK_EQ(spgz->finish(), Error::OK); + + Vector hello_compressed; + int hello_compressed_size = spgz->get_available_bytes(); + hello_compressed.resize(hello_compressed_size); + CHECK_EQ(spgz->get_data(hello_compressed.ptrw(), hello_compressed_size), Error::OK); + + spgz->clear(); + + CHECK_EQ(spgz->start_decompression(is_deflate), Error::OK); + CHECK_EQ(spgz->put_data(hello_compressed.ptr(), hello_compressed.size()), Error::OK); + + Vector hello_decompressed; + int hello_decompressed_size = spgz->get_available_bytes(); + hello_decompressed.resize(hello_decompressed_size); + CHECK_EQ(spgz->get_data(hello_decompressed.ptrw(), hello_decompressed_size), Error::OK); + CHECK_EQ(hello_decompressed, hello.to_ascii_buffer()); +} + +TEST_CASE("[StreamPeerGZIP] Compress/Decompress big chunks of data") { // GH-97201 + Ref spgz; + spgz.instantiate(); + CHECK_EQ(spgz->start_compression(false), Error::OK); + + Vector big_data; + big_data.resize(2500); + // Filling it with random data because the issue is related to the size of the data when it's compress. + // Random data results in bigger compressed data size. + for (int i = 0; i < big_data.size(); i++) { + big_data.write[i] = Math::random(48, 122); + } + CHECK_EQ(spgz->put_data(big_data.ptr(), big_data.size()), Error::OK); + CHECK_EQ(spgz->finish(), Error::OK); + + Vector big_data_compressed; + int big_data_compressed_size = spgz->get_available_bytes(); + big_data_compressed.resize(big_data_compressed_size); + CHECK_EQ(spgz->get_data(big_data_compressed.ptrw(), big_data_compressed_size), Error::OK); + + spgz->clear(); + + CHECK_EQ(spgz->start_decompression(false), Error::OK); + CHECK_EQ(spgz->put_data(big_data_compressed.ptr(), big_data_compressed.size()), Error::OK); + + Vector big_data_decompressed; + int big_data_decompressed_size = spgz->get_available_bytes(); + big_data_decompressed.resize(big_data_decompressed_size); + CHECK_EQ(spgz->get_data(big_data_decompressed.ptrw(), big_data_decompressed_size), Error::OK); + CHECK_EQ(big_data_decompressed, big_data); +} + +TEST_CASE("[StreamPeerGZIP] Can't start twice") { + Ref spgz; + spgz.instantiate(); + CHECK_EQ(spgz->start_compression(false), Error::OK); + + ERR_PRINT_OFF; + CHECK_EQ(spgz->start_compression(false), Error::ERR_ALREADY_IN_USE); + CHECK_EQ(spgz->start_decompression(false), Error::ERR_ALREADY_IN_USE); + ERR_PRINT_ON; +} + +TEST_CASE("[StreamPeerGZIP] Can't start with a buffer size equal or less than zero") { + Ref spgz; + spgz.instantiate(); + + ERR_PRINT_OFF; + CHECK_EQ(spgz->start_compression(false, 0), Error::ERR_INVALID_PARAMETER); + CHECK_EQ(spgz->start_compression(false, -1), Error::ERR_INVALID_PARAMETER); + CHECK_EQ(spgz->start_decompression(false, 0), Error::ERR_INVALID_PARAMETER); + CHECK_EQ(spgz->start_decompression(false, -1), Error::ERR_INVALID_PARAMETER); + ERR_PRINT_ON; +} + +TEST_CASE("[StreamPeerGZIP] Can't put/get data with a buffer size less than zero") { + Ref spgz; + spgz.instantiate(); + CHECK_EQ(spgz->start_compression(false), Error::OK); + + ERR_PRINT_OFF; + CHECK_EQ(spgz->put_data(hello.to_ascii_buffer().ptr(), -1), Error::ERR_INVALID_PARAMETER); + + Vector hello_compressed; + hello_compressed.resize(5); + CHECK_EQ(spgz->get_data(hello_compressed.ptrw(), -1), Error::ERR_INVALID_PARAMETER); + ERR_PRINT_ON; +} + +TEST_CASE("[StreamPeerGZIP] Needs to be started before use") { + Ref spgz; + spgz.instantiate(); + + ERR_PRINT_OFF; + CHECK_EQ(spgz->put_data(hello.to_ascii_buffer().ptr(), hello.to_ascii_buffer().size()), Error::ERR_UNCONFIGURED); + ERR_PRINT_ON; +} + +TEST_CASE("[StreamPeerGZIP] Can't be finished after clear or if it's decompressing") { + Ref spgz; + spgz.instantiate(); + + CHECK_EQ(spgz->start_compression(false), Error::OK); + spgz->clear(); + ERR_PRINT_OFF; + CHECK_EQ(spgz->finish(), Error::ERR_UNAVAILABLE); + ERR_PRINT_ON; + + spgz->clear(); + CHECK_EQ(spgz->start_decompression(false), Error::OK); + ERR_PRINT_OFF; + CHECK_EQ(spgz->finish(), Error::ERR_UNAVAILABLE); + ERR_PRINT_ON; +} + +TEST_CASE("[StreamPeerGZIP] Fails to get if nothing was compress/decompress") { + Ref spgz; + spgz.instantiate(); + + SUBCASE("Compression") { + CHECK_EQ(spgz->start_compression(false), Error::OK); + } + + SUBCASE("Decompression") { + CHECK_EQ(spgz->start_decompression(false), Error::OK); + } + + ERR_PRINT_OFF; + Vector hello_compressed; + hello_compressed.resize(5); + CHECK_EQ(spgz->get_data(hello_compressed.ptrw(), hello_compressed.size()), Error::ERR_UNAVAILABLE); + ERR_PRINT_ON; +} + +} // namespace TestStreamPeerGZIP + +#endif // TEST_STREAM_PEER_GZIP_H diff --git a/tests/test_main.cpp b/tests/test_main.cpp index 465484d6058..569c0dac183 100644 --- a/tests/test_main.cpp +++ b/tests/test_main.cpp @@ -55,6 +55,7 @@ #include "tests/core/io/test_resource.h" #include "tests/core/io/test_stream_peer.h" #include "tests/core/io/test_stream_peer_buffer.h" +#include "tests/core/io/test_stream_peer_gzip.h" #include "tests/core/io/test_xml_parser.h" #include "tests/core/math/test_aabb.h" #include "tests/core/math/test_astar.h"