Bug 21329

just a little style police..

On Sun, Sep 17, 2000 at 01:17:31PM -0400, Jonathan Chen wrote:

>  	case VM86_INTCALL: {
>  		struct vm86_intcall_args sa;
>  
> -		if (error = copyin(ua.sub_args, &sa, sizeof(sa)))
> +		if (p->p_cred->pc_ucred->cr_uid != 0) return EPERM;
> +

		if (p->p_cred->pc_ucred->cr_uid != 0)
			return EPERM;

> +		if ((error = copyin(ua.sub_args, &sa, sizeof(sa))))

Gratuitous parens. Optionally, add an explicit check here (ie, != 0 or something)
in which case the parens actually make sense.

>  			return (error);
> -		if (error = vm86_intcall(sa.intnum, &sa.vmf))
> +		if ((error = vm86_intcall(sa.intnum, &sa.vmf)))

See previous.

>  			return (error);
>  		error = copyout(&sa, ua.sub_args, sizeof(sa));
>  		}
>  		break;
> -#endif

Naturally, I can't give you technical feedback, just style(9) fascism. :->

-- 
Bill Fumerola - Network Architect, BOFH / Chimes, Inc.
                billf@chimesnet.com / billf@FreeBSD.org

On Sun, Sep 17, 2000 at 01:31:14PM -0400, Bill Fumerola wrote:
> just a little style police..
> 
> > +		if ((error = copyin(ua.sub_args, &sa, sizeof(sa))))
> 
> Gratuitous parens. Optionally, add an explicit check here (ie, != 0 or something)
> in which case the parens actually make sense.

Yes, I'm aware of what style() says, but gcc -Wall, in its brokenness,
"suggests parentheses around assignment used as truth value".  I have no
clue why gcc would think extra parentheses would be cool there, but I
decided to silence gcc instead of following style() (and didn't think of
the != 0 bit).  Anyway, this should be better:

Index: sys/i386/i386/vm86.c
===================================================================
RCS file: /export/ncvs/src/sys/i386/i386/vm86.c,v
retrieving revision 1.31
diff -u -r1.31 vm86.c
--- sys/i386/i386/vm86.c	1999/10/29 18:08:35	1.31
+++ sys/i386/i386/vm86.c	2000/09/17 17:37:08
@@ -701,18 +701,18 @@
 		}
 		break;
 
-#if 0
 	case VM86_INTCALL: {
 		struct vm86_intcall_args sa;
 
-		if (error = copyin(ua.sub_args, &sa, sizeof(sa)))
+		if (p->p_cred->pc_ucred->cr_uid != 0) return EPERM;
+
+		if (0 != (error = copyin(ua.sub_args, &sa, sizeof(sa))))
 			return (error);
-		if (error = vm86_intcall(sa.intnum, &sa.vmf))
+		if (0 != (error = vm86_intcall(sa.intnum, &sa.vmf)))
 			return (error);
 		error = copyout(&sa, ua.sub_args, sizeof(sa));
 		}
 		break;
-#endif
 
 	default:
 		error = EINVAL;

-- 
    (o_ 1-2-1-2-1-2-1-2-1-2-1-2-1-2-1-2-1-2-1-2-1-2-1-2-1-2-1-2-1-2 _o)
 \\\_\            Jonathan Chen              jon@spock.org           /_///
 <____)  No electrons were harmed during production of this message (____>
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

On Sun, Sep 17, 2000 at 01:41:10PM -0400, Jonathan Chen wrote:

> Yes, I'm aware of what style() says, but gcc -Wall, in its brokenness,
> "suggests parentheses around assignment used as truth value".  I have no
> clue why gcc would think extra parentheses would be cool there, but I
> decided to silence gcc instead of following style() (and didn't think of
> the != 0 bit).  Anyway, this should be better:

I think it's gcc's roundabout way of encouraging the practice of explicit
checks. This makes even more sense when you get into things returning NULL
and abusing the if(!something_that_might_be_null) case.

> 
[...]
> -		if (error = copyin(ua.sub_args, &sa, sizeof(sa)))
> +		if (p->p_cred->pc_ucred->cr_uid != 0) return EPERM;
> +
> +		if (0 != (error = copyin(ua.sub_args, &sa, sizeof(sa))))
>  			return (error);
> -		if (error = vm86_intcall(sa.intnum, &sa.vmf))
> +		if (0 != (error = vm86_intcall(sa.intnum, &sa.vmf)))
>  			return (error);
>  		error = copyout(&sa, ua.sub_args, sizeof(sa));
>  		}
>  		break;
> -#endif

#1. You still need the \nreturn EPERM;
#2. I'd put the != 0 after the call.

-- 
Bill Fumerola - Network Architect, BOFH / Chimes, Inc.
                billf@chimesnet.com / billf@FreeBSD.org

bleah, forgot one last bit to satisfy the style(9) police... ;P
(how many mistake can I make in one three-line patch?)

Index: sys/i386/i386/vm86.c
===================================================================
RCS file: /export/ncvs/src/sys/i386/i386/vm86.c,v
retrieving revision 1.31
diff -u -r1.31 vm86.c
--- sys/i386/i386/vm86.c	1999/10/29 18:08:35	1.31
+++ sys/i386/i386/vm86.c	2000/09/17 17:42:34
@@ -701,18 +701,19 @@
 		}
 		break;
 
-#if 0
 	case VM86_INTCALL: {
 		struct vm86_intcall_args sa;
 
-		if (error = copyin(ua.sub_args, &sa, sizeof(sa)))
+		if (p->p_cred->pc_ucred->cr_uid != 0)
+			return EPERM;
+
+		if (0 != (error = copyin(ua.sub_args, &sa, sizeof(sa))))
 			return (error);
-		if (error = vm86_intcall(sa.intnum, &sa.vmf))
+		if (0 != (error = vm86_intcall(sa.intnum, &sa.vmf)))
 			return (error);
 		error = copyout(&sa, ua.sub_args, sizeof(sa));
 		}
 		break;
-#endif
 
 	default:
 		error = EINVAL;

Comment 5 dwmalone 2000-10-02 14:19:34 UTC

Responsible Changed
From-To: freebsd-bugs->jlemon

I think jlemon committed a change roughly equivelent to the patch 
in this PR - can it be closed now?

Comment 6 jlemon 2000-10-02 14:37:52 UTC

State Changed
From-To: open->closed

Similar change applied, thanks.