Bug #33581 misplaced break statement in switch...case in bool udf_handler::get_arguments()

• Submitted: 29 Dec 2007 11:15
• Modified: 29 Dec 2007 15:45
• Reporter: Roland Bouman
• Status: Verified
• Category: MySQL Server: User-defined functions ( UDF )
• Severity: S3 (Non-critical)
• Version: 5.1.23bk
• OS: Any
• CPU Architecture: Any

[29 Dec 2007 11:15] Roland Bouman

Description:
The method get_arguments() of the udf_handler class defined in sql/item_func.cc contains a switch...case statement with a misplaced break. 

Under particular circumstances, this misplaced break could cause the case to 'fall through' which would result in an assert in debug builds. For now, the code seems safe even if it does fall through, but it should be fixed none the less. 

Alternatively, if it is not a bug but the break placed there with a reason, a clear comment must be provided to explain.

How to repeat:
file: sql/item_func.cc
method: bool udf_handler::get_arguments()
lines: 2993..3002, break statement at ln 3000

    switch (f_args.arg_type[i]) {
    case STRING_RESULT:
    case DECIMAL_RESULT:
      {
	String *res=args[i]->val_str(&buffers[str_count++]);
	if (!(args[i]->null_value))
	{
	  f_args.args[i]=    (char*) res->ptr();
	  f_args.lengths[i]= res->length();
	  break;
	}
      }
    case INT_RESULT:
    ...
    }

Suggested fix:
The break statement at line 3000 should appear after the end of block at line 3002. The case should be:

    case STRING_RESULT:
    case DECIMAL_RESULT:
      {
	String *res=args[i]->val_str(&buffers[str_count++]);
	if (!(args[i]->null_value))
	{
	  f_args.args[i]=    (char*) res->ptr();
	  f_args.lengths[i]= res->length();
	} //removed break from here
      }
      break; //break moved to here
    //next case please

[29 Dec 2007 15:45] Hartmut Holzgraefe

I think it is wrongly placed, too.

I also fail to understand why the various branches call the ->val_*() methods *before* checking null_value although the val_*() results are *only* used within the "if (!null_value)" branches ...

[4 Jan 2008 16:20] Roland Bouman

Lars! thanks for verifying.

I think that the call to val occurs before the null check because the val check has the side effect of setting the null attribute.

(but maybe I'm wrong - not that into C yet)