From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com [IPv6:2a00:1450:4864:20::12c]) by sourceware.org (Postfix) with ESMTPS id BC2303853820 for ; Mon, 28 Jun 2021 01:11:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org BC2303853820 Received: by mail-lf1-x12c.google.com with SMTP id d16so29103224lfn.3 for ; Sun, 27 Jun 2021 18:11:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vrkRbQJnKG+D0K80z6kcZQyaYKW2RqbkI73Mq5MJTsY=; b=iOf0YZ7JQyBIuaDAY13xwTpYTnX//jILHty1KlTpzHkWmYDZBmYo6Ud114XpIrOROi g4twqtc9mXWUz0ks9aCkytqUis/pdna/+je7PdnyUSEW0ErTiyOaK8vjHa57WZcVyGd6 rEMF0u9SbhYDnQDHl0pyZEkrJyIcIF72cAkOvUuMPeKMW4lGZ+rPswCBq3LYYygphf5C BSRi08fwE1sDm4LT1rDoJQKzyRpcplG+8bGRmJdfTAxGXJlKZniT1Din6cupoA8gTxpg dX5pgX4eZu5Ce8cP14420VqRvrhWCuTITtjPrVsfBkNfA2ElaOJnChJPIhu5oW22DWCI D1nQ== X-Gm-Message-State: AOAM531rYsurxKPxphDSyyq5IfQDc14sXkoAfqwRcTSqraC4DVWVKEzR n7hULej3bUepIUHPzcPCIoLC92p8W8BImNxda1DB+EXMM79VYg== X-Google-Smtp-Source: ABdhPJyaEtBoFkQrnxSoRrxR0IXzYx5zKHiACD9OZELZdeY8Ou6bdFjVWPFBOilJzXMw/wBzI+DHBp4k5UqOQ5dXFPg= X-Received: by 2002:ac2:4d81:: with SMTP id g1mr16942981lfe.319.1624842699476; Sun, 27 Jun 2021 18:11:39 -0700 (PDT) MIME-Version: 1.0 References: <20210617175751.1619846-1-adhemerval.zanella@linaro.org> <20210617175751.1619846-2-adhemerval.zanella@linaro.org> In-Reply-To: <20210617175751.1619846-2-adhemerval.zanella@linaro.org> From: Godmar Back Date: Sun, 27 Jun 2021 21:11:27 -0400 Message-ID: Subject: Re: [PATCH 2/2] posix: Add terminal control setting support for posix_spawn To: Adhemerval Zanella Cc: libc-alpha@sourceware.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Jun 2021 01:11:42 -0000 Thank you for cc'ing me. I am assuming you're asking for feedback on your patch. A few notes with what I've noticed. (1) +/* Returh the associated terminal FD in the attribute structure. */ Should it be "Return?" (2) + int __tcpgrp; This field holds a file descriptor, but the name '__tcpgrp' could be misinterpreted as a "pgrp" - as in "terminal control process group" If would suggest a different name perhaps, such as '__ctty_fd' or '__tcfd' or something like that to make it more clear that the field contains a file descriptor. (3) Is passing '0600' necessary if you open with O_RDONLY as in: + int fd = xopen (_PATH_TTY, O_RDONLY, 0600); (4) I didn't see any tests, and I'm not sure the code would work as it's written. Specifically, the child process needs to block (mask) SIGTTOU around the tcsetpgrp() call. This is because after it calls `setpgid` to place itself into a new process group, it will no longer be in the terminal's foreground process group (by definition, all process groups that are not the foreground process group are "background process groups"). Consequently, attempts to call `tcsetpgrp()` will result in SIGTTOU, which must be blocked. See tcgetpgrp(3): If tcsetpgrp() is called by a member of a background process group in its session, and the calling process is not blocking or ignoring SIGTTOU, a SIGTTOU signal is sent to all members of this background process group. Try the following test program to see this: #include #include #include #include #include #include #include int main() { pid_t pgid = 0; int ttyfd = open("/dev/tty", O_RDONLY); // typical sequence if user types 'a | b' (without the pipes) for (int i = 0; i < 2; i++) { int child = fork(); if (child == 0) { setpgid(0, pgid); // you need this call // signal(SIGTTOU, SIG_IGN); int rc = tcsetpgrp(ttyfd, getpgrp()); if (rc != 0) { perror("tcsetpgrp"); abort(); } printf("child process %d group %d successfully has terminal ownership\n", getpid(), getpgrp()); exit(0); } else { setpgid(child, pgid); } if (pgid == 0) pgid = child; printf("spawned #%d\n", i+1); } for (int i = 0; i < 2; i++) { int status; int child = waitpid(-1, &status, WUNTRACED); if (WIFSTOPPED(status)) { printf("child %d was stopped with %s\n", child, strsignal(WSTOPSIG(status))); } else if (WIFEXITED(status)) { printf("child %d exited with %d\n", child, WEXITSTATUS(status)); } } } (5) Since you have to block (or ignore) SIGTTOU around the call to tcsetpgrp, you would have to possibly restore the original signal state before exec'ing the child. This may interact with POSIX_SPAWN_SETSIGMASK and/or POSIX_SPAWN_SETSIGDEF if the caller to posix_spawn() includes SIGTTOU there. In either case, the correct semantics would have to be implemented.