| Bug #112304 | MEM_ROOT::AllocBlock() does not satisfy minimum_length bigger than wanted_length | ||
|---|---|---|---|
| Submitted: | 9 Sep 2023 18:07 | Modified: | 3 Jan 15:22 |
| Reporter: | Kaiwang CHen (OCA) | Email Updates: | |
| Status: | Closed | Impact on me: | |
| Category: | MySQL Server: Optimizer | Severity: | S3 (Non-critical) |
| Version: | 8.0 | OS: | Any |
| Assigned to: | CPU Architecture: | Any | |
| Tags: | Contribution | ||
[9 Sep 2023 18:10]
Kaiwang CHen
Satisfy minimum_length, assert overflows. (*) I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.
Contribution: my_alloc.patch (application/octet-stream, text), 3.32 KiB.
[11 Sep 2023 3:41]
Kaiwang CHen
hcduan reported that gis.gis_bugs_crashes crashed if the fix in my_alloc.patch was replaced with assert(length >= minimum_length).
+ // Any successful allocation should have enough memory to satisfy
+ // minimum_length.
+ // if (unlikely(length < minimum_length)) {
+ // length = ALIGN_SIZE(minimum_length);
+ // }
+ assert(length >= minimum_length);
This is exactly the statement "The problem did not hurt much since its introduction in WL #13459: Optimize hash table in hash join [patch 3/9 and 4/9], because the work log implementation depended on Peek() rather than return value of ForceNewBlock() thus sidestep the problem by chance."
By ForceNewBlock()-Peek()-and-if-peeked-size-satisfied-request, overwriting the memory chunk is avoided, however, the chunk is useless and wasted. The allocation will be attempted in m_overflow_mem_root, which is apparently abused.
It could be verified with gdb when both the assert and the fix are removed:
(gdb) p m_mem_root.m_current_free_end - m_mem_root.m_current_free_start
$3 = 24576
(gdb) p required_value_bytes
$4 = 60719
...
(gdb) n
78 if (static_cast<size_t>(block.second - block.first) >= required_value_bytes) {
(gdb)
82 pointer_cast<char *>(m_overflow_mem_root.Alloc(required_value_bytes));
(gdb)
69 std::pair<char *, char *> block = m_mem_root.Peek();
70 if (static_cast<size_t>(block.second - block.first) < required_value_bytes) {
71 // No room in this block; ask for a new one and try again.
72 m_mem_root.ForceNewBlock(required_value_bytes);
73 block = m_mem_root.Peek();
74 }
77 LinkedImmutableString ret{nullptr};
78 if (static_cast<size_t>(block.second - block.first) >= required_value_bytes) {
79 dptr = start_of_value = block.first;
80 } else {
81 dptr = start_of_value =
82 pointer_cast<char *>(m_overflow_mem_root.Alloc(required_value_bytes));
83 if (dptr == nullptr) {
84 return LinkedImmutableString{nullptr};
85 }
86 committed = true;
[11 Sep 2023 4:56]
MySQL Verification Team
Hello Kaiwang, Thank you for the report and contribution. regards, Umesh
[3 Jan 15:22]
Jon Stephens
Documented fix as follows in the MySQL 8.3.0 changelog:
MEM_ROOT::AllocBlock() did not satisfy the condition
minimum_length > wanted_length, due to a mismatch between
block size and mininum_length. A successful return (false) from
ForceNewBlock() is expected to have a new block supporting
minimum_length, but this assumption failed when mininum_length
was larger; thus AllocBlock() did not conform to its contract.
This fix is based on a contribution from Kaiwang Chen.
Closed.
Description: MEM_ROOT increases allocation size by 50% in each step, and the state is tracked in m_block_size. ForceNewBlock(minimum_length), a public low-level method, takes the state as wanted_length to invoke AllocBlock(). There could be a mismatch between block size and mininum_length. A successful return (false) from ForceNewBlock() is expected to have a new block supporting minimum_length. However, the assumption fails when mininum_length is the bigger. Apparently AllocBlock() does not conform to its contract: /** Allocate a new block of the given length (plus overhead for the block header). If the MEM_ROOT is near capacity, it may allocate less memory than wanted_length, but if it cannot allocate at least minimum_length, will return nullptr. */ Block *AllocBlock(size_t wanted_length, size_t minimum_length); The problem did not hurt much since its introduction in WL #13459: Optimize hash table in hash join [patch 3/9 and 4/9], because the work log implementation depended on Peak() rather than return value of ForceNewBlock() thus sidestep the problem by chance. It bites when using the return value directly. For example, an overflowed block size in the slow path of Alloc() could cheat client code into daunting memory overwrites. How to repeat: Patch my_alloc-t.cc with the following code to verify the problem. Note that the second test is commented out in the separately submitted fix to avoid assertion. diff --git a/unittest/gunit/my_alloc-t.cc b/unittest/gunit/my_alloc-t.cc index b5d480d018b..fda8a09812b 100644 --- a/unittest/gunit/my_alloc-t.cc +++ b/unittest/gunit/my_alloc-t.cc @@ -202,6 +202,16 @@ TEST_F(MyAllocTest, RawInterface) { // The value should still be there. EXPECT_STREQ("12345", store_ptr); + + // Get a new block to satisfy more than the current block size (512 * 1.5^2). + EXPECT_FALSE(alloc.ForceNewBlock(2048)); + block = alloc.Peek(); + EXPECT_EQ(2048, block.second - block.first); + + // This should give an assertion because ALIGN_SIZE(ULONG_MAX) overflows. + MEM_ROOT alloc2(PSI_NOT_INSTRUMENTED, ULONG_MAX); + EXPECT_TRUE(alloc2.Alloc(10) != nullptr); + EXPECT_EQ(ALIGN_SIZE(10),alloc2.allocated_size()); } TEST_F(MyAllocTest, ArrayAllocInitialization) { Suggested fix: Make AllocBlock() conform to its contract. Add some assertions to check overflows; explicit overflow tests are even better. Besides, block size could be aligned when passed in as argument of constructor or set_block_size().