From 6fe882cd85aee487c0534e6399ebd16c1ad2cab4 Mon Sep 17 00:00:00 2001 From: Eric Herman Date: Thu, 19 Jan 2023 17:53:51 +0100 Subject: Add my_afree after my_alloca in early return case The code already had a call to `my_afree` in the normal return case, but failed to do so in the early return case. --- client/mysql.cc | 3 +++ 1 file changed, 3 insertions(+) (limited to 'client') diff --git a/client/mysql.cc b/client/mysql.cc index 4d14dc0f5f6..d53d4f304e9 100644 --- a/client/mysql.cc +++ b/client/mysql.cc @@ -3598,7 +3598,10 @@ print_table_data(MYSQL_RES *result) { print_field_types(result); if (!mysql_num_rows(result)) + { + my_afree((uchar*) num_flag); return; + } mysql_field_seek(result,0); } separator.copy("+",1,charset_info); -- cgit v1.2.1 From 567b68129943a1cceab1d7b4c68e2a4ba011cdc0 Mon Sep 17 00:00:00 2001 From: Mikhail Chalov Date: Tue, 15 Nov 2022 14:39:30 -0800 Subject: Minimize unsafe C functions usage - replace strcat() and strcpy() (and strncat() and strncpy()) with custom safe_strcat() and safe_strcpy() functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MariaDB code base uses strcat() and strcpy() in several places. These are known to have memory safety issues and their usage is discouraged. Common security scanners like Flawfinder flags them. In MariaDB we should start using modern and safer variants on these functions. This is similar to memory issues fixes in 19af1890b56c6c147c296479bb6a4ad00fa59dbb and 9de9f105b5cb88249acc39af73d32af337d6fd5f but now replace use of strcat() and strcpy() with safer options strncat() and strncpy(). However, add '\0' forcefully to make sure the result string is correct since for these two functions it is not guaranteed what new string will be null-terminated. Example: size_t dest_len = sizeof(g->Message); strncpy(g->Message, "Null json tree", dest_len); strncat(g->Message, ":", sizeof(g->Message) - strlen(g->Message)); size_t wrote_sz = strlen(g->Message); size_t cur_len = wrote_sz >= dest_len ? dest_len - 1 : wrote_sz; g->Message[cur_len] = '\0'; All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services -- Reviewer and co-author Vicențiu Ciorbaru -- Reviewer additions: * The initial function implementation was flawed. Replaced with a simpler and also correct version. * Simplified code by making use of snprintf instead of chaining strcat. * Simplified code by removing dynamic string construction in the first place and using static strings if possible. See connect storage engine changes. --- client/mysql_plugin.c | 84 ++++++++++++++++++++++++--------------------------- client/mysqldump.c | 2 +- client/mysqltest.cc | 15 +++++---- 3 files changed, 50 insertions(+), 51 deletions(-) (limited to 'client') diff --git a/client/mysql_plugin.c b/client/mysql_plugin.c index f496db4e72b..d87d4269f89 100644 --- a/client/mysql_plugin.c +++ b/client/mysql_plugin.c @@ -569,14 +569,14 @@ static int file_exists(char * filename) @retval int error = 1, success = 0 */ -static int search_dir(const char * base_path, const char *tool_name, +static int search_dir(const char *base_path, const char *tool_name, const char *subdir, char *tool_path) { char new_path[FN_REFLEN]; char source_path[FN_REFLEN]; - strcpy(source_path, base_path); - strcat(source_path, subdir); + safe_strcpy(source_path, sizeof(source_path), base_path); + safe_strcat(source_path, sizeof(source_path), subdir); fn_format(new_path, tool_name, source_path, "", MY_UNPACK_FILENAME); if (file_exists(new_path)) { @@ -632,7 +632,7 @@ static int load_plugin_data(char *plugin_name, char *config_file) FILE *file_ptr; char path[FN_REFLEN]; char line[1024]; - char *reason= 0; + const char *reason= 0; char *res; int i= -1; @@ -643,14 +643,14 @@ static int load_plugin_data(char *plugin_name, char *config_file) } if (!file_exists(opt_plugin_ini)) { - reason= (char *)"File does not exist."; + reason= "File does not exist."; goto error; } file_ptr= fopen(opt_plugin_ini, "r"); if (file_ptr == NULL) { - reason= (char *)"Cannot open file."; + reason= "Cannot open file."; goto error; } @@ -660,17 +660,20 @@ static int load_plugin_data(char *plugin_name, char *config_file) /* Read plugin components */ while (i < 16) { + size_t line_len; + res= fgets(line, sizeof(line), file_ptr); + line_len= strlen(line); + /* strip /n */ - if (line[strlen(line)-1] == '\n') - { - line[strlen(line)-1]= '\0'; - } + if (line[line_len - 1] == '\n') + line[line_len - 1]= '\0'; + if (res == NULL) { if (i < 1) { - reason= (char *)"Bad format in plugin configuration file."; + reason= "Bad format in plugin configuration file."; fclose(file_ptr); goto error; } @@ -683,14 +686,19 @@ static int load_plugin_data(char *plugin_name, char *config_file) if (i == -1) /* if first pass, read this line as so_name */ { /* Add proper file extension for soname */ - strcat(line, FN_SOEXT); + if (safe_strcpy(line + line_len - 1, sizeof(line), FN_SOEXT)) + { + reason= "Plugin name too long."; + fclose(file_ptr); + goto error; + } /* save so_name */ plugin_data.so_name= my_strdup(line, MYF(MY_WME|MY_ZEROFILL)); i++; } else { - if (strlen(line) > 0) + if (line_len > 0) { plugin_data.components[i]= my_strdup(line, MYF(MY_WME)); i++; @@ -779,14 +787,13 @@ static int check_options(int argc, char **argv, char *operation) /* read the plugin config file and check for match against argument */ else { - if (strlen(argv[i]) + 4 + 1 > FN_REFLEN) + if (safe_strcpy(plugin_name, sizeof(plugin_name), argv[i]) || + safe_strcpy(config_file, sizeof(config_file), argv[i]) || + safe_strcat(config_file, sizeof(config_file), ".ini")) { fprintf(stderr, "ERROR: argument is too long.\n"); return 1; } - strcpy(plugin_name, argv[i]); - strcpy(config_file, argv[i]); - strcat(config_file, ".ini"); } } @@ -855,35 +862,30 @@ static int check_options(int argc, char **argv, char *operation) static int process_options(int argc, char *argv[], char *operation) { int error= 0; - int i= 0; /* Parse and execute command-line options */ if ((error= handle_options(&argc, &argv, my_long_options, get_one_option))) - goto exit; + return error; /* If the print defaults option used, exit. */ if (opt_print_defaults) - { - error= -1; - goto exit; - } + return -1; /* Add a trailing directory separator if not present */ if (opt_basedir) { - i= (int)strlength(opt_basedir); - if (opt_basedir[i-1] != FN_LIBCHAR || opt_basedir[i-1] != FN_LIBCHAR2) + size_t basedir_len= strlength(opt_basedir); + if (opt_basedir[basedir_len - 1] != FN_LIBCHAR || + opt_basedir[basedir_len - 1] != FN_LIBCHAR2) { char buff[FN_REFLEN]; - memset(buff, 0, sizeof(buff)); + if (basedir_len + 2 > FN_REFLEN) + return -1; - strncpy(buff, opt_basedir, sizeof(buff) - 1); -#ifdef __WIN__ - strncat(buff, "/", sizeof(buff) - strlen(buff) - 1); -#else - strncat(buff, FN_DIRSEP, sizeof(buff) - strlen(buff) - 1); -#endif - buff[sizeof(buff) - 1]= 0; + memcpy(buff, opt_basedir, basedir_len); + buff[basedir_len]= '/'; + buff[basedir_len + 1]= '\0'; + my_free(opt_basedir); opt_basedir= my_strdup(buff, MYF(MY_FAE)); } @@ -895,10 +897,7 @@ static int process_options(int argc, char *argv[], char *operation) generated when the defaults were read from the file, exit. */ if (!opt_no_defaults && ((error= get_default_values()))) - { - error= -1; - goto exit; - } + return -1; /* Check to ensure required options are present and validate the operation. @@ -906,11 +905,9 @@ static int process_options(int argc, char *argv[], char *operation) read a configuration file named .ini from the --plugin-dir or --plugin-ini location if the --plugin-ini option presented. */ - strcpy(operation, ""); - if ((error = check_options(argc, argv, operation))) - { - goto exit; - } + operation[0]= '\0'; + if ((error= check_options(argc, argv, operation))) + return error; if (opt_verbose) { @@ -922,8 +919,7 @@ static int process_options(int argc, char *argv[], char *operation) printf("# lc_messages_dir = %s\n", opt_lc_messages_dir); } -exit: - return error; + return 0; } diff --git a/client/mysqldump.c b/client/mysqldump.c index 294f29b7acd..b382dd41d08 100644 --- a/client/mysqldump.c +++ b/client/mysqldump.c @@ -2478,7 +2478,7 @@ static uint dump_events_for_db(char *db) if (mysql_query_with_error_report(mysql, &event_list_res, "show events")) DBUG_RETURN(0); - strcpy(delimiter, ";"); + safe_strcpy(delimiter, sizeof(delimiter), ";"); if (mysql_num_rows(event_list_res) > 0) { if (opt_xml) diff --git a/client/mysqltest.cc b/client/mysqltest.cc index 61bb3822d36..e0bd37dd934 100644 --- a/client/mysqltest.cc +++ b/client/mysqltest.cc @@ -6171,7 +6171,9 @@ int do_done(struct st_command *command) if (*cur_block->delim) { /* Restore "old" delimiter after false if block */ - strcpy (delimiter, cur_block->delim); + if (safe_strcpy(delimiter, sizeof(delimiter), cur_block->delim)) + die("Delimiter too long, truncated"); + delimiter_length= strlen(delimiter); } /* Pop block from stack, goto next line */ @@ -6426,10 +6428,12 @@ void do_block(enum block_cmd cmd, struct st_command* command) if (cur_block->ok) { cur_block->delim[0]= '\0'; - } else + } + else { /* Remember "old" delimiter if entering a false if block */ - strcpy (cur_block->delim, delimiter); + if (safe_strcpy(cur_block->delim, sizeof(cur_block->delim), delimiter)) + die("Delimiter too long, truncated"); } DBUG_PRINT("info", ("OK: %d", cur_block->ok)); @@ -11301,9 +11305,8 @@ static int setenv(const char *name, const char *value, int overwrite) char *envvar= (char *)malloc(buflen); if(!envvar) return ENOMEM; - strcpy(envvar, name); - strcat(envvar, "="); - strcat(envvar, value); + + snprintf(envvar, buflen, "%s=%s", name, value); putenv(envvar); return 0; } -- cgit v1.2.1