MySQL Bugs: #57531: mysql_close() shouldn't fall if MYSQL hasn't been initialized

Bug #57531	mysql_close() shouldn't fall if MYSQL hasn't been initialized
Submitted:	18 Oct 2010 16:08	Modified:	18 Oct 2010 20:02
Reporter:	Anatoly Lyutin	Email Updates:
Status:	Not a Bug	Impact on me:	None
Category:	MySQL Server: C API (client library)	Severity:	S3 (Non-critical)
Version:	5.0.89	OS:	Any
Assigned to:		CPU Architecture:	Any
Tags:	mysql_close

Description:
mysql_close() falls If MYSQL structure hasn`t been initialized. 

Now mysql falls with 

*** glibc has detected an error in ./a.exe: free(): invalid pointer: 0xbf8e0002 ***
======= Backtrace: =========
/lib/libc.so.6(+0x699ea)[0xb740a9ea]
/lib/libc.so.6(+0x6aeb8)[0xb740beb8]
/lib/libc.so.6(cfree+0x6d)[0xb740e93d]
/usr/lib/libmysqlclient.so.16(my_no_flags_free+0x1f)[0xb753dbcf]
/lib/libc.so.6(__libc_start_main+0xe6)[0xb73b7c66]
./a.exe[0x80484c1]
======= Memory map: ========
08048000-08049000 r-xp 00000000 00:10 629577     /tmp/a.exe

It is hard to find a problem with this backtrace.

How to repeat:
#include <mysql/mysql.h>

int main()
{
    MYSQL m;

    mysql_close(&m);
    return 0;
}

Suggested fix:
should check up MYSQL structure on garbage on its fileds.

Suggested fix:
I think that mysql_close() should check up MYSQL structure on garbage on its fields.

Our manual (http://dev.mysql.com/doc/refman/5.1/en/mysql-close.html) says: 

"Closes a previously opened connection."

So there is a hint of kind... But I agree, it would be nice to have a way to distinguish non-initialized structure from those properly initialized. Checksum field or something like that.

This is not possible under the current rules of the C language. In the given example, the variable value is uninitialized, therefore its not possible to determine whether the contents (state) of the variable are valid or not.

http://en.wikipedia.org/wiki/Uninitialized_variable

As MySQL is defined as structure in mysql.h:

typedef struct st_mysql
{
  NET           net;                    /* Communication parameters */
  unsigned char *connector_fd;          /* ConnectorFd for SSL */
  char          *host,*user,*passwd,*unix_socket,*server_version,*host_info;
  char          *info, *db;
  struct charset_info_st *charset;
  MYSQL_FIELD   *fields;
  MEM_ROOT      field_alloc;
  my_ulonglong affected_rows;
  my_ulonglong insert_id;               /* id if insert on table with NEXTNR */
  my_ulonglong extra_info;              /* Not used */
  unsigned long thread_id;              /* Id for connection in server */
  unsigned long packet_length;
  unsigned int  port;
  unsigned long client_flag,server_capabilities;
  unsigned int  protocol_version;
  unsigned int  field_count;
  unsigned int  server_status;
  unsigned int  server_language;
  unsigned int  warning_count;
  struct st_mysql_options options;
  enum mysql_status status;
  my_bool       free_me;                /* If free in mysql_close */
  my_bool       reconnect;              /* set to 1 if automatic reconnect */

  /* session-wide random string */
  char          scramble[SCRAMBLE_LENGTH+1];
  my_bool unused1;
  void *unused2, *unused3, *unused4, *unused5;

  LIST  *stmts;                     /* list of all statements */
  const struct st_mysql_methods *methods;
  void *thd;
  /*
    Points to boolean flag in MYSQL_RES  or MYSQL_STMT. We set this flag
    from mysql_stmt_close if close had to cancel result set of this object.
  */
  my_bool *unbuffered_fetch_owner;
  /* needed for embedded server - no net buffer to store the 'info' */
  char *info_buffer;
  void *extension;
} MYSQL;

what is the problem, theoretically, to add last item to the structure with, say, MD5 digest of previous items that will be correct if the structure is initialized? 

This will not catch 100% of uninitialized structure cases, theoretically, but most of them will be easy to identify.

Reading uninitialized variables is undefined behavior under the current rules of the C language.

http://en.wikipedia.org/wiki/Undefined_behavior

Also, as mentioned in one of the links, there are tools that detect this kind of error. For GCC, there is mudflap. For Windows, there is Microsoft Application Verifier and others.

OK, but what about cases when MYSQL structure is allocated dynamically or declared as static?

When dynamically allocated, same rules applies and valgrind can detect this case. When declared as static, the structure will be zero initialized, mysql_close will probably do nothing. Anyway, mysql_close should only be called upon a handle that was initialized using mysql_init.

I have catched this to a bug on a code similar:

int main()
{
   MYSQL m;
   
   if (!my_init(&m)) //initialize MYSQL and connet to DB
       goto failed;

   ....
   some code
   ...

   char *str = "SOME SQL QUERY;"
   if (mysql_query(&m, str))
     goto failed;

   ....
   some code
   ....
   
failed:
   mysql_close(&m);

return 0;
}

I think that very much it is not convenient that it is impossible to check up that MYSQL is initialized correctly. Also, I know that C language has no mechanism for check of correctness of initialization of structure`s fields.

But it would be healthy, if mysql_close() checked somehow that MYSQL was used for work and has been initialized.