From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Tom Gillespie Newsgroups: gmane.emacs.bugs Subject: bug#56002: src/process.c; make-process fails to clean up stderr process on early exit Date: Wed, 10 Aug 2022 19:33:22 -0700 Message-ID: References: <83pmj9qjgk.fsf@gnu.org> <8735e70xwl.fsf@gnus.org> <831qtrvtg5.fsf@gnu.org> <83czd9ve0n.fsf@gnu.org> <8335e4q8gm.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="2644"; mail-complaints-to="usenet@ciao.gmane.io" Cc: larsi@gnus.org, 56002@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Aug 11 04:34:10 2022 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oLy1S-0000S4-14 for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 11 Aug 2022 04:34:10 +0200 Original-Received: from localhost ([::1]:41104 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oLy1Q-000666-V1 for geb-bug-gnu-emacs@m.gmane-mx.org; Wed, 10 Aug 2022 22:34:08 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:32822) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oLy1K-00065y-FY for bug-gnu-emacs@gnu.org; Wed, 10 Aug 2022 22:34:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:33065) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1oLy1K-0000k8-7g for bug-gnu-emacs@gnu.org; Wed, 10 Aug 2022 22:34:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1oLy1K-0007HY-46 for bug-gnu-emacs@gnu.org; Wed, 10 Aug 2022 22:34:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Tom Gillespie Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 11 Aug 2022 02:34:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 56002 X-GNU-PR-Package: emacs Original-Received: via spool by 56002-submit@debbugs.gnu.org id=B56002.166018522227967 (code B ref 56002); Thu, 11 Aug 2022 02:34:02 +0000 Original-Received: (at 56002) by debbugs.gnu.org; 11 Aug 2022 02:33:42 +0000 Original-Received: from localhost ([127.0.0.1]:51047 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oLy0z-0007H0-Re for submit@debbugs.gnu.org; Wed, 10 Aug 2022 22:33:42 -0400 Original-Received: from mail-pl1-f171.google.com ([209.85.214.171]:35697) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oLy0x-0007Gj-J1 for 56002@debbugs.gnu.org; Wed, 10 Aug 2022 22:33:40 -0400 Original-Received: by mail-pl1-f171.google.com with SMTP id y1so10230262plb.2 for <56002@debbugs.gnu.org>; Wed, 10 Aug 2022 19:33:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc; bh=XpqktmbdTf812ew2yJYIx4wkD0L4/ekKD4/iqD9vmB0=; b=RokQxXwlRyX6xutrdpMcB7biXaXPskKJtsUR908SC2GYZ0/X51Vu2itK/VeESC0/KR ei4chxHiNaKB0w084bRTVLwCWsJNeGJ6ZSC/mo8wnoThHAUR8URpUZYFnNZD2vyZl6bY m1M1OedP+SL3y4Go8wU0Z5zA1e094sjqqE1P7sKnOqHjpFvylHOS4WaeJkCeQOoxmhyj qz1DosgZPmtvWamdzxCtryYljJdYAbU7XJdbr1eXKnShk2+FMvhon7+7Q4y8uTWYvBzB QJ9KRI+DhafBksx+tfSvuYZ8W1pz6qW6L9qtVS/NbrLGKx4o7R8sArdY8DAc7/vfLFMx 00Iw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc; bh=XpqktmbdTf812ew2yJYIx4wkD0L4/ekKD4/iqD9vmB0=; b=RgOxaCAXJZxGlCAnxf6S4UJHPOdXVD2UwsrTCcLJtK1h/rDYBFJVCaQy1Ulk/i2hm2 pjwVZ7iRD6Bzo9c3QhOfC/30vrb8kFFTQwaZhzFY+gwiUv1Ux2nWGqjbd5dcWMflZso3 5RRnwMRHSoUufqonzdHZh9n0FgWPExeRqd/BDtBryuax+40m5VYi3r11/u2/Q4DmWpCU 6BgVEPcf5pQiCY5L4HryGf4bo0FuffMMct/4LcOeYPX3pCCkr9t3zeJorlsM4VRYvOHy 2KugCtMjmMlSYVFyY2B/Uzuy6SXOLICA2PVQysSKQYS6FkB4DMJEBuwhYnHvMEj2A835 WzuA== X-Gm-Message-State: ACgBeo0fUVtCaHbtlWOV730Cmny3cHCjGzRgcjbgPn0fRSL7brNo4KIS CrY6EJjAnvwF0Gi4RBF6tBS+86qENlQ8e/8zJvI= X-Google-Smtp-Source: AA6agR6BtodDR+TSYmwah5ZOtJFuMMNxkgZJwqhUwvZ8NJvYDhGxv8CHsXaOaXf1O+9Tgdha4tA0Mb0wWEp5pyhlLI4= X-Received: by 2002:a17:90a:d783:b0:1f4:e30b:ece with SMTP id z3-20020a17090ad78300b001f4e30b0ecemr6623445pju.165.1660185213577; Wed, 10 Aug 2022 19:33:33 -0700 (PDT) In-Reply-To: <8335e4q8gm.fsf@gnu.org> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:239321 Archived-At: So I realized that the current patch only fixes half the problems but it does fix them completely. Your concerns are well founded but are only relevant to the half of the problems that this patch does not fix (more detail below). Thus, this patch can be merged as is and a separate patch that uses unwind protect is needed to deal with failures that happen inside create_process. An additional test is needed for that as well since the current test only triggers a failure in Fmake_process prior to the call to create process. Ideally the existing patches would be merged and I will submit another one with the fix for issues in create_process. The function used in unwind protectis going to be more complex and will need more review so we should not hold up the existing patch to wait for that one. > Sorry, I don't understand: stderrproc in this case is not a real > process, it's just a process object. So why does it need to receive a > signal? Sorry, terminology mixup here. I'm pretty sure that it is the kernel that is closing the stderrproc file descriptor, which I assume status_notify or something similar detects and then reaps the stderrproc. Because the stderrproc file descriptor never makes it to the kernel that fd can never be closed when the primary process exits so status_notify (or similar) will see process-status as open forever unless the user intervenes. > To clean it up, make-process "just" needs to make sure this "process" > is killed and its resources released before it returns unsuccessfully. > Right? Yes, if you create stderrproc too early. No, if it is never created at a point where it would have to be cleaned up. We can avoid any errors caused by failures in Fmake_process before the call to create_process by moving the creation of stderrproc as I have done here. That change is safe from any implicit order dependencies as it is all internal to emacs. We still need the cleanup code to handle failures inside the call to create_process. However those should come in another patch, because the changes are not as simple. > For example, you are talking about vfork all the time, so I presume > you didn't analyze what happens in a build that uses posix_spawn > instead (see emacs_spawn), or when we launch subprocesses on > MS-Windows. They use different system calls in different orders, and > I worry that we could introduce subtle bugs by rocking this delicate > boat. As I just realized, this patch only mitigates cases where the cause of the error was a failure inside Fmake_process. We need another patch to deal with failures inside create_process as you suggest. For the current patch there are no cases where code in Fmake_process interacts with the operating system in ways that are of concern for the implicit order dependencies because all implicit os stuff happens inside create_process (thus the second set of patches). > Maybe I'm misunderstand something here, but the usual way of doing > that is to use record_unwind_protect immediately after creating the > stderr process, with a suitable unwind function that would perform the > necessary cleanup. This ensures that however we exit make-process, > the cleanup is never missed, and we don't leak resources. > > Why cannot we do this here? What am I missing? We could but do not need to for the issues inside Fmake_process since we can avoid writing any new code and move the call to Fmake_pipe_process to immediately before the call to create_process. There is no risk of unexpected interactions with os conventions prior to the call to create_process. For any error happening in create_process before a successful return from emacs_spawn we do need to use record unwind protect. The function needed to do that cleanup safely is not as simple and should be in an independent patch that can be reviewed separately.