linux-kernel - Re: [PATCH] Remove double initialization of retval in load_misc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20090410143657.fb81514d.akpm@linux-foundation.org>
Date:	Fri, 10 Apr 2009 14:36:57 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Vitaly Mayatskikh <v.mayatskih@...il.com>
Cc:	kirill@...temov.name, v.mayatskih@...il.com,
	linux-kernel@...r.kernel.org, xemul@...nvz.org
Subject: Re: [PATCH] Remove double initialization of retval in 
 load_misc_binary()

On Wed, 08 Apr 2009 09:56:29 +0200
Vitaly Mayatskikh <v.mayatskih@...il.com> wrote:

> At Wed, 8 Apr 2009 10:32:48 +0300, Kirill A. Shutemov wrote:
> 
> > Probably, better way is removing both 'retval = -ENOEXEC;'__ and initialize it within definition.
> 
> Agree.
> 
> Signed-off-by: Vitaly Mayatskikh <v.mayatskih@...il.com>
> 
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index c4e8353..f10150f 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -109,14 +109,12 @@ static int load_misc_binary(struct linux_binprm *bprm, struct pt_regs *regs)
>  	struct file * interp_file = NULL;
>  	char iname[BINPRM_BUF_SIZE];
>  	char *iname_addr = iname;
> -	int retval;
> +	int retval = -ENOEXEC;
>  	int fd_binary = -1;
>  
> -	retval = -ENOEXEC;
>  	if (!enabled)
>  		goto _ret;
>  
> -	retval = -ENOEXEC;
>  	if (bprm->recursion_depth > BINPRM_MAX_RECURSION)
>  		goto _ret;
>  

I don't think this is really a desirable change.

What the existing code is effectively doing is:

	if (!enabled) {
		retval = -ENOEXEC;
		goto _ret;
	}

	if (bprm->recursion_depth > BINPRM_MAX_RECURSION) {
		retval = -ENOEXEC;
		goto _ret;
	}

only it's doing this via an odd coding trick which used to (and might
still) generate more efficient code.

Those two pieces of code are logically separate things and it's just by
coincidence that they both happen to use the same errno.

Your proposed patch will create a linkage between those two unique
pieces of code which wasn't there previously.  It's a snmall thing.

Plus the compiler will surely remove one of the loads anyway, so this
is purely a source-level change.  And I don't think it made the source
easier to read and maintain!

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/