[MDEV-30713] Bug in field length handling for CONNECT engine Created: 2023-02-23 Updated: 2023-04-21 Resolved: 2023-04-21 |
|
| Status: | Closed |
| Project: | MariaDB Server |
| Component/s: | Storage Engine - Connect |
| Affects Version/s: | 10.3, 10.4.28 |
| Fix Version/s: | 10.4.29 |
| Type: | Bug | Priority: | Major |
| Reporter: | Meng-Hsiu Chiang | Assignee: | Daniel Black |
| Resolution: | Fixed | Votes: | 1 |
| Labels: | Connect-Engine, JSON, beginner-friendly, not-11.0 | ||
| Description |
|
In November 2020, github user Buggynours introduced an if statement that did not previously exist, and branches on field_length >= 0.
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.
This was caught by building with -Wtype-limits. |
| Comments |
| Comment by Daniel Lenski [ 2023-02-23 ] |
|
https://github.com/MariaDB/server/commit/28af4212b6e2afe1d42729c9c36215ed8a8d38cb#r101721201 |
| Comment by Daniel Black [ 2023-02-23 ] |
|
Monty did a cc182aca9352 that wasn't backported that just assumed the always true branch is correct. So just delete the nonsense false branch in 10.4. UTF correction already applied - 043c1d183088c7179037bb562d1715848fdf9d7e / |
| Comment by Meng-Hsiu Chiang [ 2023-02-23 ] |
|
But in storage/connect/catalog.h#L42 pcf->Length is an int and cc182aca9352 will cast it to a negative in some cases, is it intended? |
| Comment by Daniel Black [ 2023-02-23 ] |
|
I think we can safely assume that none of the various flaws that cause compile warnings are intended. So sure change COLINFO.Length, which will probably cascade to GetTypeSize too, but quickly looking, that seems to be as far as it goes. |
| Comment by Daniel Lenski [ 2023-02-23 ] |
|
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. |
| Comment by Meng-Hsiu Chiang [ 2023-02-23 ] |
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. |
| Comment by Daniel Black [ 2023-02-23 ] |
|
> code that sets it to negative values getting written/approved/merged Where? |
| Comment by Meng-Hsiu Chiang [ 2023-02-27 ] |
|
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. |
| Comment by Daniel Black [ 2023-02-28 ] |
|
> 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. |