MDEV-40454: aria_pack UBSAN 'shift exponent 64' in flush_bits()

[prathamesh04]

MDEV-40454: UBSAN shift exponent 64 is too large for 64-bit type 'ulonglong' in aria_pack

Problem

The UBSAN buildbot reports on 10.11:

/home/buildbot/src/storage/maria/aria_pack.c:3035:37: runtime error: shift exponent 64 is too large for 64-bit type 'ulonglong' (aka 'unsigned long long')
    #0 flush_bits  storage/maria/aria_pack.c:3035
    #1 write_huff_tree  storage/maria/aria_pack.c:2435
    #2 compress  storage/maria/aria_pack.c:738
    #3 main  storage/maria/aria_pack.c:274

Root cause

flush_bits() does:

bits= file_buffer.bits & ~7;
bit_buffer= file_buffer.bitbucket >> bits;   // shift exponent 64

file_buffer.bits starts at BITS_SAVED (=64) and counts down as bits are
written. flush_bits() is called at the end of every Huffman-tree block in
write_huff_tree(), even when that block wrote nothing (its tree_buff was
empty / already flushed by the previous block). In that case
file_buffer.bits & ~7 == BITS_SAVED, and bitbucket >> 64 is undefined
behavior.

On x86 the shift count is masked to & 63, so the garbage is silently
folded into the compressed output; under UBSAN with halt_on_error (as on
buildbot) aria_pack aborts. maria.aria_pack_mdev14183 is the existing
test that trips it.

Fix

Return early when there are no whole bytes to flush (bits == BITS_SAVED).
flush_bits() is a no-op in that case, so the change is behavior-preserving
and simply removes the undefined shift.

Verification (11.4 debug + UBSAN)

• Reproduced with -DWITH_UBSAN=ON:
  UBSAN_OPTIONS=halt_on_error=1 aria_pack -t t aborts with the runtime error.
• With the fix: clean run, exit 0, packed data round-trips, CHECK TABLE OK.
• mariadb-test-run --suite=maria aria_pack_mdev14183 passes; full maria
  suite is 60/60 green.

The same flush_bits() exists unchanged in 10.6/10.11/11.4, so the fix
applies to all maintained branches (merge-up from 10.6 if that's your flow).

@vuvova could you take a look?

[grooverdan]

@prathamesh04 . I'm happy to review it. As a 10.11 fix can this be rebased back to there? git rebase --onto origin/10.11 HEAD^; git push --force and edit github PR title base branch to 10.11.

[CLAassistant]

All committers have signed the CLA.

[prathamesh04]

Done as requested — rebased the single commit onto 10.11 and switched the PR base branch to 10.11. The diff is unchanged (just the 2-line flush_bits() guard). Thanks for offering to review! @grooverdan

[gkodinov]

Thank you for your contribution! This is a preliminary review.

LGTM, but I also have a question below.

Please stand by for the final review.

[gkodinov]

I believe this needs to be set to 0 on the return branch. I'd style the whole thing a bit differently:

static void flush_bits(void)
{
  int bits;
  ulonglong bit_buffer;

  bits= file_buffer.bits & ~7;
  if (bits != BITS_SAVED) {
    bit_buffer= file_buffer.bitbucket >> bits;
    bits= BITS_SAVED - bits;
    while (bits > 0)
    {
      bits-= 8;
      *file_buffer.pos++= (uchar) (bit_buffer >> bits);
    }
    if (file_buffer.pos >= file_buffer.end)
      flush_buffer(~ (ulong) 0);
    file_buffer.bits= BITS_SAVED;
  }
  file_buffer.bitbucket= 0;

[prathamesh04]

Good point, thanks @gkodinov!. I've restructured flush_bits() as you suggested: the flush is now wrapped in if (bits != BITS_SAVED) and file_buffer.bitbucket= 0; runs unconditionally, so it's reset on the no-op branch too. file_buffer.bits stays reset inside the flush path (it is already BITS_SAVED when there is nothing to flush).

Re-verified under a UBSAN build: no runtime error, exit 0, packed data round-trips and CHECK TABLE is OK; maria.aria_pack_mdev14183 passes.

[gkodinov]

FYI: According to our development cycle we work on bugs In the following periods 15.03 - 30.04, 15.06 - 30.07, 15.12 - 31.01 and 15.09 - 30.10. So, please, expect to get a review somewhere between these two dates and the goal is to have your PR merged before the second date