Re: [PATCH v2] core: use env variable instead of config var to turn on logging pack access

On Tue, Jun 4, 2013 at 1:26 PM, Junio C Hamano [off-list ref] wrote:

Nguyễn Thái Ngọc Duy [off-list ref] writes:

quoted

 > 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...

quoted

+             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