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 <[email protected]> > --- > 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 <[email protected]> > --- > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
