On Wed, Oct 23, 2013 at 7:29 PM, Jeff King <peff@xxxxxxxx> wrote: > On Wed, Oct 23, 2013 at 07:55:06PM +0700, Nguyen Thai Ngoc Duy wrote: > >> The old code does not do boundary check so any paths longer than >> PATH_MAX can cause buffer overflow. Replace it with strbuf to handle >> paths of arbitrary length. > > I think this is a reasonable solution. If we have such a long path, we > are probably about to feed it to open() or another syscall, and we will > just get ENAMETOOLONG there anyway. But certainly we need to fix the > buffer overflow, and we are probably better off letting the syscall > report failure than calling die(), because we generally handle the > syscall failure more gracefully (e.g., by reporting the failed path but > continuing). > >> - memcpy(path, state->base_dir, len); >> - strcpy(path + len, ce->name); >> - len += ce_namelen(ce); >> + strbuf_reset(&path_buf); >> + strbuf_addf(&path_buf, "%.*s%s", state->base_dir_len, state->base_dir, ce->name); >> + path = path_buf.buf; >> + len = path_buf.len; > > This is not something you introduced, but while we are here, you may > want to use ce->namelen, which would be a little faster than treating it > as a string (especially for strbuf, as it can then know up front how big > the size is). > > I doubt it's measurable, though (especially as the growth cost is > amortized due to the static buffer). I somehow feel that: strbuf_reset(&path_buf); strbuf_add(&path_buf, state->base_dir, state->base_dir_len); strbuf_addch(&path_buf, '/'); strbuf_add(&path_buf, state->name, state->name_len); feels a bit neater than using strbuf_addf. But that might just be me. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html