From owner-svn-src-head@freebsd.org Fri Dec 18 20:21:37 2015 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id E762DA4BD28; Fri, 18 Dec 2015 20:21:36 +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 7BDEF1340; Fri, 18 Dec 2015 20:21:35 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id D277A3C4D27; Sat, 19 Dec 2015 07:21:23 +1100 (AEDT) Date: Sat, 19 Dec 2015 07:21:23 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: John Baldwin cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r292443 - head/usr.sbin/boot0cfg In-Reply-To: <201512181752.tBIHq9QN069909@repo.freebsd.org> Message-ID: <20151219063208.M2407@besplex.bde.org> References: <201512181752.tBIHq9QN069909@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.1 cv=cK4dyQqN c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=kj9zAlcOel0A:10 a=pGLkceISAAAA:8 a=t5GoHRYVe5rdrHgZcJgA:9 a=MvHf3p7H2XhwkImM:21 a=84w2aD5DFUGud9fB:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 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, 18 Dec 2015 20:21:37 -0000 On Fri, 18 Dec 2015, John Baldwin wrote: > Log: > Fix the precious change to check the pointer returned by malloc(). > > Submitted by: luke > Pointy hat to: jhb Of course, malloc can't fail, especially for small utilities like boot0cfg, especially in -current. > Modified: head/usr.sbin/boot0cfg/boot0cfg.c > ============================================================================== > --- head/usr.sbin/boot0cfg/boot0cfg.c Fri Dec 18 17:39:54 2015 (r292442) > +++ head/usr.sbin/boot0cfg/boot0cfg.c Fri Dec 18 17:52:08 2015 (r292443) > @@ -337,7 +337,7 @@ read_mbr(const char *disk, u_int8_t **mb > return (mbr_size); > } > *mbr = malloc(sizeof(buf)); > - if (mbr == NULL) > + if (*mbr == NULL) > errx(1, "%s: unable to allocate MBR buffer", disk); Style bug. Someone suggested using the 'if ((*mbr = malloc(...)) == NULL)' style. That might be less clear, but it is what is used elswhere in the program, including for the old malloc check. Also, the error messages are too different. The other one calls the buffer a "read" buffer although it spells its variable name mbr. > memcpy(*mbr, buf, sizeof(buf)); > close(fd); Program for checking that malloc can't fail: X #define SIZE 16 X X #include X #include X #include X X int X main(void) X { X char *p; X size_t tot; X X tot = 0; X while ((p = malloc(SIZE)) != NULL) { X bzero(p, SIZE); X tot += SIZE; X if (tot > 4 * 1024 * 1024) X errx(1, "giving up after allocating %zu bytes", tot); X } X errx(1, "malloc failed after allocating %zu bytes", tot); X } It is hard to get this to fail in -current. ulimit -Sm 4096 no longer works for limiting data size (ulimit -m never worked). Virtual memory must be limited in -current. But ulimit -Sv 4096 prevents most programs from starting, due to bugs in dynamic linkage: pts/9:bde@freefall:~/zz> ulimit -Sv 4096 pts/9:bde@freefall:~/zz> ls /lib/libutil.so.9: mmap of entire address space failed: Cannot allocate memory pts/9:bde@freefall:~/zz> ./z /lib/libc.so.7: mmap of entire address space failed: Cannot allocate memory "./z" is this program. Linking the program statically makes it run in the measly 4096k of virtual and mallocable memory. It then allocates 4194320 bytes using the small allocation size. Of course, cc can't run in 4096k. However, it is not necessary to map the _entire_ address space like the error messages say. Dynamic "./z" runs in a measly 10000k. ls fails as above up to at least 12000k, but dumps core with 14000k. ls starts working with a measly 16000k. cc needs more still. I didn't find the exact requirement, and used 160000k to compile this little program. Not so measly. The program is careful to touch all the memory that it allocates. Without that, I think things are even more broken. Somehow the process virtual size doesn't grown above 6.2M. Apparently even the virtual memory is not completely allocated until it is used. So malloc() returns success, but failures may happen much later when the memory is used. Similarly with SIZE = 16MB but only the first byte of each allocation touched. OTOH, with size = 16MB all touched and no limits, the process grows very large and takes too long to respond to job control symbols, as if it is swapping. The -d and -m limits don't work for preventing growth of its real memory. ulimit -Sv should be used in testing since ulimit -v sets the hard limit and prevents restoring a usable soft limit. Setting -v to a low value (lower than 16000k for ls) is not useful except for testing malloc failure since it gives an unusable shell. Bruce