On Tue, Jun 4, 2013 at 1:26 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
>
>> > Have you ever tested this?
>> >
>> > Once log_pack_access goes to NULL (e.g. when it sees the empty
>> > string it was initialized to), this new test will happily
>> > dereference NULL.
>>
>> My bad. I did test when GIT_TRACE_PACK_ACCESS was set, not when it
>> was set to an empty string. All cases tested now.
>>
>> @@ -1956,6 +1958,14 @@ static void write_pack_access_log(struct packed_git *p, off_t obj_offset)
>> {
>> static FILE *log_file;
>>
>> + if (!*log_pack_access) {
>
> The first time, we will see the static empty string and come into
> this block...
>
>> + log_pack_access = getenv("GIT_TRACE_PACK_ACCESS");
>> + if (log_pack_access && !*log_pack_access)
>> + log_pack_access = NULL;
>> + if (!log_pack_access)
>> + return;
>> + }
>
> This may
>
> (1) not find the env-var, in which case log_pack_access becomes
> NULL here, and the function returns.
>
> (2) find the env-var to be an empty string, in which case
> log_pack_access becomes NULL in the next statement, and the
> function returns.
>
> (3) find the env-var to be a non-empty string, and the function
> does not return.
>
> And the next time around, (3) above may work fine; the first if()
> will fail and we do not come in. But in either (1) or (2), don't
> you keep checking the environment every time you come here?
>
> Oh, no, it is worse than that. Either case you set the variable to
> NULL, so the very first "if (!*log_pack_access)" would dereference
> NULL.
You found it out already. The only caller does this
if (log_pack_access) write_pack_access_log(p, obj_offset);
so in (1) and (2), write_pack_access_log() will never be called again.
Originally I had "log_pack_access && !*log_pack_access" in the first
if(), but I dropped it because of the caller's condition. It's a bit
fragile though. Imagine a new callsite is added.. (to be continued)
> Why not start from NULL:
>
> static const char *log_pack_access;
>
> treat that NULL as "unknown" state, and avoid running getenv over
> and over again by treating non-NULL value as "known"? Perhaps like
> this?
>
> if (!log_pack_access) {
> /* First time: is env set? */
> log_pack_access = getenv("GIT_TRACE_PACK_ACCESS");
> if (!log_pack_access)
> log_pack_access = "";
or set log_pack_access to SomePredefinedButUselessString here. I
wanted to do this but couldn't because of global variable
initialization.
> }
> /* Now GIT_TRACE_PACK_ACCESS is known */
> if (!*log_pack_access)
> return; /* not wanted */
>
> As you can no longer rely on "config is read before we do anything
> else" by switching to lazy env checking, your guard at the caller
> needs to be updated, if you want to reduce unneeded call-return
> overhead:
>
> if (!log_pack_access || *log_pack_access)
> write_pack_access_log(...);
and this turns to "if (!log_.. || log != SomePrede...)", no more
peeking into log_pack_access.
>
> But the guard is purely for performance, not correctness, because
> the function now does the "return no-op if we know we are not
> wanted" itself.
(continued here) ..so yeah this looks better.
> Also you no longer need to have an extern variable in environment.c
will fix.
--
Duy
--
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