* [PATCH ethtool 1/2] netlink: return -ENOMEM when calloc fails
@ 2020-09-24 19:27 Ivan Vecera
2020-09-24 19:27 ` [PATCH ethtool 2/2] netlink: fix memory leak Ivan Vecera
2020-09-28 15:44 ` [PATCH ethtool 1/2] netlink: return -ENOMEM when calloc fails Michal Kubecek
0 siblings, 2 replies; 5+ messages in thread
From: Ivan Vecera @ 2020-09-24 19:27 UTC (permalink / raw)
To: netdev; +Cc: Michal Kubecek
Fixes: f2c17e107900 ("netlink: add netlink handler for gfeatures (-k)")
Cc: Michal Kubecek <mkubecek@suse.cz>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
netlink/features.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/netlink/features.c b/netlink/features.c
index 3f1240437350..b2cf57eea660 100644
--- a/netlink/features.c
+++ b/netlink/features.c
@@ -112,16 +112,17 @@ int dump_features(const struct nlattr *const *tb,
unsigned int *feature_flags = NULL;
struct feature_results results;
unsigned int i, j;
- int ret;
+ int ret = 0;
ret = prepare_feature_results(tb, &results);
if (ret < 0)
return -EFAULT;
- ret = -ENOMEM;
feature_flags = calloc(results.count, sizeof(feature_flags[0]));
- if (!feature_flags)
+ if (!feature_flags) {
+ ret = -ENOMEM;
goto out_free;
+ }
/* map netdev features to legacy flags */
for (i = 0; i < results.count; i++) {
@@ -184,7 +185,7 @@ int dump_features(const struct nlattr *const *tb,
out_free:
free(feature_flags);
- return 0;
+ return ret;
}
int features_reply_cb(const struct nlmsghdr *nlhdr, void *data)
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH ethtool 2/2] netlink: fix memory leak
2020-09-24 19:27 [PATCH ethtool 1/2] netlink: return -ENOMEM when calloc fails Ivan Vecera
@ 2020-09-24 19:27 ` Ivan Vecera
2020-09-28 15:37 ` Michal Kubecek
2020-09-28 15:44 ` [PATCH ethtool 1/2] netlink: return -ENOMEM when calloc fails Michal Kubecek
1 sibling, 1 reply; 5+ messages in thread
From: Ivan Vecera @ 2020-09-24 19:27 UTC (permalink / raw)
To: netdev; +Cc: Michal Kubecek
Potentially allocated memory allocated for mask is not freed when
the allocation for value fails.
Fixes: 81a30f416ec7 ("netlink: add bitset command line parser handlers")
Cc: Michal Kubecek <mkubecek@suse.cz>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
netlink/parser.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/netlink/parser.c b/netlink/parser.c
index c5a368a65a7a..3b25f5d5a88e 100644
--- a/netlink/parser.c
+++ b/netlink/parser.c
@@ -630,8 +630,10 @@ static int parse_numeric_bitset(struct nl_context *nlctx, uint16_t type,
}
value = calloc(nwords, sizeof(uint32_t));
- if (!value)
+ if (!value) {
+ free(mask);
return -ENOMEM;
+ }
ret = __parse_num_string(arg, len1, value, force_hex1);
if (ret < 0) {
parser_err_invalid_value(nlctx, arg);
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH ethtool 2/2] netlink: fix memory leak
2020-09-24 19:27 ` [PATCH ethtool 2/2] netlink: fix memory leak Ivan Vecera
@ 2020-09-28 15:37 ` Michal Kubecek
0 siblings, 0 replies; 5+ messages in thread
From: Michal Kubecek @ 2020-09-28 15:37 UTC (permalink / raw)
To: Ivan Vecera; +Cc: netdev
[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]
On Thu, Sep 24, 2020 at 09:27:58PM +0200, Ivan Vecera wrote:
> Potentially allocated memory allocated for mask is not freed when
> the allocation for value fails.
>
> Fixes: 81a30f416ec7 ("netlink: add bitset command line parser handlers")
>
> Cc: Michal Kubecek <mkubecek@suse.cz>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
Applied, thank you.
Michal
> netlink/parser.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/netlink/parser.c b/netlink/parser.c
> index c5a368a65a7a..3b25f5d5a88e 100644
> --- a/netlink/parser.c
> +++ b/netlink/parser.c
> @@ -630,8 +630,10 @@ static int parse_numeric_bitset(struct nl_context *nlctx, uint16_t type,
> }
>
> value = calloc(nwords, sizeof(uint32_t));
> - if (!value)
> + if (!value) {
> + free(mask);
> return -ENOMEM;
> + }
> ret = __parse_num_string(arg, len1, value, force_hex1);
> if (ret < 0) {
> parser_err_invalid_value(nlctx, arg);
> --
> 2.26.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH ethtool 1/2] netlink: return -ENOMEM when calloc fails
2020-09-24 19:27 [PATCH ethtool 1/2] netlink: return -ENOMEM when calloc fails Ivan Vecera
2020-09-24 19:27 ` [PATCH ethtool 2/2] netlink: fix memory leak Ivan Vecera
@ 2020-09-28 15:44 ` Michal Kubecek
2020-09-28 19:21 ` Ivan Vecera
1 sibling, 1 reply; 5+ messages in thread
From: Michal Kubecek @ 2020-09-28 15:44 UTC (permalink / raw)
To: Ivan Vecera; +Cc: netdev
[-- Attachment #1: Type: text/plain, Size: 1663 bytes --]
On Thu, Sep 24, 2020 at 09:27:57PM +0200, Ivan Vecera wrote:
> Fixes: f2c17e107900 ("netlink: add netlink handler for gfeatures (-k)")
>
> Cc: Michal Kubecek <mkubecek@suse.cz>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
> netlink/features.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/netlink/features.c b/netlink/features.c
> index 3f1240437350..b2cf57eea660 100644
> --- a/netlink/features.c
> +++ b/netlink/features.c
> @@ -112,16 +112,17 @@ int dump_features(const struct nlattr *const *tb,
> unsigned int *feature_flags = NULL;
> struct feature_results results;
> unsigned int i, j;
> - int ret;
> + int ret = 0;
>
> ret = prepare_feature_results(tb, &results);
> if (ret < 0)
> return -EFAULT;
>
> - ret = -ENOMEM;
> feature_flags = calloc(results.count, sizeof(feature_flags[0]));
> - if (!feature_flags)
> + if (!feature_flags) {
> + ret = -ENOMEM;
> goto out_free;
> + }
>
> /* map netdev features to legacy flags */
> for (i = 0; i < results.count; i++) {
> @@ -184,7 +185,7 @@ int dump_features(const struct nlattr *const *tb,
>
> out_free:
> free(feature_flags);
> - return 0;
> + return ret;
> }
>
> int features_reply_cb(const struct nlmsghdr *nlhdr, void *data)
> --
> 2.26.2
The patch is correct but relying on ret staying zero through the whole
function is rather fragile (it could break when adding more checks in
the future) and it also isn't consistent with the way this is done in
other functions.
AFAICS you could omit the first hunk and just add "ret = 0" above the
out_free label.
Michal
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH ethtool 1/2] netlink: return -ENOMEM when calloc fails
2020-09-28 15:44 ` [PATCH ethtool 1/2] netlink: return -ENOMEM when calloc fails Michal Kubecek
@ 2020-09-28 19:21 ` Ivan Vecera
0 siblings, 0 replies; 5+ messages in thread
From: Ivan Vecera @ 2020-09-28 19:21 UTC (permalink / raw)
To: Michal Kubecek; +Cc: netdev
On Mon, 28 Sep 2020 17:44:55 +0200
Michal Kubecek <mkubecek@suse.cz> wrote:
> On Thu, Sep 24, 2020 at 09:27:57PM +0200, Ivan Vecera wrote:
> > Fixes: f2c17e107900 ("netlink: add netlink handler for gfeatures (-k)")
> >
> > Cc: Michal Kubecek <mkubecek@suse.cz>
> > Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> > ---
> > netlink/features.c | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/netlink/features.c b/netlink/features.c
> > index 3f1240437350..b2cf57eea660 100644
> > --- a/netlink/features.c
> > +++ b/netlink/features.c
> > @@ -112,16 +112,17 @@ int dump_features(const struct nlattr *const *tb,
> > unsigned int *feature_flags = NULL;
> > struct feature_results results;
> > unsigned int i, j;
> > - int ret;
> > + int ret = 0;
> >
> > ret = prepare_feature_results(tb, &results);
> > if (ret < 0)
> > return -EFAULT;
> >
> > - ret = -ENOMEM;
> > feature_flags = calloc(results.count, sizeof(feature_flags[0]));
> > - if (!feature_flags)
> > + if (!feature_flags) {
> > + ret = -ENOMEM;
> > goto out_free;
> > + }
> >
> > /* map netdev features to legacy flags */
> > for (i = 0; i < results.count; i++) {
> > @@ -184,7 +185,7 @@ int dump_features(const struct nlattr *const *tb,
> >
> > out_free:
> > free(feature_flags);
> > - return 0;
> > + return ret;
> > }
> >
> > int features_reply_cb(const struct nlmsghdr *nlhdr, void *data)
> > --
> > 2.26.2
>
> The patch is correct but relying on ret staying zero through the whole
> function is rather fragile (it could break when adding more checks in
> the future) and it also isn't consistent with the way this is done in
> other functions.
>
> AFAICS you could omit the first hunk and just add "ret = 0" above the
> out_free label.
>
> Michal
OK, will do.
Ivan
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-28 19:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-24 19:27 [PATCH ethtool 1/2] netlink: return -ENOMEM when calloc fails Ivan Vecera
2020-09-24 19:27 ` [PATCH ethtool 2/2] netlink: fix memory leak Ivan Vecera
2020-09-28 15:37 ` Michal Kubecek
2020-09-28 15:44 ` [PATCH ethtool 1/2] netlink: return -ENOMEM when calloc fails Michal Kubecek
2020-09-28 19:21 ` Ivan Vecera
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).