Commit 7bdd878a authored by Hugo Wen's avatar Hugo Wen Committed by Daniel Black

Fix few vulnerabilities found by Cppcheck

While performing SAST scanning using Cppcheck against source code of
commit 81196469, several code vulnerabilities were found.

Fix following issues:

1. Parameters of `snprintf` function are incorrect.

   Cppcheck error:

       client/mysql_plugin.c:1228: error: snprintf format string requires 6 parameters but only 5 are given.

   It is due to commit 630d7229 introduced option `--lc-messages-dir`
   in the bootstrap command. However the parameter was not even given
   in the `snprintf` after changing the format string.

   Fix:
   Restructure the code logic and correct the function parameters for
   `snprintf`.

2. Null pointer is used in a `snprintf` which could cause a crash.

   Cppcheck error:

       extra/mariabackup/xbcloud.cc:2534: error: Null pointer dereference

   The code intended to print the swift_project name, if the
   opt_swift_project_id is NULL but opt_swift_project is not NULL.
   However the parameter of `snprintf` was mistakenly using
   `opt_swift_project_id`.

   Fix:
   Change to use the correct string from `opt_swift_project`.

3. Potential double release of a memory

   Cppcheck error:

       plugin/auth_pam/testing/pam_mariadb_mtr.c:69: error: Memory pointed to by 'resp' is freed twice.

   A pointer `resp` is reused and allocated new memory after it has been
   freed. However, `resp` was not set to NULL after freed.
   Potential double release of the same pointer if the call back
   function doesn't allocate new memory for `resp` pointer.

   Fix:
   Set the `resp` pointer to NULL after the first free() to make sure
   the same address is not freed twice.

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, Inc.
parent acfb5dfd
...@@ -102,7 +102,7 @@ int main(int argc,char *argv[]) ...@@ -102,7 +102,7 @@ int main(int argc,char *argv[])
MY_INIT(argv[0]); MY_INIT(argv[0]);
sf_leaking_memory=1; /* don't report memory leaks on early exits */ sf_leaking_memory=1; /* don't report memory leaks on early exits */
plugin_data.name= 0; /* initialize name */ plugin_data.name= 0; /* initialize name */
/* /*
The following operations comprise the method for enabling or disabling The following operations comprise the method for enabling or disabling
a plugin. We begin by processing the command options then check the a plugin. We begin by processing the command options then check the
...@@ -110,15 +110,15 @@ int main(int argc,char *argv[]) ...@@ -110,15 +110,15 @@ int main(int argc,char *argv[])
--plugin-ini (if specified). If the directories are Ok, we then look --plugin-ini (if specified). If the directories are Ok, we then look
for the mysqld executable and the plugin soname. Finally, we build a for the mysqld executable and the plugin soname. Finally, we build a
bootstrap command file for use in bootstraping the server. bootstrap command file for use in bootstraping the server.
If any step fails, the method issues an error message and the tool exits. If any step fails, the method issues an error message and the tool exits.
1) Parse, execute, and verify command options. 1) Parse, execute, and verify command options.
2) Check access to directories. 2) Check access to directories.
3) Look for mysqld executable. 3) Look for mysqld executable.
4) Look for the plugin. 4) Look for the plugin.
5) Build a bootstrap file with commands to enable or disable plugin. 5) Build a bootstrap file with commands to enable or disable plugin.
*/ */
if ((error= process_options(argc, argv, operation)) || if ((error= process_options(argc, argv, operation)) ||
(error= check_access()) || (error= check_access()) ||
...@@ -126,11 +126,11 @@ int main(int argc,char *argv[]) ...@@ -126,11 +126,11 @@ int main(int argc,char *argv[])
(error= find_plugin(tp_path)) || (error= find_plugin(tp_path)) ||
(error= build_bootstrap_file(operation, bootstrap))) (error= build_bootstrap_file(operation, bootstrap)))
goto exit; goto exit;
/* Dump the bootstrap file if --verbose specified. */ /* Dump the bootstrap file if --verbose specified. */
if (opt_verbose && ((error= dump_bootstrap_file(bootstrap)))) if (opt_verbose && ((error= dump_bootstrap_file(bootstrap))))
goto exit; goto exit;
/* Start the server in bootstrap mode and execute bootstrap commands */ /* Start the server in bootstrap mode and execute bootstrap commands */
error= bootstrap_server(server_path, bootstrap); error= bootstrap_server(server_path, bootstrap);
...@@ -238,7 +238,7 @@ static int run_command(char* cmd, const char *mode) ...@@ -238,7 +238,7 @@ static int run_command(char* cmd, const char *mode)
#ifdef __WIN__ #ifdef __WIN__
/** /**
Check to see if there are spaces in a path. Check to see if there are spaces in a path.
@param[in] path The Windows path to examine. @param[in] path The Windows path to examine.
@retval int spaces found = 1, no spaces = 0 @retval int spaces found = 1, no spaces = 0
...@@ -253,7 +253,7 @@ static int has_spaces(const char *path) ...@@ -253,7 +253,7 @@ static int has_spaces(const char *path)
/** /**
Convert a Unix path to a Windows path. Convert a Unix path to a Windows path.
@param[in] path The Windows path to examine. @param[in] path The Windows path to examine.
@returns string containing path with / changed to \\ @returns string containing path with / changed to \\
...@@ -335,12 +335,12 @@ static int get_default_values() ...@@ -335,12 +335,12 @@ static int get_default_values()
#ifdef __WIN__ #ifdef __WIN__
{ {
char *format_str= 0; char *format_str= 0;
if (has_spaces(tool_path) || has_spaces(defaults_file)) if (has_spaces(tool_path) || has_spaces(defaults_file))
format_str = "\"%s --mysqld > %s\""; format_str = "\"%s --mysqld > %s\"";
else else
format_str = "%s --mysqld > %s"; format_str = "%s --mysqld > %s";
snprintf(defaults_cmd, sizeof(defaults_cmd), format_str, snprintf(defaults_cmd, sizeof(defaults_cmd), format_str,
add_quotes(tool_path), add_quotes(defaults_file)); add_quotes(tool_path), add_quotes(defaults_file));
if (opt_verbose) if (opt_verbose)
...@@ -675,7 +675,7 @@ static int load_plugin_data(char *plugin_name, char *config_file) ...@@ -675,7 +675,7 @@ static int load_plugin_data(char *plugin_name, char *config_file)
{ {
reason= "Bad format in plugin configuration file."; reason= "Bad format in plugin configuration file.";
fclose(file_ptr); fclose(file_ptr);
goto error; goto error;
} }
break; break;
} }
...@@ -709,7 +709,7 @@ static int load_plugin_data(char *plugin_name, char *config_file) ...@@ -709,7 +709,7 @@ static int load_plugin_data(char *plugin_name, char *config_file)
} }
} }
} }
fclose(file_ptr); fclose(file_ptr);
return 0; return 0;
...@@ -740,7 +740,7 @@ static int check_options(int argc, char **argv, char *operation) ...@@ -740,7 +740,7 @@ static int check_options(int argc, char **argv, char *operation)
int num_found= 0; /* number of options found (shortcut loop) */ int num_found= 0; /* number of options found (shortcut loop) */
char config_file[FN_REFLEN]; /* configuration file name */ char config_file[FN_REFLEN]; /* configuration file name */
char plugin_name[FN_REFLEN]; /* plugin name */ char plugin_name[FN_REFLEN]; /* plugin name */
/* Form prefix strings for the options. */ /* Form prefix strings for the options. */
const char *basedir_prefix = "--basedir="; const char *basedir_prefix = "--basedir=";
size_t basedir_len= strlen(basedir_prefix); size_t basedir_len= strlen(basedir_prefix);
...@@ -815,7 +815,7 @@ static int check_options(int argc, char **argv, char *operation) ...@@ -815,7 +815,7 @@ static int check_options(int argc, char **argv, char *operation)
return 1; return 1;
} }
/* If a plugin was specified, read the config file. */ /* If a plugin was specified, read the config file. */
else if (strlen(plugin_name) > 0) else if (strlen(plugin_name) > 0)
{ {
if (load_plugin_data(plugin_name, config_file)) if (load_plugin_data(plugin_name, config_file))
{ {
...@@ -847,22 +847,22 @@ static int check_options(int argc, char **argv, char *operation) ...@@ -847,22 +847,22 @@ static int check_options(int argc, char **argv, char *operation)
/** /**
Parse, execute, and verify command options. Parse, execute, and verify command options.
This method handles all of the option processing including the optional This method handles all of the option processing including the optional
features for displaying data (--print-defaults, --help ,etc.) that do not features for displaying data (--print-defaults, --help ,etc.) that do not
result in an attempt to ENABLE or DISABLE of a plugin. result in an attempt to ENABLE or DISABLE of a plugin.
@param[in] arc Count of arguments @param[in] arc Count of arguments
@param[in] argv Array of arguments @param[in] argv Array of arguments
@param[out] operation Operation (ENABLE or DISABLE) @param[out] operation Operation (ENABLE or DISABLE)
@retval int error = 1, success = 0, exit program = -1 @retval int error = 1, success = 0, exit program = -1
*/ */
static int process_options(int argc, char *argv[], char *operation) static int process_options(int argc, char *argv[], char *operation)
{ {
int error= 0; int error= 0;
/* Parse and execute command-line options */ /* Parse and execute command-line options */
if ((error= handle_options(&argc, &argv, my_long_options, get_one_option))) if ((error= handle_options(&argc, &argv, my_long_options, get_one_option)))
return error; return error;
...@@ -881,7 +881,7 @@ static int process_options(int argc, char *argv[], char *operation) ...@@ -881,7 +881,7 @@ static int process_options(int argc, char *argv[], char *operation)
char buff[FN_REFLEN]; char buff[FN_REFLEN];
if (basedir_len + 2 > FN_REFLEN) if (basedir_len + 2 > FN_REFLEN)
return -1; return -1;
memcpy(buff, opt_basedir, basedir_len); memcpy(buff, opt_basedir, basedir_len);
buff[basedir_len]= '/'; buff[basedir_len]= '/';
buff[basedir_len + 1]= '\0'; buff[basedir_len + 1]= '\0';
...@@ -890,7 +890,7 @@ static int process_options(int argc, char *argv[], char *operation) ...@@ -890,7 +890,7 @@ static int process_options(int argc, char *argv[], char *operation)
opt_basedir= my_strdup(buff, MYF(MY_FAE)); opt_basedir= my_strdup(buff, MYF(MY_FAE));
} }
} }
/* /*
If the user did not specify the option to skip loading defaults from a If the user did not specify the option to skip loading defaults from a
config file and the required options are not present or there was an error config file and the required options are not present or there was an error
...@@ -925,18 +925,18 @@ static int process_options(int argc, char *argv[], char *operation) ...@@ -925,18 +925,18 @@ static int process_options(int argc, char *argv[], char *operation)
/** /**
Check access Check access
This method checks to ensure all of the directories (opt_basedir, This method checks to ensure all of the directories (opt_basedir,
opt_plugin_dir, opt_datadir, and opt_plugin_ini) are accessible by opt_plugin_dir, opt_datadir, and opt_plugin_ini) are accessible by
the user. the user.
@retval int error = 1, success = 0 @retval int error = 1, success = 0
*/ */
static int check_access() static int check_access()
{ {
int error= 0; int error= 0;
if ((error= my_access(opt_basedir, F_OK))) if ((error= my_access(opt_basedir, F_OK)))
{ {
fprintf(stderr, "ERROR: Cannot access basedir at '%s'.\n", fprintf(stderr, "ERROR: Cannot access basedir at '%s'.\n",
...@@ -1048,13 +1048,13 @@ static int find_plugin(char *tp_path) ...@@ -1048,13 +1048,13 @@ static int find_plugin(char *tp_path)
/** /**
Build the bootstrap file. Build the bootstrap file.
Create a new file and populate it with SQL commands to ENABLE or DISABLE Create a new file and populate it with SQL commands to ENABLE or DISABLE
the plugin via REPLACE and DELETE operations on the mysql.plugin table. the plugin via REPLACE and DELETE operations on the mysql.plugin table.
param[in] operation The type of operation (ENABLE or DISABLE) param[in] operation The type of operation (ENABLE or DISABLE)
param[out] bootstrap A FILE* pointer param[out] bootstrap A FILE* pointer
@retval int error = 1, success = 0 @retval int error = 1, success = 0
*/ */
...@@ -1062,7 +1062,7 @@ static int build_bootstrap_file(char *operation, char *bootstrap) ...@@ -1062,7 +1062,7 @@ static int build_bootstrap_file(char *operation, char *bootstrap)
{ {
int error= 0; int error= 0;
FILE *file= 0; FILE *file= 0;
/* /*
Perform plugin operation : ENABLE or DISABLE Perform plugin operation : ENABLE or DISABLE
...@@ -1073,10 +1073,10 @@ static int build_bootstrap_file(char *operation, char *bootstrap) ...@@ -1073,10 +1073,10 @@ static int build_bootstrap_file(char *operation, char *bootstrap)
<plugin_name>.ini configuration file. Once the file is built, a call to <plugin_name>.ini configuration file. Once the file is built, a call to
mysqld is made in read only, bootstrap modes to read the SQL statements mysqld is made in read only, bootstrap modes to read the SQL statements
and execute them. and execute them.
Note: Replace was used so that if a user loads a newer version of a Note: Replace was used so that if a user loads a newer version of a
library with a different library name, the new library name is library with a different library name, the new library name is
used for symbols that match. used for symbols that match.
*/ */
if ((error= make_tempfile(bootstrap, "sql"))) if ((error= make_tempfile(bootstrap, "sql")))
{ {
...@@ -1123,7 +1123,7 @@ static int build_bootstrap_file(char *operation, char *bootstrap) ...@@ -1123,7 +1123,7 @@ static int build_bootstrap_file(char *operation, char *bootstrap)
printf("# Disabling %s...\n", plugin_data.name); printf("# Disabling %s...\n", plugin_data.name);
} }
} }
exit: exit:
fclose(file); fclose(file);
return error; return error;
...@@ -1132,11 +1132,11 @@ static int build_bootstrap_file(char *operation, char *bootstrap) ...@@ -1132,11 +1132,11 @@ static int build_bootstrap_file(char *operation, char *bootstrap)
/** /**
Dump bootstrap file. Dump bootstrap file.
Read the contents of the bootstrap file and print it out. Read the contents of the bootstrap file and print it out.
@param[in] bootstrap_file Name of bootstrap file to read @param[in] bootstrap_file Name of bootstrap file to read
@retval int error = 1, success = 0 @retval int error = 1, success = 0
*/ */
...@@ -1173,7 +1173,7 @@ static int dump_bootstrap_file(char *bootstrap_file) ...@@ -1173,7 +1173,7 @@ static int dump_bootstrap_file(char *bootstrap_file)
/** /**
Bootstrap the server Bootstrap the server
Create a command line sequence to launch mysqld in bootstrap mode. This Create a command line sequence to launch mysqld in bootstrap mode. This
will allow mysqld to launch a minimal server instance to read and will allow mysqld to launch a minimal server instance to read and
execute SQL commands from a file piped in (the bootstrap file). We use execute SQL commands from a file piped in (the bootstrap file). We use
...@@ -1194,47 +1194,39 @@ static int dump_bootstrap_file(char *bootstrap_file) ...@@ -1194,47 +1194,39 @@ static int dump_bootstrap_file(char *bootstrap_file)
static int bootstrap_server(char *server_path, char *bootstrap_file) static int bootstrap_server(char *server_path, char *bootstrap_file)
{ {
char bootstrap_cmd[FN_REFLEN]; char bootstrap_cmd[FN_REFLEN]= {0};
char lc_messages_dir_str[FN_REFLEN]= {0};
int error= 0; int error= 0;
#ifdef __WIN__ #ifdef __WIN__
char *format_str= 0; char *format_str= 0;
const char *verbose_str= NULL; const char *verbose_str= NULL;
#endif
if (opt_lc_messages_dir != NULL)
snprintf(lc_messages_dir_str, sizeof(lc_messages_dir_str), "--lc-messages-dir=%s",
opt_lc_messages_dir);
#ifdef __WIN__
if (opt_verbose) if (opt_verbose)
verbose_str= "--console"; verbose_str= "--console";
else else
verbose_str= ""; verbose_str= "";
if (has_spaces(opt_datadir) || has_spaces(opt_basedir) || if (has_spaces(opt_datadir) || has_spaces(opt_basedir) ||
has_spaces(bootstrap_file)) has_spaces(bootstrap_file) || has_spaces(lc_messages_dir_str))
{ format_str= "\"%s %s --bootstrap --datadir=%s --basedir=%s %s <%s\"";
if (opt_lc_messages_dir != NULL)
format_str= "\"%s %s --bootstrap --datadir=%s --basedir=%s --lc-messages-dir=%s <%s\"";
else
format_str= "\"%s %s --bootstrap --datadir=%s --basedir=%s <%s\"";
}
else else
{ format_str= "%s %s --bootstrap --datadir=%s --basedir=%s %s <%s";
if (opt_lc_messages_dir != NULL)
format_str= "\"%s %s --bootstrap --datadir=%s --basedir=%s --lc-messages-dir=%s <%s\"";
else
format_str= "%s %s --bootstrap --datadir=%s --basedir=%s <%s";
}
snprintf(bootstrap_cmd, sizeof(bootstrap_cmd), format_str, snprintf(bootstrap_cmd, sizeof(bootstrap_cmd), format_str,
add_quotes(convert_path(server_path)), verbose_str, add_quotes(convert_path(server_path)), verbose_str,
add_quotes(opt_datadir), add_quotes(opt_basedir), add_quotes(opt_datadir), add_quotes(opt_basedir),
add_quotes(bootstrap_file)); add_quotes(lc_messages_dir_str), add_quotes(bootstrap_file));
#else #else
if (opt_lc_messages_dir != NULL) snprintf(bootstrap_cmd, sizeof(bootstrap_cmd),
snprintf(bootstrap_cmd, sizeof(bootstrap_cmd), "%s --no-defaults --bootstrap --datadir=%s --basedir=%s %s"
"%s --no-defaults --bootstrap --datadir=%s --basedir=%s --lc-messages-dir=%s" " <%s", server_path, opt_datadir, opt_basedir, lc_messages_dir_str, bootstrap_file);
" <%s", server_path, opt_datadir, opt_basedir, opt_lc_messages_dir, bootstrap_file);
else
snprintf(bootstrap_cmd, sizeof(bootstrap_cmd),
"%s --no-defaults --bootstrap --datadir=%s --basedir=%s"
" <%s", server_path, opt_datadir, opt_basedir, bootstrap_file);
#endif #endif
/* Execute the command */ /* Execute the command */
...@@ -1247,6 +1239,6 @@ static int bootstrap_server(char *server_path, char *bootstrap_file) ...@@ -1247,6 +1239,6 @@ static int bootstrap_server(char *server_path, char *bootstrap_file)
fprintf(stderr, fprintf(stderr,
"ERROR: Unexpected result from bootstrap. Error code: %d.\n", "ERROR: Unexpected result from bootstrap. Error code: %d.\n",
error); error);
return error; return error;
} }
...@@ -2534,7 +2534,7 @@ swift_keystone_auth_v3(const char *auth_url, swift_auth_info *info) ...@@ -2534,7 +2534,7 @@ swift_keystone_auth_v3(const char *auth_url, swift_auth_info *info)
} else if (opt_swift_project != NULL) { } else if (opt_swift_project != NULL) {
snprintf(scope, sizeof(scope), snprintf(scope, sizeof(scope),
",\"scope\":{\"project\":{\"name\":\"%s\"%s}}", ",\"scope\":{\"project\":{\"name\":\"%s\"%s}}",
opt_swift_project_id, domain); opt_swift_project, domain);
} }
snprintf(payload, sizeof(payload), "{\"auth\":{\"identity\":" snprintf(payload, sizeof(payload), "{\"auth\":{\"identity\":"
......
...@@ -45,6 +45,7 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags __attribute__((unused)), ...@@ -45,6 +45,7 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags __attribute__((unused)),
else else
{ {
free(resp); free(resp);
resp= NULL;
msg[0].msg_style = PAM_PROMPT_ECHO_ON; msg[0].msg_style = PAM_PROMPT_ECHO_ON;
msg[0].msg = "PIN:"; msg[0].msg = "PIN:";
pam_err = (*conv->conv)(1, msgp, &resp, conv->appdata_ptr); pam_err = (*conv->conv)(1, msgp, &resp, conv->appdata_ptr);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment