On Wed, Sep 06, 2017 at 12:59:46PM +0900, Junio C Hamano wrote:
> Jeff King writes:
>
> > In the long run we may want to drop the "tempfiles must remain forever"
> > rule. This is certainly not the first time it has caused confusion or
> > leaks. And I don't think it's a fundamental issue, just
Jeff King writes:
> In the long run we may want to drop the "tempfiles must remain forever"
> rule. This is certainly not the first time it has caused confusion or
> leaks. And I don't think it's a fundamental issue, just the way the code
> is written. But in the interim, this fix is probably wor
On Sat, Sep 02, 2017 at 04:44:51AM -0400, Jeff King wrote:
> There are actually very few callers of create_tempfile (I count two).
> Everybody just uses lock_file(). So I think we could get away with just
> modifying the internals of the lock struct to hold a pointer, and then
> teaching commit_lo
On Wed, Aug 30, 2017 at 09:07:53AM +0200, Michael Haggerty wrote:
> > So it's probably safer to just let tempfile.c handle the whole lifetime,
> > and have it put all live tempfiles on the heap, and free them when
> > they're deactivated. That means our creation signature becomes more
> > like:
>
On Wed, Aug 30, 2017 at 02:06:02PM -0700, Brandon Williams wrote:
> > We could extend that protection by having sigchain_push_common() set
> > sa_mask to cover all of the related signals. On Linux and BSD the
> > current code using signal() also implies SA_RESTART. We could add that
> > to our fla
On 08/30, Jeff King wrote:
> On Wed, Aug 30, 2017 at 12:57:31PM -0700, Brandon Williams wrote:
>
> > > And I think we're fine there even with a doubly-linked list. It's still
> > > the single update of the "next" pointer that controls that second
> > > traversal.
> >
> > I know it was mentioned e
On Wed, Aug 30, 2017 at 12:57:31PM -0700, Brandon Williams wrote:
> > And I think we're fine there even with a doubly-linked list. It's still
> > the single update of the "next" pointer that controls that second
> > traversal.
>
> I know it was mentioned earlier but if this is a critical section,
On 08/30, Jeff King wrote:
> On Wed, Aug 30, 2017 at 08:55:01AM +0200, Michael Haggerty wrote:
>
> > > + tempfile->active = 0;
> > > + for (p = &tempfile_list; *p; p = &(*p)->next) {
> > > + if (*p == tempfile) {
> > > + *p = tempfile->next;
> > > + break;
>
On Wed, Aug 30, 2017 at 08:55:01AM +0200, Michael Haggerty wrote:
> > + tempfile->active = 0;
> > + for (p = &tempfile_list; *p; p = &(*p)->next) {
> > + if (*p == tempfile) {
> > + *p = tempfile->next;
> > + break;
> > + }
> > }
> >
On 08/30/2017 07:55 AM, Jeff King wrote:
> On Wed, Aug 30, 2017 at 12:55:55AM -0400, Jeff King wrote:
> [...]
> The patch below demonstrates how this could be used to turn some
> "xcalloc" lock_files into stack variables that are allowed to go out of
> scope after we commit or rollback. This solves
On 08/30/2017 06:55 AM, Jeff King wrote:
> [...]
> Something like this, which AFAICT is about as safe as the existing code
> in its list manipulation. It retains the "active" flag as an additional
> check which I think isn't strictly necessary, but potentially catches
> some logic errors.
>
> The
On Wed, Aug 30, 2017 at 12:55:55AM -0400, Jeff King wrote:
> > I feel like right now we meet (1) and not (2). But I think if we keep to
> > that lower bar of (1), it might not be that bad. We're assuming now that
> > there's no race on the tempfile->active flag, for instance. We could
> > probably
On Wed, Aug 30, 2017 at 12:31:47AM -0400, Jeff King wrote:
> > It was surprisingly hard trying to get that code to do the right thing,
> > non-racily, in every error path. Since `remove_tempfiles()` can be
> > called any time (even from a signal handler), the linked list of
> > `tempfile` objects
On Wed, Aug 30, 2017 at 6:31 AM, Jeff King wrote:
> On Wed, Aug 30, 2017 at 05:25:18AM +0200, Michael Haggerty wrote:
>> It was surprisingly hard trying to get that code to do the right thing,
>> non-racily, in every error path. Since `remove_tempfiles()` can be
>> called any time (even from a sig
On Wed, Aug 30, 2017 at 05:25:18AM +0200, Michael Haggerty wrote:
> >>> In the long run we may want to drop the "tempfiles must remain forever"
> >>> rule. This is certainly not the first time it has caused confusion or
> >>> leaks. And I don't think it's a fundamental issue, just the way the code
On 08/29/2017 09:12 PM, Jeff King wrote:
> On Tue, Aug 29, 2017 at 12:09:28PM -0700, Brandon Williams wrote:
>
>>> -- >8 --
>>> Subject: [PATCH] config: use a static lock_file struct
>>>
>>> When modifying git config, we xcalloc() a struct lock_
On Tue, Aug 29, 2017 at 12:09:28PM -0700, Brandon Williams wrote:
> > -- >8 --
> > Subject: [PATCH] config: use a static lock_file struct
> >
> > When modifying git config, we xcalloc() a struct lock_file
> > but never free it. This is necessary because the
On 08/29, Brandon Williams wrote:
> On 08/29, Jeff King wrote:
> > On Tue, Aug 29, 2017 at 02:53:41PM -0400, Jeff King wrote:
> >
> > > It looks like the config code has a minor-ish leak. Patch to follow.
> >
> > Here it is.
> >
> > -- >8 --
On 08/29, Jeff King wrote:
> On Tue, Aug 29, 2017 at 02:53:41PM -0400, Jeff King wrote:
>
> > It looks like the config code has a minor-ish leak. Patch to follow.
>
> Here it is.
>
> -- >8 --
> Subject: [PATCH] config: use a static lock_file struct
>
> When
On Tue, Aug 29, 2017 at 02:53:41PM -0400, Jeff King wrote:
> It looks like the config code has a minor-ish leak. Patch to follow.
Here it is.
-- >8 --
Subject: [PATCH] config: use a static lock_file struct
When modifying git config, we xcalloc() a struct lock_file
but never free it. T
20 matches
Mail list logo
