public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Ramsay Jones <ramsay@ramsay1•demon.co.uk>
To: Junio C Hamano <gitster@pobox•com>
Cc: GIT Mailing-list <git@vger•kernel.org>
Subject: Re: [PATCH 3/5] path.c: Use vsnpath() in the implementation of git_path()
Date: Fri, 07 Sep 2012 20:19:16 +0100	[thread overview]
Message-ID: <504A48B4.9010109@ramsay1.demon.co.uk> (raw)
In-Reply-To: <7vsjax8rap.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1•demon.co.uk> writes:
> 
>> The current implementation of git_path() is essentially the same as
>> that of vsnpath(), with two minor differences. First, git_path()
>> currently insists that the git directory path is no longer than
>> PATH_MAX-100 characters in length. However, vsnpath() does not
>> attempt this arbitrary 100 character reservation for the remaining
>> path components. Second, vsnpath() uses the "is_dir_sep()" macro,
>> rather than comparing directly to '/', to determine if the git_dir
>> path component ends with a path separator.
>> In order to benefit from the above improvements,...
> 
> In the longer term, I think this goes in the right direction, but
> the loss of reservation, especially when we know git_path() is used
> by some callers to get the base directory in $GIT_DIR that want to
> append stuff after the returned directory path to form the final
> pathname, is a bit worrysome.  It may be hiding a bug (lack of
> proper limit check) on the callers' side.

Hmm, at first I could not see what you found worrysome here.
After all, the number of inputs which leads to success (i.e. does
not result in an "/bad-path/" return) has been *increased* with
this patch.

However, I suppose you are concerned about something like this:

    char *git_dir = git_path("");
    if (strcmp(git_dir, "/bad-path/") != 0) {
        /*
         * Having studied the implementation of git_path(), I know
         * that the buffer pointed to by git_dir has space for an
         * additional 100 chars. This is enough room to concatenate
         * the doberry path, so this is safe ...
         */
        strcat(git_dir, doberry); /* oops */
    }

Yes?

Hmm, yes it would be a little disapointing to see such parasitic code!
;-)

You said above: "... especially when we know git_path() is used
by some callers to get the base directory in $GIT_DIR ...". Can you
point me to an example of such a caller; I have been unable to find
any code which does this.

ATB,
Ramsay Jones

     prev parent reply	other threads:[~2012-09-07 20:14 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-09-04 17:29 [PATCH 3/5] path.c: Use vsnpath() in the implementation of git_path() Ramsay Jones
2012-09-04 20:30 ` Junio C Hamano
2012-09-07 19:19   ` Ramsay Jones [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=504A48B4.9010109@ramsay1.demon.co.uk \
    --to=ramsay@ramsay1$(echo .)demon.co.uk \
    --cc=git@vger$(echo .)kernel.org \
    --cc=gitster@pobox$(echo .)com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox