From owner-svn-src-head@freebsd.org Fri Jun 15 04:34:52 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 0F88B1015644; Fri, 15 Jun 2018 04:34:52 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id 4527476E06; Fri, 15 Jun 2018 04:34:50 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 1E4F43CCEB0; Fri, 15 Jun 2018 14:34:47 +1000 (AEST) Date: Fri, 15 Jun 2018 14:34:44 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Brooks Davis cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r335177 - in head/sys: compat/freebsd32 compat/linux i386/ibcs2 kern vm In-Reply-To: <201806142127.w5ELRPoK022899@repo.freebsd.org> Message-ID: <20180615135333.C1111@besplex.bde.org> References: <201806142127.w5ELRPoK022899@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=cIaQihWN c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=S1YjR5Gf4vJTHfycKw0A:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 Jun 2018 04:34:52 -0000 On Thu, 14 Jun 2018, Brooks Davis wrote: > ... > Log: > Name the implementation of brk and sbrk sys_break(). This API was actually named brk(2). Now brk(2) doesn't actually give this API. brk(2) doesn't exist in libc. Only brk(3) exists. It calls the actual brk(2) and adds complications. brk(3) doesn't exist according to its (nonexistent) man page. sbrk(2) hasn't existed in libc for much longer. Only sbrk(3) exists. It calls brk(3nonexistent) and adds complications. sbrk(3) doesn't exist according to its (nonexistent) man page. > ... > Modified: head/sys/compat/freebsd32/syscalls.master > ============================================================================== > --- head/sys/compat/freebsd32/syscalls.master Thu Jun 14 21:22:14 2018 (r335176) > +++ head/sys/compat/freebsd32/syscalls.master Thu Jun 14 21:27:25 2018 (r335177) > @@ -87,8 +87,7 @@ > int mode, int dev); } > 15 AUE_CHMOD NOPROTO { int chmod(char *path, int mode); } > 16 AUE_CHOWN NOPROTO { int chown(char *path, int uid, int gid); } > -17 AUE_NULL NOPROTO { caddr_t obreak(char *nsize); } break \ > - obreak_args int > +17 AUE_NULL NOPROTO { caddr_t break(char *nsize); } > 18 AUE_GETFSSTAT COMPAT4 { int freebsd32_getfsstat( \ > struct statfs32 *buf, long bufsize, \ > int mode); } > brk(2) under any name returns int, not caddr_t. Even its man page gets this correct. This was broken for all cases except one. The type checking of declarations generated by these pseudo-prototypes is too weak to detect the error. First the good name of these APIs is broken by adding a sys_ prefix. I think the return type is ignored. Syscall implementation functions all return int (just an error code). They return actual values in td_retval. Indirections break type checking for this especially well. Old versions that passed around a pointer to td_retval could have done type checking much like is done for args structs. The type checking is also weak for args structs, and this is useful for avoiding const poisoning. Type qualifiers get lost somewhere even when they are in the pseudo-prototypes. Lots of other errors like [signed] int instead of [unsigned] uid_t visible for chown() in the above go undetected. Even the size mismatch for 16-bit mode_t vs 32-bit int visible in the above for chmod() goes undetected. > Modified: head/sys/i386/ibcs2/syscalls.master > ============================================================================== > --- head/sys/i386/ibcs2/syscalls.master Thu Jun 14 21:22:14 2018 (r335176) > +++ head/sys/i386/ibcs2/syscalls.master Thu Jun 14 21:27:25 2018 (r335177) > @@ -57,7 +57,7 @@ > 15 AUE_CHMOD STD { int ibcs2_chmod(char *path, int mode); } > 16 AUE_CHOWN STD { int ibcs2_chown(char *path, int uid, \ > int gid); } > -17 AUE_NULL NOPROTO { int obreak(caddr_t nsize); } > +17 AUE_NULL NOPROTO { caddr_t break(caddr_t nsize); } > 18 AUE_STAT STD { int ibcs2_stat(char* path, \ > struct ibcs2_stat *st); } > 19 AUE_LSEEK STD { long ibcs2_lseek(int fd, long offset, \ This was the one instance with the correct return type. All of the arg types for break() are broken too. This one is different, but the difference happens to be just a misspelling of the same wrong type. brk(2) is documented to have the newfangled arg type void *. The implementation of [s]brk(3) uses the correct type, but the syscalls.master files haven't caught up yet. Most use char *, but this one uses caddr_t (which happens to be char *). Perhaps caddr_t is actually correct for ibcs2. But then it would be the ibcs2 caddr_t that is correct. Bruce