| Message ID | 20230222125651.77895-1-amusil@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] dbctl: Fix a couple of memory leaks | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Wed, Feb 22, 2023 at 01:56:51PM +0100, Ales Musil wrote: > Nothing is being freed wherever we are calling > ctl_fatal which is fine because the program is > about to shutdown anyway however one of the > leaks was caught by address sanitizer. > Fix most of the leaks that are happening before > call to ctl_fatal. > > Direct leak of 64 byte(s) in 1 object(s) allocated from: > #0 0x568d70 in __interceptor_realloc.part.0 asan_malloc_linux.cpp.o > #1 0xa530a5 in xrealloc__ /workspace/ovn/ovs/lib/util.c:147:9 > #2 0xa530a5 in xrealloc /workspace/ovn/ovs/lib/util.c:179:12 > #3 0xa530a5 in x2nrealloc /workspace/ovn/ovs/lib/util.c:239:12 > #4 0xa2ee57 in svec_expand /workspace/ovn/ovs/lib/svec.c:92:23 > #5 0xa2ef6e in svec_terminate /workspace/ovn/ovs/lib/svec.c:116:5 > #6 0x82c117 in ovs_cmdl_env_parse_all /workspace/ovn/ovs/lib/command-line.c:98:5 > #7 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20 > #8 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12 > > Indirect leak of 10 byte(s) in 1 object(s) allocated from: > #0 0x569c07 in malloc (/workspace/ovn/utilities/ovn-nbctl+0x569c07) (BuildId: 3a287416f70de43f52382f0336980c876f655f90) > #1 0xa52d4c in xmalloc__ /workspace/ovn/ovs/lib/util.c:137:15 > #2 0xa52d4c in xmalloc /workspace/ovn/ovs/lib/util.c:172:12 > #3 0xa52d4c in xmemdup0 /workspace/ovn/ovs/lib/util.c:193:15 > #4 0xa2e580 in svec_add /workspace/ovn/ovs/lib/svec.c:71:27 > #5 0x82c041 in ovs_cmdl_env_parse_all /workspace/ovn/ovs/lib/command-line.c:91:5 > #6 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20 > #7 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12 > > Indirect leak of 8 byte(s) in 2 object(s) allocated from: > #0 0x569c07 in malloc (/workspace/ovn/utilities/ovn-nbctl+0x569c07) > #1 0xa52d4c in xmalloc__ /workspace/ovn/ovs/lib/util.c:137:15 > #2 0xa52d4c in xmalloc /workspace/ovn/ovs/lib/util.c:172:12 > #3 0xa52d4c in xmemdup0 /workspace/ovn/ovs/lib/util.c:193:15 > #4 0xa2e580 in svec_add /workspace/ovn/ovs/lib/svec.c:71:27 > #5 0x82c0b6 in ovs_cmdl_env_parse_all /workspace/ovn/ovs/lib/command-line.c:96:9 > #6 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20 > #7 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12 > > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v2: Address comments from Simon: > - Rearrange the cleanup according to suggestion. > - Allow free(NULL). Minor nit not withstanding, this looks good to me. Reviewed-by: Simon Horman <simon.horman@corigine.com> > --- > utilities/ovn-dbctl.c | 76 ++++++++++++++++++++++++++----------------- > 1 file changed, 46 insertions(+), 30 deletions(-) ... > @@ -1240,40 +1242,54 @@ dbctl_client(const struct ovn_dbctl_options *dbctl_options, > > ctl_timeout_setup(timeout); > > + char *cmd_result = NULL; > + char *cmd_error = NULL; > struct jsonrpc *client; > + int exit_status; > + char *error_str; > + > int error = unixctl_client_create(socket_name, &client); > if (error) { > - ctl_fatal("%s: could not connect to %s daemon (%s); " > - "unset %s to avoid using daemon", > - socket_name, program_name, ovs_strerror(error), > - dbctl_options->daemon_env_var_name); > + error_str = xasprintf("%s: could not connect to %s daemon (%s); " > + "unset %s to avoid using daemon", > + socket_name, program_name, ovs_strerror(error), > + dbctl_options->daemon_env_var_name); > + goto log_error; > } > > - char *cmd_result; > - char *cmd_error; > + nit: there are now two blank lines here > error = unixctl_client_transact(client, "run", > args.n, args.names, > &cmd_result, &cmd_error); > if (error) { > - ctl_fatal("%s: transaction error (%s)", > - socket_name, ovs_strerror(error)); > + error_str = xasprintf("%s: transaction error (%s)", > + socket_name, ovs_strerror(error)); > + goto log_error; > } > - svec_destroy(&args); > > - int exit_status; > if (cmd_error) { > - exit_status = EXIT_FAILURE; > fprintf(stderr, "%s: %s", program_name, cmd_error); > - } else { > - exit_status = EXIT_SUCCESS; > - fputs(cmd_result, stdout); > + goto error; > } > + > + exit_status = EXIT_SUCCESS; > + fputs(cmd_result, stdout); > + goto cleanup; > + > +log_error: > + VLOG_ERR("%s", error_str); > + ovs_error(0, "%s", error_str); > + free(error_str); > + > +error: > + exit_status = EXIT_FAILURE; > + > +cleanup: > free(cmd_result); > free(cmd_error); > jsonrpc_close(client); > - for (int i = 0; i < argc; i++) { > - free(argv[i]); > - } > - free(argv); > + svec_destroy(&args); > + destroy_argv(argc, argv); > + > exit(exit_status); > } > -- > 2.39.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 2/22/23 15:24, Simon Horman wrote: > On Wed, Feb 22, 2023 at 01:56:51PM +0100, Ales Musil wrote: >> Nothing is being freed wherever we are calling >> ctl_fatal which is fine because the program is >> about to shutdown anyway however one of the >> leaks was caught by address sanitizer. >> Fix most of the leaks that are happening before >> call to ctl_fatal. >> >> Direct leak of 64 byte(s) in 1 object(s) allocated from: >> #0 0x568d70 in __interceptor_realloc.part.0 asan_malloc_linux.cpp.o >> #1 0xa530a5 in xrealloc__ /workspace/ovn/ovs/lib/util.c:147:9 >> #2 0xa530a5 in xrealloc /workspace/ovn/ovs/lib/util.c:179:12 >> #3 0xa530a5 in x2nrealloc /workspace/ovn/ovs/lib/util.c:239:12 >> #4 0xa2ee57 in svec_expand /workspace/ovn/ovs/lib/svec.c:92:23 >> #5 0xa2ef6e in svec_terminate /workspace/ovn/ovs/lib/svec.c:116:5 >> #6 0x82c117 in ovs_cmdl_env_parse_all /workspace/ovn/ovs/lib/command-line.c:98:5 >> #7 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20 >> #8 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12 >> >> Indirect leak of 10 byte(s) in 1 object(s) allocated from: >> #0 0x569c07 in malloc (/workspace/ovn/utilities/ovn-nbctl+0x569c07) (BuildId: 3a287416f70de43f52382f0336980c876f655f90) >> #1 0xa52d4c in xmalloc__ /workspace/ovn/ovs/lib/util.c:137:15 >> #2 0xa52d4c in xmalloc /workspace/ovn/ovs/lib/util.c:172:12 >> #3 0xa52d4c in xmemdup0 /workspace/ovn/ovs/lib/util.c:193:15 >> #4 0xa2e580 in svec_add /workspace/ovn/ovs/lib/svec.c:71:27 >> #5 0x82c041 in ovs_cmdl_env_parse_all /workspace/ovn/ovs/lib/command-line.c:91:5 >> #6 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20 >> #7 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12 >> >> Indirect leak of 8 byte(s) in 2 object(s) allocated from: >> #0 0x569c07 in malloc (/workspace/ovn/utilities/ovn-nbctl+0x569c07) >> #1 0xa52d4c in xmalloc__ /workspace/ovn/ovs/lib/util.c:137:15 >> #2 0xa52d4c in xmalloc /workspace/ovn/ovs/lib/util.c:172:12 >> #3 0xa52d4c in xmemdup0 /workspace/ovn/ovs/lib/util.c:193:15 >> #4 0xa2e580 in svec_add /workspace/ovn/ovs/lib/svec.c:71:27 >> #5 0x82c0b6 in ovs_cmdl_env_parse_all /workspace/ovn/ovs/lib/command-line.c:96:9 >> #6 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20 >> #7 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12 >> >> Signed-off-by: Ales Musil <amusil@redhat.com> >> --- >> v2: Address comments from Simon: >> - Rearrange the cleanup according to suggestion. >> - Allow free(NULL). > > Minor nit not withstanding, this looks good to me. > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > I took care of the nit and applied the patch to main and backported to all stable branches down to 22.03. Thanks for the fix, Ales and for reviews, Simon! Regards, Dumitru
diff --git a/utilities/ovn-dbctl.c b/utilities/ovn-dbctl.c index f0ee0ba05..dbe6a745a 100644 --- a/utilities/ovn-dbctl.c +++ b/utilities/ovn-dbctl.c @@ -109,6 +109,15 @@ static void server_loop(const struct ovn_dbctl_options *dbctl_options, struct ovsdb_idl *idl, int argc, char *argv[]); static void ovn_dbctl_exit(int status); +static void +destroy_argv(int argc, char **argv) +{ + for (int i = 0; i < argc; i++) { + free(argv[i]); + } + free(argv); +} + int ovn_dbctl_main(int argc, char *argv[], const struct ovn_dbctl_options *dbctl_options) @@ -151,6 +160,7 @@ ovn_dbctl_main(int argc, char *argv[], char *error_s = ovs_cmdl_parse_all(argc, argv_, get_all_options(), &parsed_options, &n_parsed_options); if (error_s) { + destroy_argv(argc, argv_); ctl_fatal("%s", error_s); } @@ -179,6 +189,7 @@ ovn_dbctl_main(int argc, char *argv[], bool daemon_mode = false; if (get_detach()) { if (argc != optind) { + destroy_argv(argc, argv_); ctl_fatal("non-option arguments not supported with --detach " "(use --help for help)"); } @@ -206,11 +217,8 @@ ovn_dbctl_main(int argc, char *argv[], if (error) { ovsdb_idl_destroy(idl); idl = the_idl = NULL; + destroy_argv(argc, argv_); - for (int i = 0; i < argc; i++) { - free(argv_[i]); - } - free(argv_); ctl_fatal("%s", error); } @@ -239,21 +247,15 @@ cleanup: } free(commands); if (error) { - for (int i = 0; i < argc; i++) { - free(argv_[i]); - } - free(argv_); + destroy_argv(argc, argv_); ctl_fatal("%s", error); } } ovsdb_idl_destroy(idl); idl = the_idl = NULL; + destroy_argv(argc, argv_); - for (int i = 0; i < argc; i++) { - free(argv_[i]); - } - free(argv_); exit(EXIT_SUCCESS); } @@ -1240,40 +1242,54 @@ dbctl_client(const struct ovn_dbctl_options *dbctl_options, ctl_timeout_setup(timeout); + char *cmd_result = NULL; + char *cmd_error = NULL; struct jsonrpc *client; + int exit_status; + char *error_str; + int error = unixctl_client_create(socket_name, &client); if (error) { - ctl_fatal("%s: could not connect to %s daemon (%s); " - "unset %s to avoid using daemon", - socket_name, program_name, ovs_strerror(error), - dbctl_options->daemon_env_var_name); + error_str = xasprintf("%s: could not connect to %s daemon (%s); " + "unset %s to avoid using daemon", + socket_name, program_name, ovs_strerror(error), + dbctl_options->daemon_env_var_name); + goto log_error; } - char *cmd_result; - char *cmd_error; + error = unixctl_client_transact(client, "run", args.n, args.names, &cmd_result, &cmd_error); if (error) { - ctl_fatal("%s: transaction error (%s)", - socket_name, ovs_strerror(error)); + error_str = xasprintf("%s: transaction error (%s)", + socket_name, ovs_strerror(error)); + goto log_error; } - svec_destroy(&args); - int exit_status; if (cmd_error) { - exit_status = EXIT_FAILURE; fprintf(stderr, "%s: %s", program_name, cmd_error); - } else { - exit_status = EXIT_SUCCESS; - fputs(cmd_result, stdout); + goto error; } + + exit_status = EXIT_SUCCESS; + fputs(cmd_result, stdout); + goto cleanup; + +log_error: + VLOG_ERR("%s", error_str); + ovs_error(0, "%s", error_str); + free(error_str); + +error: + exit_status = EXIT_FAILURE; + +cleanup: free(cmd_result); free(cmd_error); jsonrpc_close(client); - for (int i = 0; i < argc; i++) { - free(argv[i]); - } - free(argv); + svec_destroy(&args); + destroy_argv(argc, argv); + exit(exit_status); }
Nothing is being freed wherever we are calling ctl_fatal which is fine because the program is about to shutdown anyway however one of the leaks was caught by address sanitizer. Fix most of the leaks that are happening before call to ctl_fatal. Direct leak of 64 byte(s) in 1 object(s) allocated from: #0 0x568d70 in __interceptor_realloc.part.0 asan_malloc_linux.cpp.o #1 0xa530a5 in xrealloc__ /workspace/ovn/ovs/lib/util.c:147:9 #2 0xa530a5 in xrealloc /workspace/ovn/ovs/lib/util.c:179:12 #3 0xa530a5 in x2nrealloc /workspace/ovn/ovs/lib/util.c:239:12 #4 0xa2ee57 in svec_expand /workspace/ovn/ovs/lib/svec.c:92:23 #5 0xa2ef6e in svec_terminate /workspace/ovn/ovs/lib/svec.c:116:5 #6 0x82c117 in ovs_cmdl_env_parse_all /workspace/ovn/ovs/lib/command-line.c:98:5 #7 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20 #8 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12 Indirect leak of 10 byte(s) in 1 object(s) allocated from: #0 0x569c07 in malloc (/workspace/ovn/utilities/ovn-nbctl+0x569c07) (BuildId: 3a287416f70de43f52382f0336980c876f655f90) #1 0xa52d4c in xmalloc__ /workspace/ovn/ovs/lib/util.c:137:15 #2 0xa52d4c in xmalloc /workspace/ovn/ovs/lib/util.c:172:12 #3 0xa52d4c in xmemdup0 /workspace/ovn/ovs/lib/util.c:193:15 #4 0xa2e580 in svec_add /workspace/ovn/ovs/lib/svec.c:71:27 #5 0x82c041 in ovs_cmdl_env_parse_all /workspace/ovn/ovs/lib/command-line.c:91:5 #6 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20 #7 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12 Indirect leak of 8 byte(s) in 2 object(s) allocated from: #0 0x569c07 in malloc (/workspace/ovn/utilities/ovn-nbctl+0x569c07) #1 0xa52d4c in xmalloc__ /workspace/ovn/ovs/lib/util.c:137:15 #2 0xa52d4c in xmalloc /workspace/ovn/ovs/lib/util.c:172:12 #3 0xa52d4c in xmemdup0 /workspace/ovn/ovs/lib/util.c:193:15 #4 0xa2e580 in svec_add /workspace/ovn/ovs/lib/svec.c:71:27 #5 0x82c0b6 in ovs_cmdl_env_parse_all /workspace/ovn/ovs/lib/command-line.c:96:9 #6 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20 #7 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12 Signed-off-by: Ales Musil <amusil@redhat.com> --- v2: Address comments from Simon: - Rearrange the cleanup according to suggestion. - Allow free(NULL). --- utilities/ovn-dbctl.c | 76 ++++++++++++++++++++++++++----------------- 1 file changed, 46 insertions(+), 30 deletions(-)