| Message ID | 975c9640eb5d8b92d190da286a41ef579d1046fb.1534929327.git.sbrivio@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Jozsef Kadlecsik |
| Headers | show |
| Series | Fix issues reported by Covscan | expand |
Hi Stefano, On Wed, 22 Aug 2018, Stefano Brivio wrote: > When check_setname is used in ipset_parse_name_compat(), the > 'str' and 'saved' macro arguments point in fact to the same > buffer. Free the 'saved' argument only after using it. > > While at it, remove a useless NULL check on 'saved'. > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com> > --- > lib/parse.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/lib/parse.c b/lib/parse.c > index 9a79ccda796c..4963d519c631 100644 > --- a/lib/parse.c > +++ b/lib/parse.c > @@ -1396,10 +1396,11 @@ ipset_parse_iptimeout(struct ipset_session *session, > #define check_setname(str, saved) \ > do { \ > if (strlen(str) > IPSET_MAXNAMELEN - 1) { \ > - if (saved != NULL) \ > - free(saved); \ > - return syntax_err("setname '%s' is longer than %u characters",\ > + int err; \ > + err = syntax_err("setname '%s' is longer than %u characters",\ > str, IPSET_MAXNAMELEN - 1); \ > + free(saved); \ > + return err; \ > } \ > } while (0) At some places check_setname() is invoked with "saved" explicitly set to NULL, so the condition to free cannot be left out. Please resubmit a new version of the patch. Thanks! Best regards, Jozsef - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary
On Fri, 24 Aug 2018, Jozsef Kadlecsik wrote: > Hi Stefano, > > On Wed, 22 Aug 2018, Stefano Brivio wrote: > > > When check_setname is used in ipset_parse_name_compat(), the > > 'str' and 'saved' macro arguments point in fact to the same > > buffer. Free the 'saved' argument only after using it. > > > > While at it, remove a useless NULL check on 'saved'. > > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com> > > --- > > lib/parse.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/lib/parse.c b/lib/parse.c > > index 9a79ccda796c..4963d519c631 100644 > > --- a/lib/parse.c > > +++ b/lib/parse.c > > @@ -1396,10 +1396,11 @@ ipset_parse_iptimeout(struct ipset_session *session, > > #define check_setname(str, saved) \ > > do { \ > > if (strlen(str) > IPSET_MAXNAMELEN - 1) { \ > > - if (saved != NULL) \ > > - free(saved); \ > > - return syntax_err("setname '%s' is longer than %u characters",\ > > + int err; \ > > + err = syntax_err("setname '%s' is longer than %u characters",\ > > str, IPSET_MAXNAMELEN - 1); \ > > + free(saved); \ > > + return err; \ > > } \ > > } while (0) > > At some places check_setname() is invoked with "saved" explicitly set to > NULL, so the condition to free cannot be left out. Please resubmit a new > version of the patch. Thanks! Why? The free() manpage says: If ptr is NULL, no operation is performed. So it is ok to always call it on a NULL pointer. The same as with kfree() in the kernel, no extra NULL check needed before. c'ya sven-haegar
Hi Jozsef, On Fri, 24 Aug 2018 21:02:12 +0200 (CEST) Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote: > > @@ -1396,10 +1396,11 @@ ipset_parse_iptimeout(struct ipset_session *session, > > #define check_setname(str, saved) \ > > do { \ > > if (strlen(str) > IPSET_MAXNAMELEN - 1) { \ > > - if (saved != NULL) \ > > - free(saved); \ > > - return syntax_err("setname '%s' is longer than %u characters",\ > > + int err; \ > > + err = syntax_err("setname '%s' is longer than %u characters",\ > > str, IPSET_MAXNAMELEN - 1); \ > > + free(saved); \ > > + return err; \ > > } \ > > } while (0) > > At some places check_setname() is invoked with "saved" explicitly set to > NULL, so the condition to free cannot be left out. Please resubmit a new > version of the patch. Thanks! I dropped this check because, as Sven-Haegar mentioned, free() on a NULL pointer has no effect. Both C89 and C99 say: The free function causes the space pointed to by ptr to be deallocated, that is, made available for further allocation. If ptr is a null pointer, no action occurs. As a side note, this pattern is being gradually removed in the kernel, also with the help of ifnullfree.cocci. Should I maintain it for ipset coding style reasons?
On Sat, 25 Aug 2018, Sven-Haegar Koch wrote: > > On Wed, 22 Aug 2018, Stefano Brivio wrote: > > > > > When check_setname is used in ipset_parse_name_compat(), the > > > 'str' and 'saved' macro arguments point in fact to the same > > > buffer. Free the 'saved' argument only after using it. > > > > > > While at it, remove a useless NULL check on 'saved'. > > > > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com> > > > --- > > > lib/parse.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/lib/parse.c b/lib/parse.c > > > index 9a79ccda796c..4963d519c631 100644 > > > --- a/lib/parse.c > > > +++ b/lib/parse.c > > > @@ -1396,10 +1396,11 @@ ipset_parse_iptimeout(struct ipset_session *session, > > > #define check_setname(str, saved) \ > > > do { \ > > > if (strlen(str) > IPSET_MAXNAMELEN - 1) { \ > > > - if (saved != NULL) \ > > > - free(saved); \ > > > - return syntax_err("setname '%s' is longer than %u characters",\ > > > + int err; \ > > > + err = syntax_err("setname '%s' is longer than %u characters",\ > > > str, IPSET_MAXNAMELEN - 1); \ > > > + free(saved); \ > > > + return err; \ > > > } \ > > > } while (0) > > > > At some places check_setname() is invoked with "saved" explicitly set to > > NULL, so the condition to free cannot be left out. Please resubmit a new > > version of the patch. Thanks! > > Why? > > The free() manpage says: > > If ptr is NULL, no operation is performed. > > So it is ok to always call it on a NULL pointer. The same as with > kfree() in the kernel, no extra NULL check needed before. You are right. I suppose old customs die slowly :-) (in old times it was not proper to call kfree() with a NULL pointer). Best regards, Jozsef - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary
Hi, On Mon, 27 Aug 2018, Stefano Brivio wrote: > On Fri, 24 Aug 2018 21:02:12 +0200 (CEST) > Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote: > > > > @@ -1396,10 +1396,11 @@ ipset_parse_iptimeout(struct ipset_session *session, > > > #define check_setname(str, saved) \ > > > do { \ > > > if (strlen(str) > IPSET_MAXNAMELEN - 1) { \ > > > - if (saved != NULL) \ > > > - free(saved); \ > > > - return syntax_err("setname '%s' is longer than %u characters",\ > > > + int err; \ > > > + err = syntax_err("setname '%s' is longer than %u characters",\ > > > str, IPSET_MAXNAMELEN - 1); \ > > > + free(saved); \ > > > + return err; \ > > > } \ > > > } while (0) > > > > At some places check_setname() is invoked with "saved" explicitly set to > > NULL, so the condition to free cannot be left out. Please resubmit a new > > version of the patch. Thanks! > > I dropped this check because, as Sven-Haegar mentioned, free() on a > NULL pointer has no effect. Both C89 and C99 say: > > The free function causes the space pointed to by ptr to be > deallocated, that is, made available for further allocation. If ptr > is a null pointer, no action occurs. > > As a side note, this pattern is being gradually removed in the kernel, > also with the help of ifnullfree.cocci. > > Should I maintain it for ipset coding style reasons? No, I have just applied your patch. Thanks! Best regards, Jozsef - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary
diff --git a/lib/parse.c b/lib/parse.c index 9a79ccda796c..4963d519c631 100644 --- a/lib/parse.c +++ b/lib/parse.c @@ -1396,10 +1396,11 @@ ipset_parse_iptimeout(struct ipset_session *session, #define check_setname(str, saved) \ do { \ if (strlen(str) > IPSET_MAXNAMELEN - 1) { \ - if (saved != NULL) \ - free(saved); \ - return syntax_err("setname '%s' is longer than %u characters",\ + int err; \ + err = syntax_err("setname '%s' is longer than %u characters",\ str, IPSET_MAXNAMELEN - 1); \ + free(saved); \ + return err; \ } \ } while (0)
When check_setname is used in ipset_parse_name_compat(), the 'str' and 'saved' macro arguments point in fact to the same buffer. Free the 'saved' argument only after using it. While at it, remove a useless NULL check on 'saved'. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> --- lib/parse.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)