linux-kernel - Re: [PATCH] Security: key: keyring: fix code style issues

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>] [day] [month] [year] [list]

Date:	Mon, 08 Mar 2010 14:15:26 -0500
From:	James Kosin <jkosin@...comgrp.com>
To:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Security: key: keyring: fix code style issues

See below my comments,

James Kosin

On 3/8/2010 1:20 PM, Chihau Chau wrote:
> From: Chihau Chau <chihau@...il.com>
> 
> This fixes some code style issues like to include <linux/uaccess.h> instead
> <asm/uaccess.h> and to remove some innecessary braces.
> 
> Signed-off-by: Chihau Chau <chihau@...il.com>
> ---
>  security/keys/keyring.c |   11 ++++-------
>  1 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index e814d21..5a44965 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -17,7 +17,7 @@
>  #include <linux/seq_file.h>
>  #include <linux/err.h>
>  #include <keys/keyring-type.h>
> -#include <asm/uaccess.h>
> +#include <linux/uaccess.h>
>  #include "internal.h"
>  
>  /*
> @@ -170,12 +170,10 @@ static void keyring_describe(const struct key *keyring, struct seq_file *m)
>  {
>  	struct keyring_list *klist;
>  
> -	if (keyring->description) {
> +	if (keyring->description)
>  		seq_puts(m, keyring->description);
> -	}
> -	else {
> +	else
>  		seq_puts(m, "[anon]");
> -	}

I'm just going to put my 2-cents in here.

a)  removing the braces doesn't change the functionality here; HOWEVER,
b)  it may introduce a serious BUG later.

The main reason is some people don't look carefully at code and often
add statements inside an if else block ...  and if not caught will
change the meaning of the statements and the order of execution.  The
worst possible case is if the else {} block is matchable to another if
statement above this... then the compiler never complains about the else
without an if test.

I'm okay with changing this to be like below with the {} matching and in
the same format if you like.

>  
>  	rcu_read_lock();
>  	klist = rcu_dereference(keyring->payload.subscriptions);
> @@ -775,8 +773,7 @@ int __key_link(struct key *keyring, struct key *key)
>  		smp_wmb();
>  		klist->nkeys++;
>  		smp_wmb();
> -	}
> -	else {
> +	} else {
>  		/* grow the key list */
>  		max = 4;
>  		if (klist)

--
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/