Re: [PATCH] config: use a static lock_file struct

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

Re: [PATCH] config: use a static lock_file struct

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

Re: [PATCH] config: use a static lock_file struct

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

Re: [PATCH] config: use a static lock_file struct

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

Re: [PATCH] config: use a static lock_file struct

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

Re: [PATCH] config: use a static lock_file struct

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

Re: [PATCH] config: use a static lock_file struct

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

Re: [PATCH] config: use a static lock_file struct

On 08/30, Jeff King wrote: > On Wed, Aug 30, 2017 at 08:55:01AM +0200, Michael Haggerty wrote: > > > > + tempfile->active = 0; > > > + for (p = _list; *p; p = &(*p)->next) { > > > + if (*p == tempfile) { > > > + *p = tempfile->next; > > > + break; > > > +

Re: [PATCH] config: use a static lock_file struct

On Wed, Aug 30, 2017 at 08:55:01AM +0200, Michael Haggerty wrote: > > + tempfile->active = 0; > > + for (p = _list; *p; p = &(*p)->next) { > > + if (*p == tempfile) { > > + *p = tempfile->next; > > + break; > > + } > > } > > } > >

Re: [PATCH] config: use a static lock_file struct

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

Re: [PATCH] config: use a static lock_file struct

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

Re: [PATCH] config: use a static lock_file struct

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

Re: [PATCH] config: use a static lock_file struct

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

Re: [PATCH] config: use a static lock_file struct

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

Re: [PATCH] config: use a static lock_file struct

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

Re: [PATCH] config: use a static lock_file struct

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_

Re: [PATCH] config: use a static lock_file struct

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

Re: [PATCH] config: use a static lock_file struct

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

Re: [PATCH] config: use a static lock_file struct

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

[PATCH] config: use a static lock_file struct

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 neve