Details
-
Bug
-
Status: Closed (View Workflow)
-
Major
-
Resolution: Fixed
-
10.4.28, 10.3(EOL)
Description
In November 2020, github user Buggynours introduced an if statement that did not previously exist, and branches on field_length >= 0.
if (fp->field_length >= 0) { |
pcf->Length = fp->field_length;
|
|
|
// length is bytes for Connect, not characters |
if (!strnicmp(chset, "utf8", 4)) |
pcf->Length /= 3;
|
|
|
} else { |
pcf->Length= 256; // BLOB? |
However, field_length is defined as uint32_t, and has been for at least 21 years.
Since this variable is unsigned, the if statement will always take the true branch. It will never take the false branch.
- What is the right behaviour here?
- Is the current version correct? ("Always take the true branch" → we should delete the nonsense false branch)
- Or is there some real case where the false branch should be taken but which is currently being ignored? (The false branch comment "BLOB?" strongly suggests yes)
- If so, this is a bug, and it has been merged into 10.2.37+, 10.3.28+, 10.4.18+, 10.5.9+, 10.6., 10.7., etc.
- Or something else?
This was caught by building with -Wtype-limits.
Attachments
Activity
If developers don't have a consistent model of whether field lengths can have negative values or not, then suppressing the warning (by deleting the branch that assumes it can be negative) may be hiding a bug.
That's the part worries me with the fix in cc182aca9352, because I don't understand the intention of having negative value for COLINFO.Length and need further clarification.
> code that sets it to negative values getting written/approved/merged
Where?
I believe Daniel was talking about this line: cc182aca9352
Before 28af4212, the branch relied on a unsigned-to-signed conversion to determine the value of COLINFO.Length: if it became negative, then set it to 256.
But after 28af4212, the value of COLINFO.Length will always be set to the value of fp->field_length and it would potentially set COLINFO.Length to a negative value, and cc182aca9352 keeps this behaviour. We would like to double confirm if it's intentional.
> We would like to double confirm if it's intentional.
I cannot gauge intent any more than anyone else. I, like anyone else with coding ability, can only deeply examine the code paths.
I did see a GetType value returning -1. Maybe that could propagate to Length.
https://github.com/MariaDB/server/pull/2508 seems to be without compile warnings, maybe Windows could detect more.
Please take a review and see what you think. If you want to add some test cases around the other Types not in the case statement that would be good. Changing TYPE_* defines to an enum to give the compile some chance of bound checking would also be a good gain.
danblack wrote:
This does not address the ambiguity or bug reported here. Following that change we have an {{if (unsigned_integer_field_length >= 0) }}, so we still have an impossible "false" branch.
If field lengths are supposed to be unsigned, and therefore cannot have negative values, then why is code that sets it to negative values getting written/approved/merged? What was the actual intent of the field_length < 0 case?
If developers don't have a consistent model of whether field lengths can have negative values or not, then suppressing the warning (by deleting the branch that assumes it can be negative) may be hiding a bug.