[PATCH] hfs: if match_strdup() fails to allocate memory in parse_options(), don't blow up the kernel.

[PATCH] hfs: if match_strdup() fails to allocate memory in parse_options(), don't blow up the kernel.

[Jesper Juhl]

From: Jesper Juhl <jesper.juhl@gmail.com>

The Coverity checker spotted that we don't check the return value of 
match_strdup() in fs/hfs/super.c::parse_options().
This is bad since match_strdup() does a memory allocation internally 
which can fail. If it does fail it'll return NULL and in that case 
we'll pass the NULL pointer on to load_nls() which will eventually 
dereference it - Boom!
Much better to check the return value, fail gracefully and log an 
error message if this happens.
This happens in two different spots. I've made the error logged in 
each location unique so that it'll be obvious in bug reports later 
exactely which one of the two spots got hit (always nice to have 
grep'able error messages that point to a unique location in the 
source).


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 super.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index 32de44e..221e314 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -297,6 +297,10 @@ static int parse_options(char *options, struct hfs_sb_info *hsb)
 				return 0;
 			}
 			p = match_strdup(&args[0]);
+			if (!p) {
+				printk(KERN_ERR "hfs: mem alloc failed in match_strdup()\n");
+				return 0;
+			}
 			hsb->nls_disk = load_nls(p);
 			if (!hsb->nls_disk) {
 				printk(KERN_ERR "hfs: unable to load codepage \"%s\"\n", p);
@@ -311,6 +315,10 @@ static int parse_options(char *options, struct hfs_sb_info *hsb)
 				return 0;
 			}
 			p = match_strdup(&args[0]);
+			if (!p) {
+				printk(KERN_ERR "hfs: memory allocation failed in match_strdup()\n");
+				return 0;
+			}
 			hsb->nls_io = load_nls(p);
 			if (!hsb->nls_io) {
 				printk(KERN_ERR "hfs: unable to load iocharset \"%s\"\n", p);

Re: [PATCH] hfs: if match_strdup() fails to allocate memory in parse_options(), don't blow up the kernel.

[Joe Perches]

On Tue, 2008-04-22 at 23:12 +0200, Jesper Juhl wrote:
> From: Jesper Juhl <jesper.juhl@gmail.com>
> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
> index 32de44e..221e314 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -297,6 +297,10 @@ static int parse_options(char *options, struct hfs_sb_info *hsb)
>  				return 0;
>  			}
>  			p = match_strdup(&args[0]);
> +			if (!p) {
> +				printk(KERN_ERR "hfs: mem alloc failed in match_strdup()\n");
> +				return 0;
> +			}
>  			hsb->nls_disk = load_nls(p);
>  			if (!hsb->nls_disk) {
>  				printk(KERN_ERR "hfs: unable to load codepage \"%s\"\n", p);
> @@ -311,6 +315,10 @@ static int parse_options(char *options, struct hfs_sb_info *hsb)
>  				return 0;
>  			}
>  			p = match_strdup(&args[0]);
> +			if (!p) {
> +				printk(KERN_ERR "hfs: memory allocation failed in match_strdup()\n");
> +				return 0;
> +			}
>  			hsb->nls_io = load_nls(p);
>  			if (!hsb->nls_io) {
>  				printk(KERN_ERR "hfs: unable to load iocharset \"%s\"\n", p);
> 

Using different strings in the printk wastes memory.

Re: [PATCH] hfs: if match_strdup() fails to allocate memory in parse_options(), don't blow up the kernel.

[Jesper Juhl]

On 22/04/2008, Joe Perches <joe@perches.com> wrote:
> On Tue, 2008-04-22 at 23:12 +0200, Jesper Juhl wrote:
>  > From: Jesper Juhl <jesper.juhl@gmail.com>
>
> > diff --git a/fs/hfs/super.c b/fs/hfs/super.c
>  > index 32de44e..221e314 100644
>  > --- a/fs/hfs/super.c
>  > +++ b/fs/hfs/super.c
>  > @@ -297,6 +297,10 @@ static int parse_options(char *options, struct hfs_sb_info *hsb)
>  >                               return 0;
>  >                       }
>  >                       p = match_strdup(&args[0]);
>  > +                     if (!p) {
>  > +                             printk(KERN_ERR "hfs: mem alloc failed in match_strdup()\n");
>  > +                             return 0;
>  > +                     }
>  >                       hsb->nls_disk = load_nls(p);
>  >                       if (!hsb->nls_disk) {
>  >                               printk(KERN_ERR "hfs: unable to load codepage \"%s\"\n", p);
>  > @@ -311,6 +315,10 @@ static int parse_options(char *options, struct hfs_sb_info *hsb)
>  >                               return 0;
>  >                       }
>  >                       p = match_strdup(&args[0]);
>  > +                     if (!p) {
>  > +                             printk(KERN_ERR "hfs: memory allocation failed in match_strdup()\n");
>  > +                             return 0;
>  > +                     }
>  >                       hsb->nls_io = load_nls(p);
>  >                       if (!hsb->nls_io) {
>  >                               printk(KERN_ERR "hfs: unable to load iocharset \"%s\"\n", p);
>  >
>
>
> Using different strings in the printk wastes memory.
>

True, but I personally prefer being able to do  grep "error message"
and hit just one source location over saving a few bytes of memory...
If Roman wants the strings to be identical I can resubmit the patch. Roman?

-- 
Jesper Juhl <jesper.juhl@gmail.com>
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