From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 49224 invoked by alias); 14 Jun 2018 16:43:31 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 49215 invoked by uid 89); 14 Jun 2018 16:43:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: zimbra.cs.ucla.edu Subject: Re: [PATCH v2] libio: Flush stream at freopen (BZ#21037) To: Adhemerval Zanella , Andreas Schwab Cc: libc-alpha@sourceware.org References: <1528925590-29895-1-git-send-email-adhemerval.zanella@linaro.org> <25a001ee-301f-1af5-20ed-27b883500f04@linaro.org> From: Paul Eggert Openpgp: preference=signencrypt Message-ID: <2df65bd4-8dfc-187a-c917-87da4510fd15@cs.ucla.edu> Date: Thu, 14 Jun 2018 16:43:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <25a001ee-301f-1af5-20ed-27b883500f04@linaro.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable X-SW-Source: 2018-06/txt/msg00417.txt.bz2 On 06/14/2018 08:01 AM, Adhemerval Zanella wrote: > + char fdfilename[30];=20 The magic number 30 should be turned into a named constant defined in=20 fd_to_filename.h, to help prevent future mistakes. Once that is done,=20 you can change the signature of fd_to_filename to not pass the size, and=20 to require the caller to pass an array of at least size 30, so that=20 fd_to_filename need not check for buffer overflow (see below for more on=20 this). > + const char *gfilename; > + if (filename =3D=3D NULL && fd >=3D 0) > + gfilename =3D fd_to_filename (fd, fdfilename, sizeof fdfilename) > + ? fdfilename : NULL; > + else > + gfilename =3D filename; Cleaner would be: =C2=A0 const char *gfilename =C2=A0=C2=A0=C2=A0 =3D filename !=3D NULL ? filename : fd_to_filename (fd,= fdfilename); That is, let fd_to_filename worry about what to do with negative fd, and=20 have it return fdfilename or NULL, and don't pass the size (which should=20 be that magic number regardless). > -static inline const char * > -fd_to_filename (int fd) > +static inline bool > +fd_to_filename (int fd, char *buf, size_t len) > { > - char *ret =3D malloc (30); > + __snprintf (buf, len, "/proc/self/fd/%d", fd); >=20=20=20 > - if (ret !=3D NULL) > - { > - struct stat64 st; > - > - *_fitoa_word (fd, __stpcpy (ret, "/proc/self/fd/"), 10, 0) =3D '\0= '; > - > - /* We must make sure the file exists. */ > - if (__lxstat64 (_STAT_VER, ret, &st) < 0) > - { > - /* /proc is not mounted or something else happened. Don't > - return the file name. */ > - free (ret); > - ret =3D NULL; > - } > - } > - return ret; > + /* We must make sure the file exists. */ > + if (__lxstat64 (_STAT_VER, buf, & (struct stat64) {}) < 0) > + /* /proc is not mounted or something else happened. */ > + return false; > + return true; > } The __snprintf would be quite wrong if the string did not fit. Again, I=20 suggest simply requiring the buffer to be long enough and not checking=20 its length, and sticking with stpcpy + _fitoa_word which should be more=20 efficient than __snprintf anyway (or if you prefer simplicity to speed,=20 just use sprintf). The '& (struct stat64) {}' construct looks pretty but is less efficient=20 as it makes the compiler zero out the structure unnecessarily, so the=20 code should keep doing that struct the old-fashioned way.