MySQL Bugs: #89419: Incorrect use of std::max

Bug #89419	Incorrect use of std::max
Submitted:	25 Jan 2018 16:06	Modified:	22 Oct 2018 6:25
Reporter:	Zsolt Parragi (OCA)	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: Optimizer	Severity:	S3 (Non-critical)
Version:	5.6, 5.7	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:

sql/sql_join_buffer.h contains two member functions named rem_space, with the same implementation:

  ulong rem_space() 
  { 
    return std::max<ulong>(buff_size-(end_pos-buff)-aux_buff_size, 0UL);
  }

Because it uses the ulong template parameter, the compiler performs an implicit cast to ulong on the first parameter before calling the function.

This means that it will be always at least 0.

The correct implementation of this function is to use a signed max, then cast the result to an unsigned type:

    return static_cast<ulong>(
      std::max<long>(buff_size-(end_pos-buff)-aux_buff_size, 0L)
    );

How to repeat:
Found as a warning when compiling with clang

Hi!

Thank you for your bug report.

In order to proceed further, could you please let us know why do you think that first parameter can be a negative value. Please, provide a real and convincing example.

Second, regardless if the first parameter is negative or zero, the result of the max() standard function would be the same in both cases, id est 0 (zero). So, why change anything ???

Hello

For the why: I don't know when it would be negative - but the original code tries to take the maximum of the value of the expression, and 0. That only makes sense if the value can in fact be negative. I only assumed that the original code is there for the reason, and the expression can reach a negative value.

If it can't be a negative number, removing the std::max check would also be correct.

For the second part, that's not correct, it's not the same.

The original code uses max<ulong>, which means that it performs an unsigned comparison.

Imagine the following:

return std::max<ulong>(-1, 0UL);

Here, first -1, which is a signed integer, will be converted to an unsigned integer. This basically means that the binary representation of -1 will be treated as an unsigned number, which results in a quite large number. In the case of -1, it'll be actually std::numeric_limits<ulong>::max(), the possible largest unsigned long value (see two'c complement).

max will be only called after this conversion, and that large value will be returned.

With the changed code, we call the signed version of the maximum function: first we take the maximum of the expression's value and 0, both as signed number. This means that the result of the maximum call will be guaranteed to be at least 0, and an unsigned cast is safe in this case.

Hi,

You are quite write about <ulong> typecast, but I have investigated further and was assured that first parameter can never be negative. It is due to the fact of how are all values calculated.

Not a bug.

If the first argument can never be non-nonnegative, then why a

return std::max<ulong>(buff_size-(end_pos-buff)-aux_buff_size, 0UL);

is "not a bug" implementation and not

return buff_size-(end_pos-buff)-aux_buff_size;

one, optionally with DBUG_ASSERT(buff_size > (end_pos - buff) + aux_buff_size) ? Given that the former also results in a compiler warning, which we are trying to fix?

Hello

You say that you investigated it, and it can't be negative.

But since you closed this as not a bug -- which I do not agree with, even if it couldn't be negative, because it is a possible bug source --, I also wanted to be sure, so I searched for those variables too.

So here's a possible way to make it negative, in write_record_data.

First, the size of the aux buffer is adjusted:

  uint incr= aux_buffer_incr();
  ulong rem= rem_space();
  uint len= pack_length;
  aux_buff_size+= len+incr < rem ? incr : rem;

As you can see, if the condition is false, the remaining space will be exactly 0 after this, as everything is reserved for the aux buffer.

Later in the  same function:

  if (with_length)
  {
    rec_len_ptr= cp;   
    cp+= size_of_rec_len;
  }

And there are some other conditions, which possibly increase the value of cp, even if the buffer is full.

Which, at the end, will be written to end pos without any condition, always:

  end_pos= pos= cp;

So, if:

* the auxiliary buffer takes up all the remaining space
* and even one of these conditions increase the value of cp

The result of the expression will be negative.

Hi!

Your analysis is not convincing as it is has couple of false presumptions. However, I am verifying this bug, in order that one assert() is added (may be only in debug mode) for the possible negative value. This is a part of code that is not changed much and we never had this situation.

However, adding one assert would be welcome, as other changes in the code might ensue.

Verified.

https://bugs.mysql.com/bug.php?id=90853

In file included from ./mysql-5.7.22/sql/sql_optimizer.cc:37:
./mysql-5.7.22/sql/sql_join_buffer.h:347:12: warning: taking the max of a value and unsigned zero is always equal to the other value [-Wmax-unsigned-zero]
    return std::max<ulong>(buff_size-(end_pos-buff)-aux_buff_size, 0UL);

This was fixed in 8.0 (bug#83254).

This is fixed only in 8.0, because a fix for this bug was far too complex for either 5.6 or 5.7 ......