git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Brandon Williams <bmwill@google.com>
To: Eric Wong <e@80x24.org>
Cc: Jonathan Nieder <jrnieder@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH 5/5] run-command: add note about forking and threading
Date: Tue, 11 Apr 2017 10:34:45 -0700	[thread overview]
Message-ID: <20170411173445.GF36152@google.com> (raw)
In-Reply-To: <20170411005348.GA30913@starla>

On 04/11, Eric Wong wrote:
> Jonathan Nieder <jrnieder@gmail.com> wrote:
> > Why can't git use e.g. posix_spawn to avoid this?
> 
> posix_spawn does not support chdir, and it seems we run non-git
> commands so no using "git -C" for those.

This is actually the biggest reason why I didn't go down that route from
the start.  I didn't want to dig through each and every user of
run-command and verify that removing chdir wouldn't break them (or add
in some other way to do it).

> 
> > fork()-ing in a threaded context is very painful for maintainability.
> > Any library function you are using could start taking a lock, and then
> > you have a deadlock.  So you have to make use of a very small
> > whitelisted list of library functions for this to work.
> 
> Completely agreed.

Yes it is difficult to get right, but it seems very doable to just do
all of the heavy-lifting prior to fork/exec making it easier to just use
async-safe between the fork/exec in the child.

> 
> On the other hand, I believe we should make run-command
> vfork-compatible (and Brandon's series is a big (but incomplete)
> step in the (IMHO) right direction); as anything which is
> vfork-safe would also be safe in the presence of threads+(plain) fork.
> With vfork; the two processes share heap until execve.

I haven't looked to much into vfork, one of the benefits of vfork is
that it is slightly more preferment than vanilla fork correct?  What are
some of the other benefits of using vfork over fork?

> 
> I posted some notes about it last year:
> 
>   https://public-inbox.org/git/20160629200142.GA17878@dcvr.yhbt.net/
> 
> > The function calls you have to audit are not only between fork() and
> > exec() in the normal control flow.  You have to worry about signal
> > handlers, too.
> 
> Yes, all that auditing is necessary for vfork; too, but totally
> doable.  The mainline Ruby implementation has been using vfork
> for spawning subprocesses for several years, now; and I think the
> ruby-core developers (myself included) have fixed all the
> problems with it; even in multi-threaded code which calls malloc.

-- 
Brandon Williams

  parent reply	other threads:[~2017-04-11 17:34 UTC|newest]

Thread overview: 140+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-10 23:49 [PATCH 0/5] forking and threading Brandon Williams
2017-04-10 23:49 ` [PATCH 1/5] run-command: convert sane_execvp to sane_execvpe Brandon Williams
2017-04-12 19:22   ` Brandon Williams
2017-04-10 23:49 ` [PATCH 2/5] run-command: prepare argv before forking Brandon Williams
2017-04-10 23:49 ` [PATCH 3/5] run-command: allocate child_err " Brandon Williams
2017-04-10 23:49 ` [PATCH 4/5] run-command: prepare child environment " Brandon Williams
2017-04-11  0:58   ` Jonathan Nieder
2017-04-11 17:27     ` Brandon Williams
2017-04-11 17:30       ` Jonathan Nieder
2017-04-10 23:49 ` [PATCH 5/5] run-command: add note about forking and threading Brandon Williams
2017-04-11  0:26   ` Jonathan Nieder
2017-04-11  0:53     ` Eric Wong
2017-04-11 17:33       ` Jonathan Nieder
2017-04-11 17:34       ` Brandon Williams [this message]
2017-04-11 17:40         ` Eric Wong
2017-04-11  7:05 ` [PATCH 6/5] run-command: avoid potential dangers in forked child Eric Wong
2017-04-11 16:29   ` Brandon Williams
2017-04-11 16:59     ` Eric Wong
2017-04-11 17:17       ` Brandon Williams
2017-04-11 17:37 ` [PATCH 0/5] forking and threading Jonathan Nieder
2017-04-11 17:54   ` Brandon Williams
2017-04-13 18:32 ` [PATCH v2 0/6] " Brandon Williams
2017-04-13 18:32   ` [PATCH v2 1/6] t5550: use write_script to generate post-update hook Brandon Williams
2017-04-13 20:43     ` Jonathan Nieder
2017-04-13 20:59       ` Eric Wong
2017-04-13 21:35         ` Brandon Williams
2017-04-13 21:39           ` Eric Wong
2017-04-13 18:32   ` [PATCH v2 2/6] run-command: prepare command before forking Brandon Williams
2017-04-13 21:14     ` Jonathan Nieder
2017-04-13 22:41       ` Brandon Williams
2017-04-13 18:32   ` [PATCH v2 3/6] run-command: prepare child environment " Brandon Williams
2017-04-13 18:32   ` [PATCH v2 4/6] run-command: don't die in child when duping /dev/null Brandon Williams
2017-04-13 19:29     ` Eric Wong
2017-04-13 19:43       ` Brandon Williams
2017-04-13 18:32   ` [PATCH v2 5/6] run-command: eliminate calls to error handling functions in child Brandon Williams
2017-04-13 18:32   ` [PATCH v2 6/6] run-command: add note about forking and threading Brandon Williams
2017-04-13 20:50   ` [PATCH v2 0/6] " Jonathan Nieder
2017-04-13 23:44     ` Brandon Williams
2017-04-13 21:14   ` [PATCH 7/6] run-command: block signals between fork and execve Eric Wong
2017-04-13 23:37     ` Brandon Williams
2017-04-14  2:42     ` Brandon Williams
2017-04-14  5:26       ` Eric Wong
2017-04-14  5:35         ` Eric Wong
2017-04-14 16:58   ` [PATCH v3 00/10] forking and threading Brandon Williams
2017-04-14 16:58     ` [PATCH v3 01/10] t5550: use write_script to generate post-update hook Brandon Williams
2017-04-14 16:58     ` [PATCH v3 02/10] t0061: run_command executes scripts without a #! line Brandon Williams
2017-04-14 16:58     ` [PATCH v3 03/10] run-command: prepare command before forking Brandon Williams
2017-04-14 16:58     ` [PATCH v3 04/10] run-command: use the async-signal-safe execv instead of execvp Brandon Williams
2017-04-14 16:58     ` [PATCH v3 05/10] run-command: prepare child environment before forking Brandon Williams
2017-04-14 16:58     ` [PATCH v3 06/10] run-command: don't die in child when duping /dev/null Brandon Williams
2017-04-14 19:38       ` Eric Wong
2017-04-14 20:19         ` Brandon Williams
2017-04-14 16:58     ` [PATCH v3 07/10] run-command: eliminate calls to error handling functions in child Brandon Williams
2017-04-14 18:50       ` Eric Wong
2017-04-14 20:22         ` Brandon Williams
2017-04-14 16:59     ` [PATCH v3 08/10] run-command: handle dup2 and close errors " Brandon Williams
2017-04-14 16:59     ` [PATCH v3 09/10] run-command: add note about forking and threading Brandon Williams
2017-04-14 16:59     ` [PATCH v3 10/10] run-command: block signals between fork and execve Brandon Williams
2017-04-14 20:24       ` Brandon Williams
2017-04-14 21:35         ` Eric Wong
2017-04-17 22:08     ` [PATCH v4 00/10] forking and threading Brandon Williams
2017-04-17 22:08       ` [PATCH v4 01/10] t5550: use write_script to generate post-update hook Brandon Williams
2017-04-17 22:08       ` [PATCH v4 02/10] t0061: run_command executes scripts without a #! line Brandon Williams
2017-04-17 22:08       ` [PATCH v4 03/10] run-command: prepare command before forking Brandon Williams
2017-04-17 22:08       ` [PATCH v4 04/10] run-command: use the async-signal-safe execv instead of execvp Brandon Williams
2017-04-17 22:08       ` [PATCH v4 05/10] run-command: prepare child environment before forking Brandon Williams
2017-04-18  0:26         ` Eric Wong
2017-04-18 21:02           ` Brandon Williams
2017-04-17 22:08       ` [PATCH v4 06/10] run-command: don't die in child when duping /dev/null Brandon Williams
2017-04-17 22:08       ` [PATCH v4 07/10] run-command: eliminate calls to error handling functions in child Brandon Williams
2017-04-17 22:08       ` [PATCH v4 08/10] run-command: handle dup2 and close errors " Brandon Williams
2017-04-17 22:08       ` [PATCH v4 09/10] run-command: add note about forking and threading Brandon Williams
2017-04-17 22:08       ` [PATCH v4 10/10] run-command: block signals between fork and execve Brandon Williams
2017-04-18 23:17       ` [PATCH v5 00/11] forking and threading Brandon Williams
2017-04-18 23:17         ` [PATCH v5 01/11] t5550: use write_script to generate post-update hook Brandon Williams
2017-04-18 23:17         ` [PATCH v5 02/11] t0061: run_command executes scripts without a #! line Brandon Williams
2017-04-19  5:43           ` Johannes Sixt
2017-04-19  6:21             ` Johannes Sixt
2017-04-19 15:56               ` Brandon Williams
2017-04-19 18:18                 ` Johannes Sixt
2017-04-20 10:47                 ` Johannes Schindelin
2017-04-20 17:02                   ` Brandon Williams
2017-04-20 20:24                     ` Johannes Schindelin
2017-04-20 20:49                       ` Brandon Williams
2017-04-18 23:17         ` [PATCH v5 03/11] run-command: prepare command before forking Brandon Williams
2017-04-18 23:17         ` [PATCH v5 04/11] run-command: use the async-signal-safe execv instead of execvp Brandon Williams
2017-04-18 23:17         ` [PATCH v5 05/11] string-list: add string_list_remove function Brandon Williams
2017-04-18 23:31           ` Stefan Beller
2017-04-18 23:36             ` Brandon Williams
2017-04-18 23:40               ` Stefan Beller
2017-04-18 23:18         ` [PATCH v5 06/11] run-command: prepare child environment before forking Brandon Williams
2017-04-18 23:18         ` [PATCH v5 07/11] run-command: don't die in child when duping /dev/null Brandon Williams
2017-04-18 23:18         ` [PATCH v5 08/11] run-command: eliminate calls to error handling functions in child Brandon Williams
2017-04-18 23:18         ` [PATCH v5 09/11] run-command: handle dup2 and close errors " Brandon Williams
2017-04-18 23:18         ` [PATCH v5 10/11] run-command: add note about forking and threading Brandon Williams
2017-04-18 23:18         ` [PATCH v5 11/11] run-command: block signals between fork and execve Brandon Williams
2017-04-19  6:00           ` Johannes Sixt
2017-04-19  7:48             ` Eric Wong
2017-04-19 16:10               ` Brandon Williams
2017-04-19 23:13         ` [PATCH v6 00/11] forking and threading Brandon Williams
2017-04-19 23:13           ` [PATCH v6 01/11] t5550: use write_script to generate post-update hook Brandon Williams
2017-04-19 23:13           ` [PATCH v6 02/11] t0061: run_command executes scripts without a #! line Brandon Williams
2017-04-20 10:49             ` Johannes Schindelin
2017-04-20 16:58               ` Brandon Williams
2017-04-19 23:13           ` [PATCH v6 03/11] run-command: prepare command before forking Brandon Williams
2017-04-19 23:13           ` [PATCH v6 04/11] run-command: use the async-signal-safe execv instead of execvp Brandon Williams
2017-05-17  2:15             ` Junio C Hamano
2017-05-17  2:26               ` Jeff King
2017-05-17  2:28                 ` Jeff King
2017-05-17  3:41                 ` Junio C Hamano
2017-05-17 14:52                 ` Brandon Williams
2017-04-19 23:13           ` [PATCH v6 05/11] string-list: add string_list_remove function Brandon Williams
2017-04-19 23:13           ` [PATCH v6 06/11] run-command: prepare child environment before forking Brandon Williams
2017-04-19 23:13           ` [PATCH v6 07/11] run-command: don't die in child when duping /dev/null Brandon Williams
2017-04-19 23:13           ` [PATCH v6 08/11] run-command: eliminate calls to error handling functions in child Brandon Williams
2017-04-19 23:13           ` [PATCH v6 09/11] run-command: handle dup2 and close errors " Brandon Williams
2017-04-19 23:13           ` [PATCH v6 10/11] run-command: add note about forking and threading Brandon Williams
2017-04-19 23:13           ` [PATCH v6 11/11] run-command: block signals between fork and execve Brandon Williams
2017-04-24 22:37           ` [PATCH v6 00/11] forking and threading Brandon Williams
2017-04-24 23:50             ` [PATCH v6 12/11] run-command: don't try to execute directories Brandon Williams
2017-04-25  0:17               ` Jonathan Nieder
2017-04-25  1:58                 ` Junio C Hamano
2017-04-25  2:51                   ` Jonathan Nieder
2017-04-25  2:56                 ` Jeff King
2017-04-25  1:47               ` Junio C Hamano
2017-04-25  2:57               ` Jonathan Nieder
2017-04-25 17:54               ` [PATCH v7 1/2] exec_cmd: expose is_executable function Brandon Williams
2017-04-25 17:54                 ` [PATCH v7 2/2] run-command: don't try to execute directories Brandon Williams
2017-04-25 18:51                   ` Jonathan Nieder
2017-04-25 19:32                     ` Brandon Williams
2017-04-25 18:04                 ` [PATCH v7 1/2] exec_cmd: expose is_executable function Jonathan Nieder
2017-04-25 18:18                 ` Johannes Sixt
2017-04-25 18:38                   ` Brandon Williams
2017-04-25 23:46                 ` [PATCH v8 1/2] run-command: " Brandon Williams
2017-04-25 23:47                   ` [PATCH v8 2/2] run-command: restrict PATH search to executable files Brandon Williams
2017-04-25 23:50                     ` Jonathan Nieder
2017-04-26  1:44                     ` Junio C Hamano
2017-04-26 17:10                       ` [PATCH v9 " Brandon Williams
2017-04-27  0:33                         ` Junio C Hamano
2017-04-25 23:48                   ` [PATCH v8 1/2] run-command: expose is_executable function Jonathan Nieder

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=20170411173445.GF36152@google.com \
    --to=bmwill@google.com \
    --cc=e@80x24.org \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    /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).