| Message ID | 20250608080030.2436566-1-roid@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] ovsdb-server: Fix possible memory leak of config_file_path. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/cirrus-robot | success | cirrus build: passed |
On 6/8/25 10:00 AM, Roi Dayan via dev wrote: > Reported by Coverity. > overwrite_var: Overwriting "config_file_path" in > "config_file_path = abs_file_name(ovs_dbdir(), optarg)" > leaks the storage that "config_file_path" points to. > Make sure any existing config_file_path is freed before > overwriting. > > Signed-off-by: Roi Dayan <roid@nvidia.com> > Acked-by: Eli Britstein <elibr@nvidia.com> > --- > ovsdb/ovsdb-server.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c > index a247ae8f0ae0..a874a2130d47 100644 > --- a/ovsdb/ovsdb-server.c > +++ b/ovsdb/ovsdb-server.c > @@ -2747,8 +2747,10 @@ parse_options(int argc, char *argv[], > break; > > case OPT_CONFIG_FILE: > - config_file_path = abs_file_name(ovs_dbdir(), optarg); > - add_default_db = false; > + if (!config_file_path) { > + config_file_path = abs_file_name(ovs_dbdir(), optarg); > + add_default_db = false; > + } > break; > > case '?': Hi, Roi and Eli. I suppose this patch attempts to solve the same issue as the patch Eelco sent last week: https://patchwork.ozlabs.org/project/openvswitch/patch/7e7ba27a887674df28bbc23b753b58b300981756.1749133911.git.echaudro@redhat.com/ The solution in Eelco's version seems better as typically in command line interfaces the last provided argument is used out of duplicated ones, not the first. Best regards, Ilya Maximets.
On 09/06/2025 1:03, Ilya Maximets wrote: > On 6/8/25 10:00 AM, Roi Dayan via dev wrote: >> Reported by Coverity. >> overwrite_var: Overwriting "config_file_path" in >> "config_file_path = abs_file_name(ovs_dbdir(), optarg)" >> leaks the storage that "config_file_path" points to. >> Make sure any existing config_file_path is freed before >> overwriting. >> >> Signed-off-by: Roi Dayan <roid@nvidia.com> >> Acked-by: Eli Britstein <elibr@nvidia.com> >> --- >> ovsdb/ovsdb-server.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c >> index a247ae8f0ae0..a874a2130d47 100644 >> --- a/ovsdb/ovsdb-server.c >> +++ b/ovsdb/ovsdb-server.c >> @@ -2747,8 +2747,10 @@ parse_options(int argc, char *argv[], >> break; >> >> case OPT_CONFIG_FILE: >> - config_file_path = abs_file_name(ovs_dbdir(), optarg); >> - add_default_db = false; >> + if (!config_file_path) { >> + config_file_path = abs_file_name(ovs_dbdir(), optarg); >> + add_default_db = false; >> + } >> break; >> >> case '?': > > > Hi, Roi and Eli. I suppose this patch attempts to solve the same issue > as the patch Eelco sent last week: > https://patchwork.ozlabs.org/project/openvswitch/patch/7e7ba27a887674df28bbc23b753b58b300981756.1749133911.git.echaudro@redhat.com/ > > The solution in Eelco's version seems better as typically in command line > interfaces the last provided argument is used out of duplicated ones, not > the first. > > Best regards, Ilya Maximets. didn't notice. thanks. I think it's not consistent though. In dpctl_dump_flows() filter and types_list can only be set once as there is a null check.
diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c index a247ae8f0ae0..a874a2130d47 100644 --- a/ovsdb/ovsdb-server.c +++ b/ovsdb/ovsdb-server.c @@ -2747,8 +2747,10 @@ parse_options(int argc, char *argv[], break; case OPT_CONFIG_FILE: - config_file_path = abs_file_name(ovs_dbdir(), optarg); - add_default_db = false; + if (!config_file_path) { + config_file_path = abs_file_name(ovs_dbdir(), optarg); + add_default_db = false; + } break; case '?':