There are paths through the irda_setsockopt() function where we return and
may or may not have allocated a new ias_obj but in any case have not used
it for anything yet and we end up leaking memory.
As far as I can tell, in the case where we didn't allocate a new ias_ob
but simply were planning to use one already available then we should not
free it before returning. But when we have allocated a brand new ias_obj
but have not yet used it or put it on any lists etc and then return,
that's a memory leak.
There are two cases:
1)
switch (optname) {
case IRLMP_IAS_SET:
...
if(ias_obj == (struct ias_object *) NULL) {
/* Create a new object */
--[alloc]--> ias_obj = irias_new_object(ias_opt->irda_class_name,
jiffies);
...
switch(ias_opt->irda_attrib_type) {
case IAS_OCT_SEQ:
/* Check length */
if(ias_opt->attribute.irda_attrib_octet_seq.len >
IAS_MAX_OCTET_STRING) {
kfree(ias_opt);
--[leak]--> return -EINVAL;
...
}
irias_insert_object(ias_obj);
The allocated object isn't referenced at all until we get outside the
inner switch, so clearly we leak it (if we took the path that allocated it
that is).
2)
The second case is the same as the above, except it's the default: case in
the inner switch instead of case IAS_OCT_SEQ:
default :
kfree(ias_opt);
return -EINVAL;
The way I propose to fix this is with a new variable that keeps track of
whether or not we found an existing ias_obj to use or if we took the
allocation path, then use that variable to determine if we should free
ias_obj before returning from the function.
I'm not very intimate with this code, so if there's a better solution I'd
very much like to hear it. It's also entirely possible that someone with
more knowledge of this code can prove that these cases can't actually ever
happen - if that's the case then please let me know.
This patch is meant to be applied on top of
[PATCH 1/2] irda: return -ENOMEM upon failure to allocate new ias_obj
The Coverity checker gets credit for pointing its finger towards this.
Signed-off-by: Jesper Juhl <[EMAIL PROTECTED]>
---
af_irda.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/net/irda/af_irda.c b/net/irda/af_irda.c
index e33f0a5..352e8a7 100644
--- a/net/irda/af_irda.c
+++ b/net/irda/af_irda.c
@@ -1824,7 +1824,8 @@ static int irda_setsockopt(struct socket *sock, int
level, int optname,
struct irda_sock *self = irda_sk(sk);
struct irda_ias_set *ias_opt;
struct ias_object *ias_obj;
- struct ias_attrib * ias_attr; /* Attribute in IAS object */
+ struct ias_attrib *ias_attr; /* Attribute in IAS object */
+ int alloc_new_obj = 0;
int opt;
IRDA_DEBUG(2, "%s(%p)\n", __FUNCTION__, self);
@@ -1885,6 +1886,7 @@ static int irda_setsockopt(struct socket *sock, int
level, int optname,
kfree(ias_opt);
return -ENOMEM;
}
+ alloc_new_obj = 1;
}
/* Do we have the attribute already ? */
@@ -1908,6 +1910,8 @@ static int irda_setsockopt(struct socket *sock, int
level, int optname,
if(ias_opt->attribute.irda_attrib_octet_seq.len >
IAS_MAX_OCTET_STRING) {
kfree(ias_opt);
+ if (alloc_new_obj)
+ kfree(ias_obj);
return -EINVAL;
}
/* Add an octet sequence attribute */
@@ -1936,6 +1940,8 @@ static int irda_setsockopt(struct socket *sock, int
level, int optname,
break;
default :
kfree(ias_opt);
+ if (alloc_new_obj)
+ kfree(ias_obj);
return -EINVAL;
}
irias_insert_object(ias_obj);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
