MySQL Bugs: #89417: Incorrect assert about the checksum length

Bug #89417	Incorrect assert about the checksum length
Submitted:	25 Jan 2018 15:48	Modified:	16 Mar 2018 14:50
Reporter:	Zsolt Parragi (OCA)	Email Updates:
Status:	Verified	Impact on me:	None
Category:	MySQL Server: Logging	Severity:	S3 (Non-critical)
Version:	5.6, 5.7	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
For 5.6, in sql/log_event.cc:

    DBUG_ASSERT((bytes_read == data_written -
                 (old_pre_checksum_fd ||
                  (description_event->checksum_alg ==
                   BINLOG_CHECKSUM_ALG_OFF)) ?
                 0 : BINLOG_CHECKSUM_LEN)
                ||
                (bytes_read == data_written -1 -
                 (old_pre_checksum_fd ||
                  (description_event->checksum_alg ==
                   BINLOG_CHECKSUM_ALG_OFF)) ?
                 0 : BINLOG_CHECKSUM_LEN));

For 5.7, in libbinlogevents/src/statement_events.cc:

  BAPI_ASSERT(((bytes_read == data_written) ? false : true) ||
              ((bytes_read == data_written - 1) ? false : true));

In both cases:

* if bytes_read == data_written, then the second condition can't be true, and the negation will evaluate to true
* similarly, if the second condition is true, then the first can't be true, and it'll be negated to a true value

So the expression will never evaluate to false, and the assert will never trigger.

I think in both cases, the assert should be:

   const int checksum_length = (old_pre_checksum_fd || description_event->footer()->checksum_alg == BINLOG_CHECKSUM_ALG_OFF) ? 0 : BINLOG_CHECKSUM_LEN;
   size_t data_written= (header()->data_written- checksum_length);
   BAPI_ASSERT(bytes_read == data_written || (bytes_read == data_written - 1));

How to repeat:
found when analyzing gcc7 warnings

Hi!

Thank you for your bug report.

I have analysed thoroughly your report and I must admit that I have a problem with your logic. In both cases, 5.6 and 5.7, both conditions are ORed, as they are logically evalueated with || operator.

Hence:

true || false 

and

false || true

will both evalueate to true !!!!!

Hello

Yes, that's exactly my point - in their current state, these assertions will be always true, which means, that they effectively don't check anything.

My proposed change, at the end of the description corrects this - that's only true in two cases: when

* bytes_read == data_written
* or bytes_read == data_written - 1

Which I think is the intent behind the original assertion.

Hi,

There are definitely cases where both parts can be false.

Hello

In my version, yes - that's the point of the assertion.

But in the originals, I don't think so - the expression inside the assertion always ends up as "true" which means it's basically a no-op.

Hi!

There are cases, although rare, where both can be false. 

And we stick to our version, we do not know any other versions.

Sinisa,

Could you please tell us what are those "rare cases" when

BAPI_ASSERT(((bytes_read == data_written) ? false : true) ||
              ((bytes_read == data_written - 1) ? false : true));

both parts can be false?

Hi,

Memory corruption, of course ....

Hello

Could you show me an example then please?

This is the current code from 5.7:

  BAPI_ASSERT(((bytes_read == data_written) ? false : true) ||
              ((bytes_read == data_written - 1) ? false : true));

This generates an essertion when the expression inside evaluates to false:

BAPI_ASSERT(false)

Inside this assertion is an expression with an or operator:

BAPI_ASSERT(a || b)

So this only evaluates to false, if both are true.

In our case, these  are:

a: (bytes_read == data_written) ? false : true) 
b: (bytes_read == data_written - 1) ? false : true)

This is essentaially a negation, so lets simplify this a bit:

a: !(bytes_read == data_written)
b: !(bytes_read == data_written - 1)

We can further simplify this by moving the outside negation inside, by changing the operation:

a: bytes_read != data_written
b: bytes_read != data_written - 1

So this assertion will only fire if both of these expressions evaluate to false.

If the first is false, that means that bytes_read == data_written.
In this case, the second expression has to be true - if it equals to data_written, it can't equal to data_written - 1.

Same does for the opposite case.

Hi!

If both parts of the || operator are true, the expression evaluates to true. And `a` and `b` can not both be true.

Yes, we can pretend for a moment that variables in question are declared "volatile" (they are not), and that "a memory corruption" (of course) will happen right between two loads. The developer will review the crash and will know immediately "oh, that, a memory corruption". Might as well checksum the heap, would catch more corruptions.

But since the variables are not volatile, maybe the real explanation for those "rare cases" would be compiler codegen bugs? Maybe cosmic rays hitting CPU and flipping register bits? Do they have ECC there?

Sinisa, please let us know if our bug reports are not welcome here

The expression evaluates to true if any of the parts are true, it's an OR, not an AND.

And the question is, when does it evaluate to false, as it is an assertion.

Hi,

I fully understand your logic, but i do not know about this BAPI_ASSERT.

I have searched both 5.6 and 5.7 sources and I can't find it ......

Otherwise, your logic is correct ......

I mentioned the filenames for both 5.6 and 5.7 in my original bug report.

Here's the exact location for 5.7:
https://github.com/mysql/mysql-server/blob/5.7/libbinlogevents/src/statement_events.cpp#L5...

True.

We thank you for your bug report that is quite clear right now. We shall also enjoy analysing all your future bug reports.

Verified  as reported......