Bug #12925 GET_LL in my_getopt.c is buggy

Description:
While working on bug #12701, I changed some innobase config variables to type longlong, and changed them to use GET_LL in mysqld.cc and upped their maximum values to LONGLONG_MAX. Then I added code to innobase to detect values over 4GB on 32-bit machines and error out. To my surprise that code never triggered. Adding debug prints, I found out no matter what I put to my config file, the values were clamped to 4095MB before being handed to innobase.

Diving into the option handling code revealed the following:

    case GET_INT:
    case GET_UINT:           /* fall through */
      *((int*) result_pos)= (int) getopt_ll(argument, opts, &err);
      break;
    case GET_LONG:
    case GET_ULONG:          /* fall through */
      *((long*) result_pos)= (long) getopt_ll(argument, opts, &err);
      break;
    case GET_LL:
      *((longlong*) result_pos)= getopt_ll(argument, opts, &err);
      break;
    case GET_ULL:
      *((ulonglong*) result_pos)= getopt_ull(argument, opts, &err);
      break;

and getopt_ll:

static longlong getopt_ll(char *arg, const struct my_option *optp, int *err)
{
  longlong num;
  ulonglong block_size= (optp->block_size ? (ulonglong) optp->block_size : 1L);
  
  num= eval_num_suffix(arg, err, (char*) optp->name);
  if (num > 0 && (ulonglong) num > (ulonglong) (ulong) optp->max_value &&
      optp->max_value) /* if max value is not set -> no upper limit */
    num= (longlong) (ulong) optp->max_value;
  num= ((num - (longlong) optp->sub_size) / block_size);
  num= (longlong) (num * block_size);

  return max(num, optp->min_value);
}

The problem is that optp->max_value is cast to ulong, which is wrong for longlong values. Indeed, if I changed the variable to use getopt_ull, which is almost the same but doesn't do the ulong casting, the code worked.

How to repeat:
Change some config variable to type longlong and using GET_LL and having maximum value over 4GB, put that value in the config file as something over 4GB, observe how it's clamped to 4095MB.

Suggested fix:
I could fix GET_LL myself, but I'm not sure why int/uint/long/ulong/longlong are all using the same function (getopt_ll), since it's obviously problematic, so would prefer someone who knows the code to do it.

the double cast

  (ulonglong) (ulong) optp->max_value

clearly looks like some sort of copy/paste bug to me

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/internals/29261

problem is in 4.1, too.

The patch is woefully inadequate. After applying it to my tree and having

innodb_buffer_pool_size=25600M

in my ~/.my.cnf, and adding a printf to getopt_ll so I know what it's returning I get this:

getopt_ll: '25600M', 9223372036853727232

i.e. it's returning optp->max_value - 1 MB, probably because the patch still leaves one ulong cast, in the if condition.

I can't believe the test case added:

DIE_UNLESS(opt_getopt_ll_test > LL(4*1024*1024*1024));

It's checking that the value parsed is bigger than 4GB, but not checking that it's actually the correct value? What's up with that?

Now, if I modify the code like this:

   num= eval_num_suffix(arg, err, (char*) optp->name);
-  if (num > 0 && (ulonglong) num > (ulonglong) (ulong) optp->max_value &&
+  if (num > 0 && (ulonglong) num > (ulonglong) optp->max_value &&
       optp->max_value) /* if max value is not set -> no upper limit */
-    num= (longlong) (ulong) optp->max_value;
+    num= optp->max_value;
   num= ((num - (longlong) optp->sub_size) / block_size);
   num= (longlong) (num * block_size);

it seems to work:

getopt_ll: '25600M', 26843545600

However, I'm not sure what those ulonglong casts are doing there since this is dealing only with values that fit in a longlong, and also, why a max_value of 0 is special-cased as "no max value" instead of simply requiring every config value to supply correct max values. That's a way bigger change than I'm prepared to make, however.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/internals/29677

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/internals/29781

Jim, any ETA on this? This is blocking bug #12701, and proper support for 64-bit Windows would be nice to have.

Fixed in 4.1.16 and 5.0.16.

Noted in 4.1.16, 5.0.16 changelogs.