There are two code paths in drivers/mtd/devices/phram.c::phram_setup() that
will leak memory.
Memory is allocated to the variable 'name' with kmalloc() by the
parse_name() function, but if we leave by way of the parse_err() macro,
then that memory is never kfree()'d, nor is it ever used with
register_device() so it won't be freed later either - leak.
Found by the Coverity checker as #593 - simple fix below.
Signed-off-by: Jesper Juhl <[email protected]>
---
drivers/mtd/devices/phram.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
--- linux-2.6.17-rc4-git2-orig/drivers/mtd/devices/phram.c 2006-03-20 06:53:29.000000000 +0100
+++ linux-2.6.17-rc4-git2/drivers/mtd/devices/phram.c 2006-05-14 01:05:27.000000000 +0200
@@ -266,12 +266,16 @@ static int phram_setup(const char *val,
return 0;
ret = parse_num32(&start, token[1]);
- if (ret)
+ if (ret) {
+ kfree(name);
parse_err("illegal start address\n");
+ }
ret = parse_num32(&len, token[2]);
- if (ret)
+ if (ret) {
+ kfree(name);
parse_err("illegal device length\n");
+ }
register_device(name, start, len);
On Sun, 2006-05-14 at 01:07 +0200, Jesper Juhl wrote:
> There are two code paths in drivers/mtd/devices/phram.c::phram_setup() that
> will leak memory.
> Memory is allocated to the variable 'name' with kmalloc() by the
> parse_name() function, but if we leave by way of the parse_err() macro,
> then that memory is never kfree()'d, nor is it ever used with
> register_device() so it won't be freed later either - leak.
>
> Found by the Coverity checker as #593 - simple fix below.
Applied; thanks. Please Cc me and/or [email protected] on
MTD patches.
(Ew. The parse_err() macro contains a 'return'. Who do I slap for that?)
--
dwmw2
On 14/05/06, David Woodhouse <[email protected]> wrote:
> On Sun, 2006-05-14 at 01:07 +0200, Jesper Juhl wrote:
> > There are two code paths in drivers/mtd/devices/phram.c::phram_setup() that
> > will leak memory.
> > Memory is allocated to the variable 'name' with kmalloc() by the
> > parse_name() function, but if we leave by way of the parse_err() macro,
> > then that memory is never kfree()'d, nor is it ever used with
> > register_device() so it won't be freed later either - leak.
> >
> > Found by the Coverity checker as #593 - simple fix below.
>
> Applied; thanks. Please Cc me and/or [email protected] on
> MTD patches.
>
Sure thing, will do. The same problem exists in
drivers/mtd/devices/block2mtd.c, I'm cooking up a patch for that one
as we speak.
> (Ew. The parse_err() macro contains a 'return'. Who do I slap for that?)
>
Want me to fix the macro and the users of it?
--
Jesper Juhl <[email protected]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
On Sun, 2006-05-14 at 01:34 +0200, Jesper Juhl wrote:
> Sure thing, will do. The same problem exists in
> drivers/mtd/devices/block2mtd.c, I'm cooking up a patch for that one
> as we speak.
OK, thanks.
> > (Ew. The parse_err() macro contains a 'return'. Who do I slap for
> > that?)
>
> Want me to fix the macro and the users of it?
Well, the exclamation was intended to provoke Jörn or Jochen into fixing
it for themselves, but if you get there first that'd be great too :)
--
dwmw2
On Sun, 14 May 2006 00:40:42 +0100, David Woodhouse wrote:
> >
> > Want me to fix the macro and the users of it?
>
> Well, the exclamation was intended to provoke J?rn or Jochen into fixing
> it for themselves, but if you get there first that'd be great too :)
The only question is: does it make the code better? The code has
seven printk/return combinations. Each of them would chew up 2 more
lines without the macro. So phram_setup would grow from 44 to 58
lines, not nice either.
What bugs me more is the hidden allocation in parse_name. Looks like
that should return a pointer or ERR_PTR(foo), not an int.
J?rn
--
There are two ways of constructing a software design: one way is to make
it so simple that there are obviously no deficiencies, and the other is
to make it so complicated that there are no obvious deficiencies.
-- C. A. R. Hoare
On Sun, 2006-05-14 at 08:37 +0200, Jörn Engel wrote:
> The only question is: does it make the code better? The code has
> seven printk/return combinations. Each of them would chew up 2 more
> lines without the macro. So phram_setup would grow from 44 to 58
> lines, not nice either.
How about this then...
diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
index e09e416..fd2d43f 100644
--- a/drivers/mtd/devices/phram.c
+++ b/drivers/mtd/devices/phram.c
@@ -187,36 +187,41 @@ static int ustrtoul(const char *cp, char
return result;
}
-static int parse_num32(uint32_t *num32, const char *token)
+static int parse_num32(uint32_t *num32, const char *token, char *name)
{
char *endp;
unsigned long n;
n = ustrtoul(token, &endp, 0);
- if (*endp)
+ if (*endp) {
+ ERROR("Illegal %s\n", name);
return -EINVAL;
+ }
*num32 = n;
return 0;
}
-static int parse_name(char **pname, const char *token)
+static char *parse_name(const char *token)
{
size_t len;
char *name;
len = strlen(token) + 1;
- if (len > 64)
- return -ENOSPC;
+ if (len > 64) {
+ ERROR("name too long");
+ return ERR_PTR(-ENOSPC);
+ }
name = kmalloc(len, GFP_KERNEL);
- if (!name)
- return -ENOMEM;
+ if (!name) {
+ ERROR("out of memory");
+ return ERR_PTR(-ENOMEM);
+ }
strcpy(name, token);
- *pname = name;
- return 0;
+ return name;
}
@@ -228,11 +233,6 @@ static inline void kill_final_newline(ch
}
-#define parse_err(fmt, args...) do { \
- ERROR(fmt , ## args); \
- return 0; \
-} while (0)
-
static int phram_setup(const char *val, struct kernel_param *kp)
{
char buf[64+12+12], *str = buf;
@@ -242,8 +242,10 @@ static int phram_setup(const char *val,
uint32_t len;
int i, ret;
- if (strnlen(val, sizeof(buf)) >= sizeof(buf))
- parse_err("parameter too long\n");
+ if (strnlen(val, sizeof(buf)) >= sizeof(buf)) {
+ ERROR("parameter too long\n");
+ return -EINVAL;
+ }
strcpy(str, val);
kill_final_newline(str);
@@ -251,30 +253,24 @@ static int phram_setup(const char *val,
for (i=0; i<3; i++)
token[i] = strsep(&str, ",");
- if (str)
- parse_err("too many arguments\n");
-
- if (!token[2])
- parse_err("not enough arguments\n");
-
- ret = parse_name(&name, token[0]);
- if (ret == -ENOMEM)
- parse_err("out of memory\n");
- if (ret == -ENOSPC)
- parse_err("name too long\n");
- if (ret)
- return 0;
+ if (str) {
+ ERROR("too many arguments\n");
+ return -EINVAL;
+ }
- ret = parse_num32(&start, token[1]);
- if (ret) {
- kfree(name);
- parse_err("illegal start address\n");
+ if (!token[2]) {
+ ERROR("not enough arguments\n");
+ return -EINVAL;
}
- ret = parse_num32(&len, token[2]);
- if (ret) {
+ name = prase_name(token[0]);
+ if (IS_ERR(name))
+ return PTR_ERR(name);
+
+ if ((ret = parse_num32(&start, token[1], "start address")) ||
+ (ret = parse_num32(&len, token[2], "device length"))) {
kfree(name);
- parse_err("illegal device length\n");
+ return ret;
}
register_device(name, start, len);
--
dwmw2
On Sun, 14 May 2006 12:03:49 +0100, David Woodhouse wrote:
> On Sun, 2006-05-14 at 08:37 +0200, J?rn Engel wrote:
> > The only question is: does it make the code better? The code has
> > seven printk/return combinations. Each of them would chew up 2 more
> > lines without the macro. So phram_setup would grow from 44 to 58
> > lines, not nice either.
>
> How about this then...
Moving the printk into leaf functions? My plan still is to collect a
bunch of those and put somewhere in lib/. So maybe that's a bad idea.
Apart from that, I don't have a strong opinion. The macro sucks.
Long functions suck. Looks about the same to me. If you care enough
to get rid of the macro, go ahead.
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index e09e416..fd2d43f 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -187,36 +187,41 @@ static int ustrtoul(const char *cp, char
> return result;
> }
>
> -static int parse_num32(uint32_t *num32, const char *token)
> +static int parse_num32(uint32_t *num32, const char *token, char *name)
> {
> char *endp;
> unsigned long n;
>
> n = ustrtoul(token, &endp, 0);
> - if (*endp)
> + if (*endp) {
> + ERROR("Illegal %s\n", name);
> return -EINVAL;
> + }
>
> *num32 = n;
> return 0;
> }
>
> -static int parse_name(char **pname, const char *token)
> +static char *parse_name(const char *token)
> {
> size_t len;
> char *name;
>
> len = strlen(token) + 1;
> - if (len > 64)
> - return -ENOSPC;
> + if (len > 64) {
> + ERROR("name too long");
> + return ERR_PTR(-ENOSPC);
> + }
>
> name = kmalloc(len, GFP_KERNEL);
> - if (!name)
> - return -ENOMEM;
> + if (!name) {
> + ERROR("out of memory");
> + return ERR_PTR(-ENOMEM);
> + }
>
> strcpy(name, token);
>
> - *pname = name;
> - return 0;
> + return name;
> }
>
>
> @@ -228,11 +233,6 @@ static inline void kill_final_newline(ch
> }
>
>
> -#define parse_err(fmt, args...) do { \
> - ERROR(fmt , ## args); \
> - return 0; \
> -} while (0)
> -
> static int phram_setup(const char *val, struct kernel_param *kp)
> {
> char buf[64+12+12], *str = buf;
> @@ -242,8 +242,10 @@ static int phram_setup(const char *val,
> uint32_t len;
> int i, ret;
>
> - if (strnlen(val, sizeof(buf)) >= sizeof(buf))
> - parse_err("parameter too long\n");
> + if (strnlen(val, sizeof(buf)) >= sizeof(buf)) {
> + ERROR("parameter too long\n");
> + return -EINVAL;
> + }
>
> strcpy(str, val);
> kill_final_newline(str);
> @@ -251,30 +253,24 @@ static int phram_setup(const char *val,
> for (i=0; i<3; i++)
> token[i] = strsep(&str, ",");
>
> - if (str)
> - parse_err("too many arguments\n");
> -
> - if (!token[2])
> - parse_err("not enough arguments\n");
> -
> - ret = parse_name(&name, token[0]);
> - if (ret == -ENOMEM)
> - parse_err("out of memory\n");
> - if (ret == -ENOSPC)
> - parse_err("name too long\n");
> - if (ret)
> - return 0;
> + if (str) {
> + ERROR("too many arguments\n");
> + return -EINVAL;
> + }
>
> - ret = parse_num32(&start, token[1]);
> - if (ret) {
> - kfree(name);
> - parse_err("illegal start address\n");
> + if (!token[2]) {
> + ERROR("not enough arguments\n");
> + return -EINVAL;
> }
>
> - ret = parse_num32(&len, token[2]);
> - if (ret) {
> + name = prase_name(token[0]);
> + if (IS_ERR(name))
> + return PTR_ERR(name);
> +
> + if ((ret = parse_num32(&start, token[1], "start address")) ||
> + (ret = parse_num32(&len, token[2], "device length"))) {
> kfree(name);
> - parse_err("illegal device length\n");
> + return ret;
> }
>
> register_device(name, start, len);
>
> --
> dwmw2
J?rn
--
Correctness comes second.
Features come third.
Performance comes last.
Maintainability is needed for all of them.
Hi J?rn,
On Sunday, 14. May 2006 15:21, J?rn Engel wrote:
> On Sun, 14 May 2006 12:03:49 +0100, David Woodhouse wrote:
> > On Sun, 2006-05-14 at 08:37 +0200, J?rn Engel wrote:
> > > The only question is: does it make the code better? The code has
> > > seven printk/return combinations. Each of them would chew up 2 more
> > > lines without the macro. So phram_setup would grow from 44 to 58
> > > lines, not nice either.
> >
> > How about this then...
>
> Moving the printk into leaf functions? My plan still is to collect a
> bunch of those and put somewhere in lib/. So maybe that's a bad idea.
That has been done already. One is kstrdup() from <linux/string.h>
implementation in mm/util.c . So duplication can go.
parse_num32 is implemented as memparse() from
<linux/kernel.h> already, code is in lib/cmdline.c
It only misses the SI prefix stuff (kiBi and such).
Maybe SI units could be added to memparse() in
lib/cmdline.c and all those reimplementations ripped of drivers/mtd/*/* ?
diff --git a/lib/cmdline.c b/lib/cmdline.c
index 0331ed8..d13c580 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -108,6 +108,8 @@ unsigned long long memparse (char *ptr,
case 'k':
ret <<= 10;
(*retptr)++;
+ if ((**retptr) == 'i')
+ (*retptr)++;
default:
break;
}
> The macro sucks.
Agreed stronly! It is also against Documentation/CodingStyle, Chapter 11
No worries, error handling always clutters up our nice code.
The only accepted way around this in Linux is "goto error_label",
so we can see the algorithm and read up on error handling at the
end of a function, after the algorithm.
Regards
Ingo Oeser
On Sun, 14 May 2006 18:37:15 +0200, Ingo Oeser wrote:
> On Sunday, 14. May 2006 15:21, J?rn Engel wrote:
> >
> > Moving the printk into leaf functions? My plan still is to collect a
> > bunch of those and put somewhere in lib/. So maybe that's a bad idea.
>
> That has been done already. One is kstrdup() from <linux/string.h>
> implementation in mm/util.c . So duplication can go.
Good point.
> parse_num32 is implemented as memparse() from
> <linux/kernel.h> already, code is in lib/cmdline.c
>
> It only misses the SI prefix stuff (kiBi and such).
>
> Maybe SI units could be added to memparse() in
> lib/cmdline.c and all those reimplementations ripped of drivers/mtd/*/* ?
Not quite as simple. My version is does two things. First, it
accepts "ki" and second, it does not accept "k". The latter is
important, because "k" means 1000, not 1024.
There are existing users that still use the historical interpretation,
so I guess we need both variants.
> > The macro sucks.
> Agreed stronly! It is also against Documentation/CodingStyle, Chapter 11
>
> No worries, error handling always clutters up our nice code.
>
> The only accepted way around this in Linux is "goto error_label",
> so we can see the algorithm and read up on error handling at the
> end of a function, after the algorithm.
Doesn't work here. Every single error gives a different message. Oh
well, let's go for the longer function.
J?rn
--
You can't tell where a program is going to spend its time. Bottlenecks
occur in surprising places, so don't try to second guess and put in a
speed hack until you've proven that's where the bottleneck is.
-- Rob Pike