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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 4, 2013, 06:26 UTC
Message-ID: <7vzjv68k8b.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1370299959-5573-1-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

Show 14 quoted lines

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

Show 6 quoted lines

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

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 = "";
	}
        /* 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(...);

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.

Also you no longer need to have an extern variable in environment.c