MDEV-40176: field_charset()->charpos(blob..) called with NULL#5304
MDEV-40176: field_charset()->charpos(blob..) called with NULL#5304grooverdan wants to merge 1 commit into
Conversation
Problem: When `my_charpos_mb()` is called by `Field_blob::get_key_image_itRAW` with a start/end both being NULL. Because of this the blob_length must have been 0. Fix: Rather than relying on character set functions to calculate the storage of nothing, bypass the calculation as the charpos() isn't going to return a value less than 0 (the blob_length).
There was a problem hiding this comment.
Code Review
This pull request introduces a null check for the blob pointer in Field_blob::get_key_image_itRAW before calculating the character length. The reviewer suggested checking if (blob_length > 0) instead of if (blob) to optimize performance for empty BLOBs and prevent passing NULL pointers to charpos.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (blob) | ||
| { | ||
| size_t local_char_length= length / mbmaxlen(); | ||
| local_char_length= field_charset()->charpos(blob, blob + blob_length, | ||
| local_char_length); | ||
| set_if_smaller(blob_length, local_char_length); | ||
| } |
There was a problem hiding this comment.
Instead of checking if (blob), it is more efficient and robust to check if (blob_length > 0). If the BLOB length is 0, there is no need to perform any character position calculations or division, as blob_length is already 0 and cannot be reduced further. This avoids redundant overhead for empty BLOBs and safely prevents passing NULL pointers to charpos.
if (blob_length > 0)
{
size_t local_char_length= length / mbmaxlen();
local_char_length= field_charset()->charpos(blob, blob + blob_length,
local_char_length);
set_if_smaller(blob_length, local_char_length);
}There was a problem hiding this comment.
bit clearer. Could DBUG_ASSERT(blob) in the block too.
| local_char_length= field_charset()->charpos(blob, blob + blob_length, | ||
| local_char_length); | ||
| set_if_smaller(blob_length, local_char_length); | ||
| if (blob) |
There was a problem hiding this comment.
I believe this is not the only place where we call charpos function and if we were to do this kind of checks, we should be doing it in all other places too.
Fixing the root cause, i.e my_charpos_* set of functions to handle null seems like correct fix to me. thoughts @grooverdan ?
|
@grooverdan , caller-side guard isn't wrong, it would stop the crash but it still leaves the actual UBSAN. |
Problem:
When
my_charpos_mb()is called byField_blob::get_key_image_itRAWwith a start/end both being NULL. Because of this the blob_length
must have been 0.
Fix:
Rather than relying on character set functions to calculate the
storage of nothing, bypass the calculation as the charpos() isn't
going to return a value less than 0 (the blob_length).