MySQL Bugs: #25058: ignored return codes in memory allocation functions

Bug #25058	ignored return codes in memory allocation functions
Submitted:	14 Dec 2006 4:19	Modified:	12 May 2009 1:51
Reporter:	Sean Pringle	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: General	Severity:	S3 (Non-critical)
Version:	5.0.64, 5.0.30, 4.0.26	OS:	Any
Assigned to:	Alexey Botchkov	CPU Architecture:	Any
Tags:	bfsm_2006_12_21

Description:
Some functions that allocate memory return boolean (or other) codes that are ignored by some callers. For example, if there is a malloc() failure in init_dynamic_array() or insert_dynamic(), the insert operation will fail as the return flag is ignored.

How to repeat:
The following from MySQL 5.0.30 ignore the return value from insert_dynamic():

sp_head.cc : add_instr
sp_pcontext.cc : push_variable, push_cond, push_cursor
sql_select.cc : add_key_part, add_ft_keys, update_ref_and_keys
mi_delete.c : d_search
mi_write.c : _mi_insert
instance_options.cc : add_option
slave.c : add_wild_table_rule

Many callers to String::append ignore the return result.
Callers to Dynamic_array::append might be a potential problem.

Suggested fix:
Simple checks could make memory allocation more robust.

As an example of some of the problems, I used MySQL 4.0.26 and added __attribute__((warn_unused_result)) to many of the declarations for functions that return a value. Most (hopefully all) of these return values should checked. The numbers below are the number of warnings per directory for function calls that ignore the return value.

myisam   44
mysys    19
sql     579 (435 for String::append)

Thank you for a problem report. Verified just as descibed. One just need to review a code to check.

Is there any chance that someone will fix this soon?

Is there an ETA for fixing this?

Can we send you patches with changes to declarations to use warn_unused_result?

Adding warn_unused_result is simple, I don't think that having a patch would make it easier :)

You could just list here all functions that you think should have warn_unused_result.

Besides, I agree that we should start using it.

Warnings from the sql directory. This is the number of ignored return values by called function.
    814 'bool String::append(const char*, uint32)',  G
    470 'bool String::copy(const char*, uint32, CHARSET_INFO*)',  G
    432 'bool String::real_alloc(uint32)',  G
    382 'bool String::append(const char*)',  G
    229 'bool String::append(char)',  G
    219 'bool String::realloc(uint32)',  G
    126 'bool String::copy(const String&)',  G
     41 'bool MYSQL_LOG::write(Log_event*)',  G
     40 'int end_io_cache(IO_CACHE*)',  G
     40 'char* fn_format(char*, const char*, const char*, const char*, uint)',  G
     38 'my_bool _hash_init(HASH*, CHARSET_INFO*, uint, uint, uint, byte* (*)(const byte*, uint*, my_bool), void (*)(void*), uint)',  G
     34 'my_bool init_dynamic_array(DYNAMIC_ARRAY*, uint, uint, uint)',  G
     32 'bool String::append(const String&)',  G
     29 'bool MYSQL_LOG::write(THD*, enum_server_command, const char*, ...)',  G
     25 'bool String::append(const char*, uint32, CHARSET_INFO*)',  G
     24 'int my_delete(const char*, myf)',  G
     19 'my_bool my_hash_insert(HASH*, const byte*)',  G
     19 'my_bool insert_dynamic(DYNAMIC_ARRAY*, char*)',  G
     19 'bool get_field(MEM_ROOT*, Field*, String*)',  G
     19 'bool String::copy(const char*, uint32, CHARSET_INFO*, CHARSET_INFO*, uint*)',  G
     19 'bool String::append_with_prefill(const char*, uint32, uint32, char)',  G
     18 'my_bool reinit_io_cache(IO_CACHE*, cache_type, my_off_t, pbool, pbool)',  G
     18 'bool String::alloc(uint32)',  G
     13 'my_off_t my_seek(File, my_off_t, int, myf)',  G
     12 'int quick_rm_table(db_type, const char*, const char*)',  G
     11 'bool String::copy()',  G
     10 'int closefrm(TABLE*)',  G
      5 'char* convert_dirname(char*, const char*, const char*)',  G
      5 'bool remove_table_from_cache(THD*, const char*, const char*, uint)',  G
      5 'bool load_db_opt_by_name(THD*, const char*, HA_CREATE_INFO*)',  G
      4 'int init_ftfuncs(THD*, SELECT_LEX*, bool)',  G
      4 'bool rm_temporary_table(db_type, char*)',  G
      4 'bool close_temporary_table(THD*, const char*, const char*)',  G
      3 'int String::reserve(uint32)',  G
      3 'int MYSQL_LOG::close_index_file()',  G
      3 'bool mysql_rename_table(db_type, const char*, const char*, const char*, const char*)',  G
      3 'bool MYSQL_LOG::open(const char*, enum_log_type, const char*, cache_type, bool, ulong, bool)',  G
      2 'int my_rename(const char*, const char*, myf)',  G
      2 'int append_query_string(CHARSET_INFO*, const String*, String*)',  G
      2 'int MYSQL_LOG::purge_logs_before_date(time_t)',  G
      2 'int MYSQL_LOG::get_current_log(LOG_INFO*)',  G
     18 'bool String::alloc(uint32)',  G
     13 'my_off_t my_seek(File, my_off_t, int, myf)',  G
     12 'int quick_rm_table(db_type, const char*, const char*)',  G
     11 'bool String::copy()',  G
     10 'int closefrm(TABLE*)',  G
      5 'char* convert_dirname(char*, const char*, const char*)',  G
      5 'bool remove_table_from_cache(THD*, const char*, const char*, uint)',  G
      5 'bool load_db_opt_by_name(THD*, const char*, HA_CREATE_INFO*)',  G
      4 'int init_ftfuncs(THD*, SELECT_LEX*, bool)',  G
      4 'bool rm_temporary_table(db_type, char*)',  G
      4 'bool close_temporary_table(THD*, const char*, const char*)',  G
      3 'int String::reserve(uint32)',  G
      3 'int MYSQL_LOG::close_index_file()',  G
      3 'bool mysql_rename_table(db_type, const char*, const char*, const char*, const char*)',  G
      3 'bool MYSQL_LOG::open(const char*, enum_log_type, const char*, cache_type, bool, ulong, bool)',  G
      2 'int my_rename(const char*, const char*, myf)',  G
      2 'int append_query_string(CHARSET_INFO*, const String*, String*)',  G
      2 'int MYSQL_LOG::purge_logs_before_date(time_t)',  G
      2 'int MYSQL_LOG::get_current_log(LOG_INFO*)',  G
      2 'bool MYSQL_LOG::write(THD*, const char*, uint, time_t, bool)',  G
      1 'uint my_pwrite(File, const byte*, uint, my_off_t, myf)',  G
      1 'my_bool grant_init()',  G
      1 'int my_setwd(const char*, myf)',  G
      1 'int my_realpath(char*, const char*, myf)',  G
      1 'int my_getwd(char*, uint, myf)',  G
      1 'int my_fclose(FILE*, myf)',  G
      1 'int my_chsize(File, my_off_t, int, myf)',  G
      1 'int MYSQL_LOG::raw_get_current_log(LOG_INFO*)',  G
      1 'int MYSQL_LOG::purge_logs(const char*, bool, bool, bool, ulonglong*)',  G
      1 'init_dynamic_array',  G
      1 'bool mysql_rm_db(THD*, char*, bool, bool)',  G
      1 'bool mysql_execute_command(THD*)',  G
      1 'bool mysql_create_db(THD*, char*, HA_CREATE_INFO*, bool)',  G
      1 'bool drop_locked_tables(THD*, const char*, const char*)',  G
      1 'bool dispatch_command(enum_server_command, THD*, char*, uint)',  G
      1 'bool check_access(THD*, ulong, const char*, ulong*, bool, bool, bool)',  G
      1 'bool MYSQL_LOG::open_update_log(const char*)',  G
      1 'bool MYSQL_LOG::open_slow_log(const char*)',  G
      1 'bool MYSQL_LOG::open_query_log(const char*)',  G
      1 'bool MYSQL_LOG::open_audit_log(const char*)',  G
      1 'bool MYSQL_LOG::flush_and_sync()',  G

Counts for ignored return values in the mysys directory
      6 'my_delete',  G
      5 'my_fclose',  G
      3 'my_b_flush_io_cache',  G
      3 'convert_dirname',  G
      2 'my_realpath',  G
      2 'fn_format',  G
      1 'my_rename',  G
      1 'my_readlink',  G
      1 'my_fseek',  G
      1 'fn_same',  G
      1 'end_io_cache',  G

And for the myisam directory. For all of these, I changed sql_class.h, my_sys.h, mysql_priv.h, sql_acl.h, sql_string.h and hash.h. Many headers were not edited, so there are many ignored return values not reported.

     23 'fn_format',  G
     12 'end_io_cache',  G
      9 'my_delete',  G
      7 'my_write',  G
      7 'my_seek',  G
      6 'reinit_io_cache',  G
      5 'my_lock',  G
      2 'my_delete_with_symlink',  G
      2 'insert_dynamic',  G
      2 'init_io_cache',  G
      1 'my_realpath',  G
      1 'my_fclose',  G
      1 'my_copystat',  G
      1 'my_chsize',  G
      1 'init_dynamic_array',  G

Created a bug #40492 to handle the additional report by Mark Callaghan.

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/commits/59552

2720 Alexey Botchkov	2008-11-21
      Bug#25058 ignored return codes in memory allocation functions
         memory allocation error checks added for functions
         calling insert_dynamic()
      
      per-file messages:
        myisam/mi_delete.c
      Bug#25058 ignored return codes in memory allocation functions
          out-of-memory errors handled
        myisam/mi_write.c
      Bug#25058 ignored return codes in memory allocation functions
          out-of-memory errors handled
        server-tools/instance-manager/instance_options.cc
      Bug#25058 ignored return codes in memory allocation functions
          out-of-memory errors handled
        sql/slave.cc
      Bug#25058 ignored return codes in memory allocation functions
          out-of-memory errors handled
        sql/sp_head.cc
      Bug#25058 ignored return codes in memory allocation functions
          out-of-memory errors handled
        sql/sp_head.h
      Bug#25058 ignored return codes in memory allocation functions
          out-of-memory errors handled
        sql/sp_pcontext.cc
      Bug#25058 ignored return codes in memory allocation functions
          out-of-memory errors handled
        sql/sp_pcontext.h
      Bug#25058 ignored return codes in memory allocation functions
          out-of-memory errors handled
        sql/sql_select.cc
      Bug#25058 ignored return codes in memory allocation functions
          out-of-memory errors handled
        sql/sql_yacc.yy
      Bug#25058 ignored return codes in memory allocation functions
          out-of-memory errors handled

Pushed into 5.0.82 (revid:davi.arnaut@sun.com-20090505184158-dvmedh8n472y8np5) (version source revid:davi.arnaut@sun.com-20090505184158-dvmedh8n472y8np5) (merge vers: 5.0.82) (pib:6)

Pushed into 5.1.35 (revid:davi.arnaut@sun.com-20090505190206-9xmh7dlc6kom8exp) (version source revid:davi.arnaut@sun.com-20090505190206-9xmh7dlc6kom8exp) (merge vers: 5.1.35) (pib:6)

Pushed into 6.0.12-alpha (revid:svoj@sun.com-20090506125450-yokcmvqf2g7jhujq) (version source revid:holyfoot@mysql.com-20090429035014-0eqarsso851hl65i) (merge vers: 6.0.11-alpha) (pib:6)

Noted in 5.0.82, 5.1.35, 6.0.12 changelogs.

Several memory allocation functions were not being checked for
out-of-memory return values.

Pushed into 5.1.35-ndb-6.3.26 (revid:jonas@mysql.com-20090615074202-0r5r2jmi83tww6sf) (version source revid:jonas@mysql.com-20090615070837-9pccutgc7repvb4d) (merge vers: 5.1.35-ndb-6.3.26) (pib:6)

Pushed into 5.1.35-ndb-7.0.7 (revid:jonas@mysql.com-20090615074335-9hcltksp5cu5fucn) (version source revid:jonas@mysql.com-20090615072714-rmfkvrbbipd9r32c) (merge vers: 5.1.35-ndb-7.0.7) (pib:6)

Pushed into 5.1.35-ndb-6.2.19 (revid:jonas@mysql.com-20090615061520-sq7ds4yw299ggugm) (version source revid:jonas@mysql.com-20090615054654-ebgpz7elwu1xj36j) (merge vers: 5.1.35-ndb-6.2.19) (pib:6)

Is this bug for real? For instance, what is the point of returning bool from add_ft_keys(), if we pre-allocate a KEYUSE array of such size so that its reallocation is never necessary?

Moreover, failure to add a KEYUSE element will not cause a wrong query result. it will only cause the certain way to do 'ref' access not to be considered.

It's for real but some cases might be shown by analysis to be impossible.

If it's impossible for add_ft_keys() to ever be called with a pre-allocated KEYUSE array that requires reallocation then the return code might be redundant. If it's not impossible then some way to handle the case is needed. If add_ft_keys() is believed never to need a return code, then maybe it shouldn't have a return code but should fail with an error if the impossible happens.

Not considering some ref methods due to a failure to allocate RAM would need an error message in the error log to alert the DBA that their server was configured to use too much RAM.

Mark is trying to find all of the ways in which the server can fail in strange ways when memory is limited and get some predictable handling of them in place instead. Even an assert is better than a random failure without cause.