From: René Scharfe Date: Tue, 29 Jul 2014 00:04:59 GMT Subject: Re: [PATCH v3 05/10] abspath: convert real_path_internal() to strbuf Message-ID: <53D6E52B.1050006@web.de> In-Reply-To: Am 28.07.2014 um 23:42 schrieb Junio C Hamano: > Jeff King writes: > >> On Mon, Jul 28, 2014 at 08:28:30PM +0200, René Scharfe wrote: >> >>> @@ -60,26 +58,22 @@ static const char *real_path_internal(const char *path, int die_on_error) >>> goto error_out; >>> } >>> >>> - if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX) { >>> - if (die_on_error) >>> - die("Too long path: %.*s", 60, path); >>> - else >>> - goto error_out; >>> - } >>> + strbuf_init(&sb, 0); >>> + strbuf_addstr(&sb, path); >> >> As with the other patch I just mentioned, should this be strbuf_reset, >> not strbuf_init? We want to reset the static buffer back to zero-size, >> not throw it away and leak whatever was there. >> >> -Peff > > Yes, this one seems to be leaking. > > "Next call to the function invalidates the return value the last > caller received" feels like playing with fire. Most existing > callers are safe in that the first thing they do to the returned > string is xstrdup() it, but we would need to check all the other > callers. That's the price we pay for using static variables, no? Callers need to consume them as long as they're fresh and multi-threading is not allowed. Before, callers could use wrong buffer contents, after the patch they could still have a pointer to freed memory, which should be more noticeable in tests. Getting a strbuf_add_real_path() in order to avoid static variables would be nice. And it would also be nice if it worked without calling chdir(). Nice topics for follow-up patches. :) > I briefly thought it is not OK for set_git_work_tree(), which gets > new_work_tree, calls real_path() to receive the value from the > function, and then calls real_path() again on it. The "We've > already done it" optimization is the only thing that makes it safe, > which feels overly fragile. It wasn't introduced as an optimization, but to silence valgrind (1d679de5: make_absolute_path: return the input path if it points to our buffer). set_git_work_tree() calls real_path() only once in each of its two branches. However, one caller (init) hands it a path returned by real_path(); we can change that (sent a patch). René