From: Brandon Casey <drafnel@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Brandon Casey <bcasey@nvidia.com>,
"git@vger.kernel.org" <git@vger.kernel.org>,
Junio C Hamano <gitster@pobox.com>,
daniel@haxx.se
Subject: Re: [PATCH] http.c: don't rewrite the user:passwd string multiple times
Date: Tue, 18 Jun 2013 19:41:15 -0700 [thread overview]
Message-ID: <CA+sFfMcsOx14UdzLF_JsgkpUQU6yG7DE+00eA3d+Lo-qncDgew@mail.gmail.com> (raw)
In-Reply-To: <20130618221327.GA14234@sigill.intra.peff.net>
On Tue, Jun 18, 2013 at 3:13 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jun 18, 2013 at 12:29:03PM -0700, Brandon Casey wrote:
>> > It could be a problem when we have multiple handles in play
>> > simultaneously (we invalidate the pointer that another simultaneous
>> > handle is using, but do not immediately reset its pointer).
>>
>> Don't we have multiple handles in play at the same time? What's going
>> on in get_active_slot() when USE_CURL_MULTI is defined? It appears to
>> be maintaining a list of "slot" 's, each with its own curl handle
>> initialized either by curl_easy_duphandle() or get_curl_handle().
>
> Yes, we do; the dumb http walker will pipeline loose pack and object
> requests (which makes a big difference when fetching small files). The
> smart http code may use the curl-multi interface under the hood, but it
> should only have a single handle, and the use of the multi interface is
> just for sharing code with the dumb fetch.
>
>> So, yeah, this is what I was referring to when I mentioned
>> "potentially dangerous". Since the current code does not change the
>> size of the string, the pointer will never change, so we won't ever
>> invalidate a pointer that another handle is using.
>
> Agreed. I did not so much mean to dispute your "potentially dangerous"
> claim as clarify exactly what the potential is. :)
Ah, yes, I did read your sentence "It could be a problem when we have
multiple handles in play simultaneously" as "It could be a problem [at
some point in the future] when we [modify the code to] have multiple
handles in play simultaneously, [but since we are not doing that now,
it is not a problem]". Now that I read that sentence again, I see you
are alluding to the dumb http walker code path that I was also
thinking about.
>> The other thing I thought was potentially dangerous, was just
>> truncating the string. Again, if there are multiple curl handles in
>> use (which I thought was a possibility), then merely truncating the
>> string that contains the username/password could potentially cause a
>> problem for another handle that could be in the middle of
>> authenticating using the string. But, I don't know if there is any
>> multi-processing happening within the curl library.
>
> I don't think curl does any threading; when we are not inside
> curl_*_perform, there is no curl code running at all (Daniel can correct
> me if I'm wrong on that).
>
> So I think from curl's perspective a truncation and exact rewrite is
> atomic, and it sees only the final content. I don't know what would
> happen if you truncated and put in _different_ contents. For example, if
> curl would have written out half of the username/password, blocked and
> returned from curl_multi_perform, then you update the buffer, then it
> resumes writing.
>
> IOW, I believe the current code is safe (though in a very subtle way),
> but if you were to allow password update, I'm not sure if it would be or
> not (and if not, you would need a per-handle buffer to make it safe).
>
> I'm fine with making the safety less subtle (e.g., your patch, with a
> comment added).
Ok, will do.
-Brandon
next prev parent reply other threads:[~2013-06-19 2:41 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-06-18 2:00 [PATCH] http.c: don't rewrite the user:passwd string multiple times Brandon Casey
2013-06-18 4:15 ` Eric Sunshine
2013-06-18 5:19 ` Jeff King
2013-06-18 6:36 ` Daniel Stenberg
2013-06-18 15:32 ` Junio C Hamano
2013-06-18 19:29 ` Brandon Casey
2013-06-18 22:13 ` Jeff King
2013-06-19 2:41 ` Brandon Casey [this message]
2013-06-19 2:43 ` [PATCH v2] " Brandon Casey
2013-06-19 5:26 ` Jeff King
2013-06-19 7:40 ` [PATCH] " Daniel Stenberg
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CA+sFfMcsOx14UdzLF_JsgkpUQU6yG7DE+00eA3d+Lo-qncDgew@mail.gmail.com \
--to=drafnel@gmail.com \
--cc=bcasey@nvidia.com \
--cc=daniel@haxx.se \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).