From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/484 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: musl bugs Date: Tue, 27 Sep 2011 17:00:04 -0400 Message-ID: <20110927210004.GO132@brightrain.aerifal.cx> References: <20110927160646.GA23877@albatros> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: dough.gmane.org 1317157590 22479 80.91.229.12 (27 Sep 2011 21:06:30 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Tue, 27 Sep 2011 21:06:30 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-485-gllmg-musl=m.gmane.org@lists.openwall.com Tue Sep 27 23:06:24 2011 Return-path: Envelope-to: gllmg-musl@lo.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by lo.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1R8erA-0006OZ-2j for gllmg-musl@lo.gmane.org; Tue, 27 Sep 2011 23:06:24 +0200 Original-Received: (qmail 18294 invoked by uid 550); 27 Sep 2011 21:06:23 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 18279 invoked from network); 27 Sep 2011 21:06:23 -0000 Content-Disposition: inline In-Reply-To: <20110927160646.GA23877@albatros> User-Agent: Mutt/1.5.21 (2010-09-15) Xref: news.gmane.org gmane.linux.lib.musl.general:484 Archived-At: On Tue, Sep 27, 2011 at 08:06:46PM +0400, Vasiliy Kulikov wrote: > Hi Rich, > > getmntent_r(): > - fgets() should be checked for too small buffer. And what should happen? ERANGE? This non-standardized stuff is so poorly documented... Should it seek back and allow you to read the entry next time with a larger buffer? Or should it just fail? > - Looks like fgets() may fail. Then ferror() should be used together > with feof(). Yes, I used to mistakenly think errors also set the EOF indicator... > getmntent(): > - Is linebuf[256] big enough? IMO as the buffer is not supplied by a > user, it should be dynamically allocated. Calling getmntent() and > getting truncated result/ERANGE is somewhat not expected. You're probably right... > addmntent(): > - Here fseek() can be easily checked for errors => return 1 in case of > error. Fixing it. > hasmntopt(): > - Implementation is wrong. The argument is not a substring, but a > single option, possibly with "=value". Glibc's implementation is OK > IMO. I will look into this... > prctl() and other places: > - Why no va_end()? It is __builtin_va_end() sometimes, and AFAIU it is > not a noop. Do you have a list? AFAIK it is (and must be) a no-op in the real world, but for correctness we should use it anyway. (An implementation theoretically could malloc as part of va_start/va_arg, but this would render it a junk/useless implementation because variadic functions would crash on OOM..) > getgrgid() and getgrnam(): > - errno is not saved while calling endgrent() (close() inside). POSIX > says close() may return EIO if I/O error happened during close() with > RO fd, altering errno. Also getpwuid and getpwnam... Fixed. > execvp(): > - As the code chooses the first possible path in $PATH, the > /usr/local/bin should be the last path. POSIX says it should start > with null path (current dir), but it is crazy. Where does it say this? I see (in the execvp documentation in POSIX 2008): "If this environment variable is not present, the results of the search are implementation-defined." In particular, unless you use sysconf to obtain and set a default PATH, there's no implication that the standard utilities should be in the search. I put /usr/local/bin first because the idea is that you use it locally to override possibly-shared/distro-provided binaries in /usr/bin and /bin. I'm open to suggestions on changing this if it's problematic, but I don't think it's a conformance issue. > - I don't see an overflow here (comment claims so)... Well there's definitely a VLA overflow, and there could be an arithmetic wrap-around if file points to a substring of getenv("PATH") (pathological but possible). Both issues should be fixed, probably by simply rejecting file longer than NAME_MAX and path components longer than PATH_MAX, and simply using a fixed-size buffer of size PATH_MAX. Rich