Am 28.07.2014 um 23:42 schrieb Junio C Hamano:
Jeff King <peff@xxxxxxxx> 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é
--
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