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

Bug #40492	ignored return codes in memory allocation functions
Submitted:	4 Nov 2008 10:51	Modified:	4 Nov 2008 10:52
Reporter:	Georgi Kodinov	Email Updates:
Status:	Verified	Impact on me:	None
Category:	MySQL Server: General	Severity:	S3 (Non-critical)
Version:	4.1 and up	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
This is a spin-off of bug #25058 to handle the additional data provided by Mark Callaghan.
Here's a  copy of the data provided in bug #25058:

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

How to repeat:
Look at the code

Suggested fix:
use warn_unused_result to identify and then fix all the above places.

For 5.1.46, I count 2500+ warnings for ignored return values from the methods in sql_string.h that fail when memory allocation fails.

The reason risk is high is that some places where we ignore return codes require a change in the execution flow, i.e., since we don't use RAII (http://en.wikipedia.org/wiki/RAII), addition of end: labels with resource cleanup of every function involved. 
Subtle bugs can easily get introduced when adding a jump to a label, not all resources can get cleaned upon error, etc.

High effort since the real fix is to not go over the code and add a bunch of checks, but make sure that sloppy code is not re-introduced. 
This requires a framework for testing the server in an environment that has spurious OOM errors.

Is there any work here that is low risk? I'm thinking that a developer could decide which ones are low risk and do those as a first step, then defer anything else. It won't be perfect but it'll reduce the risk of encountering uncaught failed memory allocations at runtime a bit. Gradual improvement beats none in things like this.