Re: [PATCH] security: Use || instead of | for boolean expressions

From: Rui Teng
Date: Thu Jun 02 2016 - 12:53:25 EST

On 6/2/16 10:13 PM, Serge E. Hallyn wrote:
On Wed, Jun 01, 2016 at 02:03:02PM +0800, Rui Teng wrote:
Sparse spits out the following warning:
security/commoncap.c:989:41: warning: dubious: !x | y

Bitwise and logical are equivalent here, but logical was intended.
Replacing the bit-wise '|' with the boolean '||' silences the sparse warning.


this looks ok, but I'm worried by

The generated code for both cases is the same.

That cannot be. The logical result should be the same, but the
generated code cannot be.

Thanks for cc:ing the author.

I tried to write a sample code to verify it before. Both || and | will
generate the same assembly code.

For example, compiling following code with "gcc -O2 -S main.c", and
replacing || with | can generate the same assembly code.

- main.c ------------

int parse(int a, int b, int c)
if (a || b || c)
return 1;
return 0;

Of cause, it is only a sample on x86, but even if the generated code is
not the same, the logical will be better than bitwise.
Because (a || b || c) means (a != 0 || b != 0 || c != 0), once a != 0,
the whole expression will be true(short-circuit evaluation).
and (a | b | c) means calculate the bitwise first and check the result
in the end. And since the args are all integer, there is no need to
avoid any short-circuit.

I'm cc:ing Andy as this code came in with his patch. Is there an
actual reason for having used bitwise here?


Signed-off-by: Rui Teng <rui.teng@xxxxxxxxxxxxxxxxxx>
security/commoncap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index e7fadde..8f6fb24 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -976,7 +976,7 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,

- if (arg3 | arg4 | arg5)
+ if (arg3 || arg4 || arg5)
return -EINVAL;

new = prepare_creds();
@@ -986,7 +986,7 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
return commit_creds(new);

- if (((!cap_valid(arg3)) | arg4 | arg5))
+ if (((!cap_valid(arg3)) || arg4 || arg5))
return -EINVAL;

if (arg2 == PR_CAP_AMBIENT_IS_SET) {