From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7190 invoked by alias); 15 Aug 2014 11:33:39 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 33019 Received: (qmail 14740 invoked from network); 15 Aug 2014 11:33:37 -0000 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.2 Date: Fri, 15 Aug 2014 19:23:16 +0800 From: Han Pingtian To: zsh-workers@zsh.org Subject: Re: zsh 5.0.5-dev-2 Message-ID: <20140815112316.GA17063@localhost.localdomain> Mail-Followup-To: zsh-workers@zsh.org References: <20140812212920.67dcb116@pws-pc.ntlworld.com> <29575.1407969294@thecus.kiddle.eu> <20140814093442.1a74c5b7@pwslap01u.europe.root.pri> <20140814103227.74c7d168@pwslap01u.europe.root.pri> <140814092045.ZM18007@torch.brasslantern.com> <20140814205429.44baf512@pws-pc.ntlworld.com> <140814214412.ZM4177@torch.brasslantern.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <140814214412.ZM4177@torch.brasslantern.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 14081511-5806-0000-0000-000000397E52 Hi, Looks like on the 747 line of Src/utils.c: 747 sprintf(xbuf2, "%s/%s", xbuf, *pp); The "cd .." will trigger a buffer overflow if I compile zsh with -D FORTIFY_SOURCE=2 . Shall we return -1 here if it will overflow xbuf2? On Thu, Aug 14, 2014 at 09:44:12PM -0700, Bart Schaefer wrote: > On Aug 14, 8:54pm, Peter Stephenson wrote: > } > } It's entirely unclear to me from looking at xsymlinks() when an empty > } xbuf would actually constitute a failure. > > This occurs in the (t0 == -1) case when the reason that readlink() has > failed is because the path so far concatenated with the next segment of > the split path has produced a string longer than PATH_MAX. In that case > (xbuflen + pplen > sizeof(xbuf)) we completely truncate xbuf and give up. > > } If it can't expand something > } as a symbolic link it simply treats it literally; > > Yes, but for a /path/to/somereallylongname, appending somereallylongname > to the expansion of xbuf so far, might overflow xbuf. > > } If we don't have an answer now I'd be inclined to comment out the > } warning. > > Maybe just make xsymlinks() have a ternary return value instead of a > boolean one, and return an actual error value in the case where we've > truncated on overflow instead of as part of normal expansion to root? > > I don't know if all of these changes are necessary. It has always > bothered me that the return value of the recursive calls to xsymlinks() > are ignored, but maybe this isn't the right way to handle them. > > Incidentally, anybody recall what the obviously obsolete comment above > the xsymlinks() definition in Src/utils.c is about? > > diff --git a/Src/utils.c b/Src/utils.c > index 998e46a..076a33c 100644 > --- a/Src/utils.c > +++ b/Src/utils.c > @@ -716,7 +716,6 @@ slashsplit(char *s) > } > > /* expands symlinks and .. or . expressions */ > -/* if flag = 0, only expand .. and . expressions */ > > /**/ > static int > @@ -753,6 +752,7 @@ xsymlinks(char *s) > strcat(xbuf, *pp); > } else { > *xbuf = 0; > + ret = -1; > break; > } > } else { > @@ -760,9 +760,11 @@ xsymlinks(char *s) > metafy(xbuf3, t0, META_NOALLOC); > if (*xbuf3 == '/') { > strcpy(xbuf, ""); > - xsymlinks(xbuf3 + 1); > + if (xsymlinks(xbuf3 + 1) < 0) > + ret = -1; > } else > - xsymlinks(xbuf3); > + if (xsymlinks(xbuf3) < 0) > + ret = -1; > } > } > freearray(opp); > @@ -781,11 +783,10 @@ xsymlink(char *s) > if (*s != '/') > return NULL; > *xbuf = '\0'; > - xsymlinks(s + 1); > - if (!*xbuf) { > + if (xsymlinks(s + 1) < 0) > zwarn("path expansion failed, using root directory"); > + if (!*xbuf) > return ztrdup("/"); > - } > return ztrdup(xbuf); > } > > @@ -795,7 +796,7 @@ print_if_link(char *s) > { > if (*s == '/') { > *xbuf = '\0'; > - if (xsymlinks(s + 1)) > + if (xsymlinks(s + 1) > 0) > printf(" -> "), zputs(*xbuf ? xbuf : "/", stdout); > } > } > > -- > Barton E. Schaefer