From owner-svn-src-projects@freebsd.org Sun Aug 9 12:54:27 2015 Return-Path: Delivered-To: svn-src-projects@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 846FE9986D6 for ; Sun, 9 Aug 2015 12:54:27 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 19ADE326; Sun, 9 Aug 2015 12:54:27 +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 mail110.syd.optusnet.com.au (Postfix) with ESMTPS id 94733781D5D; Sun, 9 Aug 2015 22:54:16 +1000 (AEST) Date: Sun, 9 Aug 2015 22:54:15 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Baptiste Daroussin cc: src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: Re: svn commit: r286521 - projects/collation/lib/libc/locale In-Reply-To: <201508091150.t79Boo3v096088@repo.freebsd.org> Message-ID: <20150809223647.O2415@besplex.bde.org> References: <201508091150.t79Boo3v096088@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=eZjABOwH c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=kj9zAlcOel0A:10 a=D80xplrrwImFV1nse3cA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 09 Aug 2015 12:54:27 -0000 On Sun, 9 Aug 2015, Baptiste Daroussin wrote: > Log: > Use asprintf/free instead of snprintf Why? It takes 3 times as much code, and immediately gave you a memory leak when you wrote only twice as much. You fixed the memory leak in the next commit, but it might not always be so easy to see. > Modified: projects/collation/lib/libc/locale/collate.c > ============================================================================== > --- projects/collation/lib/libc/locale/collate.c Sun Aug 9 11:47:01 2015 (r286520) > +++ projects/collation/lib/libc/locale/collate.c Sun Aug 9 11:50:50 2015 (r286521) > @@ -107,7 +107,7 @@ int > __collate_load_tables_l(const char *encoding, struct xlocale_collate *table) > { > int i, chains, z; > - char buf[PATH_MAX]; > + char *buf; POSIX_ME_HARDER code would use {PATH_MAX} = sysconf(__PATH_MAX) and error handling for sysconf() and then for malloc()ing {PATH_MAX} bytes. This would take 10 times as much code, except it could use a VLA with no error checking for the allocation starting with C99. The asprintf() method would be better then. > char *TMP; > char *map; > collate_info_t *info; > @@ -120,11 +120,13 @@ __collate_load_tables_l(const char *enco > return (_LDP_CACHE); > } > > - (void) snprintf(buf, sizeof (buf), "%s/%s/LC_COLLATE", > - _PathLocale, encoding); > + asnprintf(&buf, "%s/%s/LC_COLLATE", _PathLocale, encoding); > + if (buf == NULL) > + return (_LDP_ERROR); There was no error checking for snprintf(). In fact, its value was voided. But error checking for snprintf() can be added just as easily as error checking for asprintf(). I think this code doesn't compile, because asnprintf() makes no sense and doesn't exist. > > if ((fd = _open(buf, O_RDONLY)) < 0) Memory leak here. > return (_LDP_ERROR); > + free(buf); The memory leak can be fixed better than in the committed version using: fd = _open(...); free(buf); if (fd < 0) return (_LDP_ERROR); > if (_fstat(fd, &sbuf) < 0) { > (void) _close(fd); > return (_LDP_ERROR); Bruce