From: Eric Sunshine Date: Thu, 03 Sep 2015 14:47:10 GMT Subject: Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X) Message-ID: In-Reply-To: On Thu, Sep 3, 2015 at 10:39 AM, Eric Sunshine wrote: > On Tue, Sep 1, 2015 at 2:26 PM, Karthik Nayak wrote: >> +static void append_lines(struct strbuf *out, const char *buf, unsigned long size, int lines) >> +{ >> + if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size)) >> + size += 2; > > Aside from the +2 which Matthieu already questioned, this code has a > much more serious problem. strstr() assumes that 'buf' is > NUL-terminated, however, the fact that buf's size is also being passed > to the function, implies that it may not be NUL-terminated. > Consequently, strstr() could zip well past the end of 'buf', trying to > consult memory not part of 'buf', which is a violation of the C > standard. Even with the band-aid (sp <= buf + size) which tries to > detect this situation, it's still a violation, and could crash if > strstr() attempts to consult memory which hasn't even been allocated > to the process or is otherwise protected in some fashion. > > It's possible that 'buf' may actually always be NUL-terminated, but > this code does not convey that fact. If it is known to be > NUL-terminated, then it is safe to use strstr(), however, that should > be shown more clearly either by revising the code or adding an > appropriate comment. Also, if 'buf' is indeed unconditionally NUL-terminated, then the (sp <= buf + size) check is wasted code since the result of strstr() will always be either NULL or pointing somewhere within the NUL-terminated string.