MySQL Bugs: #95606: ib_sdi_get may return false positive DB_OUT_OF

Bug #95606	ib_sdi_get may return false positive DB_OUT_OF_MEMORY
Submitted:	3 Jun 2019 6:49	Modified:	10 Jul 2019 18:51
Reporter:	Sergei Glushchenko	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: InnoDB storage engine	Severity:	S3 (Non-critical)
Version:	8.0.16	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
ib_sdi_get stores compressed SDI into provided buffer. It also returns two lengths, actual length of compressed SDI and actual length of uncompressed SDI. It also returns DB_OUT_OF_MEMORY when buffer is too small to accommodate compressed SDI.

The bug is that DB_OUT_OF_MEMORY returned when size of provided buffer is smaller than uncompressed SDI length. So, caller must provide a buffer that is large enough to accommodate uncompressed SDI in order to get compressed SDI.

How to repeat:
Read the code

Suggested fix:
diff --git a/storage/innobase/api/api0api.cc b/storage/innobase/api/api0api.cc
index 82703153c8e..a2dbdced0ee 100644
--- a/storage/innobase/api/api0api.cc
+++ b/storage/innobase/api/api0api.cc
@@ -3071,7 +3071,7 @@ dberr_t ib_sdi_get(uint32_t tablespace_id, const ib_sdi_key_t *ib_sdi_key,

       /* If the passed memory is not sufficient, we
       return failure and the actual length of SDI. */
-      if (buf_len < *uncomp_sdi_len) {
+      if (buf_len < *comp_sdi_len) {
         ib_tuple_delete(tuple);
         ib_tuple_delete(key_tpl);
         ib_cursor_close(ib_crsr);

Hi Mr. Glushchenko,

Thank you for your bug report.

I have analysed the code, compress API details and your patch.

I am afraid that this is not so simple, so the patch should be a bit different. It is theoretically possible that compressed size is larger than uncompressed size.  Hence, wouldn't it be better to set variable `buf_len` to the larger of two values and then do away with that conditional altogether.

What is your opinion.

Note that ib_sdi_get returns both compressed and uncompressed length. But buffer is passed to store compressed SDI. I would expect caller to pass buffer large enough for compressed SDI, then allocate another buffer large enough for uncompressed SDI and decompress SDI into it. Note also that ib_sdi_get does not uncompress SDI, it is caller's responsibility to perform decompression.

Hi,

Thank you for your answer.

My question is whether you still stand behind your patch. Do you  think that no other changes would be necessary. 

You have, seemingly, missed a code six lines above which does the following:

    uint32_t buf_len = *comp_sdi_len;

hence, your patch would look like this, in effect:

       if (*comp_sdi_len < *comp_sdi_len)

Are you sure that this is what you want ???/

I didn't miss that code. It is you who missed following code two lines above:

      ib_tuple_read_u32(tuple, 2, uncomp_sdi_len);
      ib_tuple_read_u32(tuple, 3, comp_sdi_len);

comp_sdi_len is use as both input and output. As input it is used to pass compbressed buffer length provided by caller and as an output it is used to return actual comppressed buffer length.

so, condition transforms to:

if (provided_comp_buf_len < actual_comp_buf_len) {
...
}

Hi,

I have not been visiting that part of the code for the long, long time.

In short, you are correct. Not only because of the retrieval of the data from the tuple, but also for the fact that decompression occurs in other functions, after this function has already being called.

Verified as reported.

Posted by developer:
 
Fixed as of the upcoming 8.0.18 release, and here's the changelog entry:

An incorrect argument was used to compare serialized dictionary
information (SDI) input and output values while checking the size of the
buffer used to store SDI.

Daniel,

Thanks a lot .....