Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-30713

Bug in field length handling for CONNECT engine

Details

    Description

      In November 2020, github user Buggynours introduced an if statement that did not previously exist, and branches on field_length >= 0.

      https://github.com/MariaDB/server/commit/28af4212b6e2afe1d42729c9c36215ed8a8d38cb#diff-df4382972e2df64f7429b4c4b2037ddeff0c1d6970a5e13b4f98f7995c928cf0

      	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

          Ascending order - Click to sort in descending order
          View 4 older comments
          dlenski Daniel Lenski (Inactive) added a comment -

          danblack wrote:

          UTF correction already applied - 043c1d183088c7179037bb562d1715848fdf9d7e

          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.

          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.

          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.

          dlenski Daniel Lenski (Inactive) added a comment - danblack wrote: UTF correction already applied - 043c1d183088c7179037bb562d1715848fdf9d7e 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. 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. 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.
          cmenghsi Meng-Hsiu Chiang added a comment -

          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.

          cmenghsi Meng-Hsiu Chiang added a comment - 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.
          danblack Daniel Black added a comment -

          > code that sets it to negative values getting written/approved/merged

          Where?

          danblack Daniel Black added a comment - > code that sets it to negative values getting written/approved/merged Where?
          cmenghsi Meng-Hsiu Chiang added a comment - - edited

          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.

          cmenghsi Meng-Hsiu Chiang added a comment - - edited 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.
          danblack Daniel Black added a comment -

          > 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 Daniel Black added a comment - > 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.

          People

            danblack Daniel Black
            cmenghsi Meng-Hsiu Chiang
            Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Git Integration

                Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.