MySQL Bugs: #28735: fetch_result

Bug #28735	fetch_result_str bug
Submitted:	29 May 2007 6:58	Modified:	26 Jun 2009 15:10
Reporter:	David Anderson	Email Updates:
Status:	Not a Bug	Impact on me:	None
Category:	MySQL Server: C API (client library)	Severity:	S3 (Non-critical)
Version:	5.0.41	OS:	Any
Assigned to:	[ name withheld ]	CPU Architecture:	Any
Tags:	documentation, prepared statement

Description:
I apologize if this report is wrong or pedantic but I ran into a lot of trouble trying to wrap my head around how MySQL null terminates, and I think I ran into a bug while investigating.

When using mysql_stmt_fetch or mysql_stmt_fetch_column with a MYSQL_BIND::buffer_type of MYSQL_TYPE_STRING, you need to specify a buffer_length, but the docs not specify whether that length should include the null terminator.

Whether you should include a null terminator or not, I believe there is a bug in the "fetch_result_str" function of libmysql.c in libmysql and libmysql_r.  Consider a buffer size being passed whose length is equal to the length of the string MySQL is trying to copy, for example returning "mysql" into a buffer of size 5.

CASE 1: If the null terminator is supposed to be included in buffer_length, "fetch_result_str" will skip copying the NULL terminator because of this line:

  if (copy_length != param->buffer_length)

That is correct, however, it means the buffer won't be terminated, and thus it should be reported as truncated, which this line won't do because the lengths will be equal.

  *param->error= copy_length < length;

CASE 2: If the null terminator is NOT supposed to be included in the buffer size, then this line is wrong:

  if (copy_length != param->buffer_length)

Because a proper buffer size won't be null terminated if the sizes are equal.

How to repeat:
Queries to run first:
DROP TABLE IF EXISTS test_table;
CREATE TABLE test_table (hello varchar(20));
INSERT INTO test_table VALUES ('mysql');

Then see attached C file.  The output for me is:
Actual length: 5 (error: 0) ([6]=61)

As you can see, the error is 0 even though the string was truncated and not null-terminated.  So there is some sort of inconsistency somewhere.  

Suggested fix:
Although this could be addressed by just documenting the behavior, specific fixes are below:

CASE 1 Fix:

Change this line:
   *param->error= copy_length < length;
To this:
   *param->error= copy_length <= length;

Additionally, there is another inconsistency.  The outputted length is reported as the number of bytes not including the null terminator.  This means when a user supplies that length as the new buffer size, it will still be truncated, but not zero terminated.  Since checking length > old_length is done by some applications (such as Perl's MySQL DBD implementation), I think this also needs to be addressed in one of two ways:

1) It should be noted in the documentation that the outputted length does not include the null terminator, and that the user should add 1 to correct the buffer size, or;
2) MySQL should add 1 to the outputted length so the user knows exactly how much to reallocate, and so old comparison tests for reallocation will work.

CASE 2 Fix:

Remove this line:
  if (copy_length != param->buffer_length)

Sample code to reproduce problem

Attachment: mysql_bug_report.c (text/plain), 1.16 KiB.

After writing some code to work around this in my application, I think the suggestion for MySQL to add 1 to the length output for the user is bad and would be inconsistent with common convention -- please disregard it.

Thank you for a problem report. So, from your point of view, is this a bug that should be somehow fixed, or just a request for documenting properly?

It is a bug in both cases; the fix depends on the expected behaviour, which is what I couldn't find in the documentation.

I think this should be changed:
*param->error= copy_length < length;

To be <= instead, and the documentation should say that the null terminator isn't included in the outputted size.

Thanks :)

Sorry for a delay with this bug report.

While anybody can repeat your findings on any version of MySQL, our documentation (http://dev.mysql.com/doc/refman/5.1/en/c-api-prepared-statement-datatypes.html, for example) clearly says that NULL terminator is not included:

"For output value binding, the return value of mysql_stmt_fetch()  determines the interpretation of the length:

    * If mysql_stmt_fetch() returns 0, *length indicates the actual length of the parameter value."

So, application programmer that expects null terminator at the end of the string should just allocate buffer one byte longer. 

To summarize, current behavior (including error code 0 in case like yours) is expected by zillion of programs and should not be changed. There is also no big need to document anything additionally, IMHO.

On the contrary, I just bumped my head on this issue as well, and could not find adequate documentation.

C strings which are not NULL terminated are quite surprising. If the application needs to do something special (add 1? add 1 and NULL terminate after reading?) it would be very helpful to document that specifically.