linux-kernel - Re: [PATCH 5/5] Fixes a potential bug in android/logger.c

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

Date:	Thu, 02 Aug 2012 09:50:44 +1000
From:	Ryan Mallon <rmallon@...il.com>
To:	Cruz Julian Bishop <cruzjbishop@...il.com>
CC:	greg@...ah.com, swetland@...gle.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] Fixes a potential bug in android/logger.c

On 01/08/12 14:54, Cruz Julian Bishop wrote:
> Previously, when calling is_between(a, b, c), the calculation was wrong.
> It counted C as between A and B if C was equal to B, but not A.
> 
> Example of this are:
> 
> is_between(1, 10, 10) = 1 (Expected: 0)
> is_between(1, 10, 1) = 0 (Expected: 0)
> is_between(20, 10, 10) = 1 (Expected: 0)
> 
> And so on and so forth.
> 
> Obviously, ten is not a number between one and ten - only two to eight are, so I made this patch :)

Is nine not a number between one and ten? :-p.

The question with a patch like this is whether the function's
documentation, which says it returns 1 if a < c < b is wrong, or whether
the implementation, which does a < c <= b is wrong. If the documentation
is wrong, and something is relying on the current implementation, then
this patch might actually break things.

> Signed-off-by: Cruz Julian Bishop <cruzjbishop@...il.com>
> ---
>  drivers/staging/android/logger.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index 226d8b5..925df5c 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -298,11 +298,11 @@ static inline int is_between(size_t a, size_t b, size_t c)
>  {
>  	if (a < b) {
>  		/* is c between a and b? */
> -		if (a < c && c <= b)
> +		if (a < c && c < b)
>  			return 1;
>  	} else {
>  		/* is c outside of b through a? */
> -		if (c <= b || a < c)
> +		if (c < b || a < c)
>  			return 1;
>  	}

A couple of other improvements could be done here. The function should
really return bool, inline is unnecessary (the compiler is smart enough
to do that for us), and we can simplify the logic a bit too:

static bool is_between(size_t a, size_t b, size_t c)
{
	if (a < b)
		swap(a, b);

	return (a < c && c < b);
}

~Ryan




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