Skip to content

Commit

Permalink
Fix few vulnerabilities found by Cppcheck
Browse files Browse the repository at this point in the history 
While performing SAST scanning using Cppcheck against source code of
commit 8119646, 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 630d722 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.
  • Loading branch information
@HugoWenTD @grooverdan
HugoWenTD authored and grooverdan committed Mar 2, 2023
1 parent acfb5df commit 7bdd878
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 61 deletions.
112 changes: 52 additions & 60 deletions client/mysql_plugin.c
View file
Expand Up @@ -102,35 +102,35 @@ int main(int argc,char *argv[])
MY_INIT(argv[0]);
sf_leaking_memory=1; /* don't report memory leaks on early exits */
plugin_data.name= 0; /* initialize name */

/*
The following operations comprise the method for enabling or disabling
a plugin. We begin by processing the command options then check the
directories specified for --datadir, --basedir, --plugin-dir, and
--plugin-ini (if specified). If the directories are Ok, we then look
for the mysqld executable and the plugin soname. Finally, we build a
bootstrap command file for use in bootstraping the server.
If any step fails, the method issues an error message and the tool exits.
1) Parse, execute, and verify command options.
2) Check access to directories.
3) Look for mysqld executable.
4) Look for the plugin.
5) Build a bootstrap file with commands to enable or disable plugin.
*/
if ((error= process_options(argc, argv, operation)) ||
(error= check_access()) ||
(error= find_tool("mysqld" FN_EXEEXT, server_path)) ||
(error= find_plugin(tp_path)) ||
(error= build_bootstrap_file(operation, bootstrap)))
goto exit;

/* Dump the bootstrap file if --verbose specified. */
if (opt_verbose && ((error= dump_bootstrap_file(bootstrap))))
goto exit;

/* Start the server in bootstrap mode and execute bootstrap commands */
error= bootstrap_server(server_path, bootstrap);

Expand Down Expand Up @@ -238,7 +238,7 @@ static int run_command(char* cmd, const char *mode)
#ifdef __WIN__
/**
Check to see if there are spaces in a path.
@param[in] path The Windows path to examine.
@retval int spaces found = 1, no spaces = 0
Expand All @@ -253,7 +253,7 @@ static int has_spaces(const char *path)

/**
Convert a Unix path to a Windows path.
@param[in] path The Windows path to examine.
@returns string containing path with / changed to \\
Expand Down Expand Up @@ -335,12 +335,12 @@ static int get_default_values()
#ifdef __WIN__
{
char *format_str= 0;

if (has_spaces(tool_path) || has_spaces(defaults_file))
format_str = "\"%s --mysqld > %s\"";
else
format_str = "%s --mysqld > %s";

snprintf(defaults_cmd, sizeof(defaults_cmd), format_str,
add_quotes(tool_path), add_quotes(defaults_file));
if (opt_verbose)
Expand Down Expand Up @@ -675,7 +675,7 @@ static int load_plugin_data(char *plugin_name, char *config_file)
{
reason= "Bad format in plugin configuration file.";
fclose(file_ptr);
goto error;
goto error;
}
break;
}
Expand Down Expand Up @@ -709,7 +709,7 @@ static int load_plugin_data(char *plugin_name, char *config_file)
}
}
}

fclose(file_ptr);
return 0;

Expand Down Expand Up @@ -740,7 +740,7 @@ static int check_options(int argc, char **argv, char *operation)
int num_found= 0; /* number of options found (shortcut loop) */
char config_file[FN_REFLEN]; /* configuration file name */
char plugin_name[FN_REFLEN]; /* plugin name */

/* Form prefix strings for the options. */
const char *basedir_prefix = "--basedir=";
size_t basedir_len= strlen(basedir_prefix);
Expand Down Expand Up @@ -815,7 +815,7 @@ static int check_options(int argc, char **argv, char *operation)
return 1;
}
/* 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))
{
Expand Down Expand Up @@ -847,22 +847,22 @@ static int check_options(int argc, char **argv, char *operation)

/**
Parse, execute, and verify command options.
This method handles all of the option processing including the optional
features for displaying data (--print-defaults, --help ,etc.) that do not
result in an attempt to ENABLE or DISABLE of a plugin.
@param[in] arc Count of arguments
@param[in] argv Array of arguments
@param[out] operation Operation (ENABLE or DISABLE)
@retval int error = 1, success = 0, exit program = -1
*/

static int process_options(int argc, char *argv[], char *operation)
{
int error= 0;

/* Parse and execute command-line options */
if ((error= handle_options(&argc, &argv, my_long_options, get_one_option)))
return error;
Expand All @@ -881,7 +881,7 @@ static int process_options(int argc, char *argv[], char *operation)
char buff[FN_REFLEN];
if (basedir_len + 2 > FN_REFLEN)
return -1;

memcpy(buff, opt_basedir, basedir_len);
buff[basedir_len]= '/';
buff[basedir_len + 1]= '\0';
Expand All @@ -890,7 +890,7 @@ static int process_options(int argc, char *argv[], char *operation)
opt_basedir= my_strdup(buff, MYF(MY_FAE));
}
}

/*
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
Expand Down Expand Up @@ -925,18 +925,18 @@ static int process_options(int argc, char *argv[], char *operation)

/**
Check access
This method checks to ensure all of the directories (opt_basedir,
opt_plugin_dir, opt_datadir, and opt_plugin_ini) are accessible by
the user.
@retval int error = 1, success = 0
*/

static int check_access()
{
int error= 0;

if ((error= my_access(opt_basedir, F_OK)))
{
fprintf(stderr, "ERROR: Cannot access basedir at '%s'.\n",
Expand Down Expand Up @@ -1048,21 +1048,21 @@ static int find_plugin(char *tp_path)

/**
Build the bootstrap file.
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.
param[in] operation The type of operation (ENABLE or DISABLE)
param[out] bootstrap A FILE* pointer
@retval int error = 1, success = 0
*/

static int build_bootstrap_file(char *operation, char *bootstrap)
{
int error= 0;
FILE *file= 0;

/*
Perform plugin operation : ENABLE or DISABLE
Expand All @@ -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
mysqld is made in read only, bootstrap modes to read the SQL statements
and execute them.
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
used for symbols that match.
used for symbols that match.
*/
if ((error= make_tempfile(bootstrap, "sql")))
{
Expand Down Expand Up @@ -1123,7 +1123,7 @@ static int build_bootstrap_file(char *operation, char *bootstrap)
printf("# Disabling %s...\n", plugin_data.name);
}
}

exit:
fclose(file);
return error;
Expand All @@ -1132,11 +1132,11 @@ static int build_bootstrap_file(char *operation, char *bootstrap)

/**
Dump bootstrap file.
Read the contents of the bootstrap file and print it out.
@param[in] bootstrap_file Name of bootstrap file to read
@retval int error = 1, success = 0
*/

Expand Down Expand Up @@ -1173,7 +1173,7 @@ static int dump_bootstrap_file(char *bootstrap_file)

/**
Bootstrap the server
Create a command line sequence to launch mysqld in bootstrap mode. This
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
Expand All @@ -1194,47 +1194,39 @@ static int dump_bootstrap_file(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;

#ifdef __WIN__
char *format_str= 0;
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)
verbose_str= "--console";
else
verbose_str= "";

if (has_spaces(opt_datadir) || has_spaces(opt_basedir) ||
has_spaces(bootstrap_file))
{
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\"";
}
has_spaces(bootstrap_file) || has_spaces(lc_messages_dir_str))
format_str= "\"%s %s --bootstrap --datadir=%s --basedir=%s %s <%s\"";
else
{
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";
}
format_str= "%s %s --bootstrap --datadir=%s --basedir=%s %s <%s";

snprintf(bootstrap_cmd, sizeof(bootstrap_cmd), format_str,
add_quotes(convert_path(server_path)), verbose_str,
add_quotes(opt_datadir), add_quotes(opt_basedir),
add_quotes(bootstrap_file));
add_quotes(lc_messages_dir_str), add_quotes(bootstrap_file));
#else
if (opt_lc_messages_dir != NULL)
snprintf(bootstrap_cmd, sizeof(bootstrap_cmd),
"%s --no-defaults --bootstrap --datadir=%s --basedir=%s --lc-messages-dir=%s"
" <%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);

snprintf(bootstrap_cmd, sizeof(bootstrap_cmd),
"%s --no-defaults --bootstrap --datadir=%s --basedir=%s %s"
" <%s", server_path, opt_datadir, opt_basedir, lc_messages_dir_str, bootstrap_file);
#endif

/* Execute the command */
Expand All @@ -1247,6 +1239,6 @@ static int bootstrap_server(char *server_path, char *bootstrap_file)
fprintf(stderr,
"ERROR: Unexpected result from bootstrap. Error code: %d.\n",
error);

return error;
}
2 changes: 1 addition & 1 deletion extra/mariabackup/xbcloud.cc
View file
Expand Up @@ -2534,7 +2534,7 @@ swift_keystone_auth_v3(const char *auth_url, swift_auth_info *info)
} else if (opt_swift_project != NULL) {
snprintf(scope, sizeof(scope),
",\"scope\":{\"project\":{\"name\":\"%s\"%s}}",
opt_swift_project_id, domain);
opt_swift_project, domain);
}

snprintf(payload, sizeof(payload), "{\"auth\":{\"identity\":"
Expand Down
1 change: 1 addition & 0 deletions plugin/auth_pam/testing/pam_mariadb_mtr.c
View file
Expand Up @@ -45,6 +45,7 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags __attribute__((unused)),
else
{
free(resp);
resp= NULL;
msg[0].msg_style = PAM_PROMPT_ECHO_ON;
msg[0].msg = "PIN:";
pam_err = (*conv->conv)(1, msgp, &resp, conv->appdata_ptr);
Expand Down

0 comments on commit 7bdd878

Please sign in to comment.