Re: [PATCH v3 05/10] abspath: convert real_path_internal() to strbuf
René Scharfe writes: >> "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. Yes, I didn't mean to say that fixing this leak by a static whose lifetime rule is "alive until next call" is introducing a new brittleness. The existing callers have lived with that lifetime rule with the callee without the changes in this series, and fixing the leak by replacing _init() with _reset() will make the callee give the same old "alive until next call" lifetime rule to its callers. > 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. :) Yup. Nice, but outside the scope. Of course it is related and can be done as a "while we know about the issue" close follow-up. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 05/10] abspath: convert real_path_internal() to strbuf
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é
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 05/10] abspath: convert real_path_internal() to strbuf
Am 28.07.2014 um 21:09 schrieb Jeff King:
On Mon, Jul 28, 2014 at 08:28:30PM +0200, René Scharfe wrote:
static const char *real_path_internal(const char *path, int die_on_error)
{
- static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
+ static struct strbuf sb = STRBUF_INIT;
Hrm. I thought at first that this was our usual trick of keeping two
"simultaneous" static buffers, so that we can do:
printf("paths '%s' and '%s'\n", real_path(foo), real_path(bar));
But it looks like that is not the case, and we only have two for
swapping back and forth as we figure out the answer (but they both need
to be static, because we do not know which one we will return in the
end). Is that right?
AFAICS it's only swapped to avoid copying the results of a readlink()
call against one buffer into the other. So, yes, that's how I
understand it as well.
René
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 05/10] abspath: convert real_path_internal() to strbuf
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.
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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 05/10] abspath: convert real_path_internal() to strbuf
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 05/10] abspath: convert real_path_internal() to strbuf
On Mon, Jul 28, 2014 at 08:28:30PM +0200, René Scharfe wrote:
> static const char *real_path_internal(const char *path, int die_on_error)
> {
> - static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
> + static struct strbuf sb = STRBUF_INIT;
Hrm. I thought at first that this was our usual trick of keeping two
"simultaneous" static buffers, so that we can do:
printf("paths '%s' and '%s'\n", real_path(foo), real_path(bar));
But it looks like that is not the case, and we only have two for
swapping back and forth as we figure out the answer (but they both need
to be static, because we do not know which one we will return in the
end). Is that right?
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
