From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp0 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id yGuCCBERXl8MNQAA0tVLHw (envelope-from ) for ; Sun, 13 Sep 2020 12:31:13 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp0 with LMTPS id 6OvxAhERXl8yUQAA1q6Kng (envelope-from ) for ; Sun, 13 Sep 2020 12:31:13 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 47125940539 for ; Sun, 13 Sep 2020 12:31:12 +0000 (UTC) Received: from localhost ([::1]:59224 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kHR9z-0008BT-74 for larch@yhetil.org; Sun, 13 Sep 2020 08:31:11 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:48486) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kHR9q-00089c-Qy for guix-patches@gnu.org; Sun, 13 Sep 2020 08:31:02 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:38083) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kHR9q-000112-GT for guix-patches@gnu.org; Sun, 13 Sep 2020 08:31:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1kHR9q-0006tV-DM for guix-patches@gnu.org; Sun, 13 Sep 2020 08:31:02 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#43367] [core-updates]: [PATCH 0/5]: Prevent wrap-progam from double-wrapping. Resent-From: Brendan Tildesley Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Sun, 13 Sep 2020 12:31:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 43367 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Danny Milosavljevic Cc: 43367@debbugs.gnu.org Received: via spool by 43367-submit@debbugs.gnu.org id=B43367.160000025426488 (code B ref 43367); Sun, 13 Sep 2020 12:31:02 +0000 Received: (at 43367) by debbugs.gnu.org; 13 Sep 2020 12:30:54 +0000 Received: from localhost ([127.0.0.1]:49629 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kHR9h-0006t9-OQ for submit@debbugs.gnu.org; Sun, 13 Sep 2020 08:30:54 -0400 Received: from mout-p-202.mailbox.org ([80.241.56.172]:57644) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kHR9f-0006sv-5P for 43367@debbugs.gnu.org; Sun, 13 Sep 2020 08:30:53 -0400 Received: from smtp2.mailbox.org (smtp2.mailbox.org [IPv6:2001:67c:2050:105:465:1:2:0]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-384) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-p-202.mailbox.org (Postfix) with ESMTPS id 4Bq83S4hDrzQlCD; Sun, 13 Sep 2020 14:30:44 +0200 (CEST) X-Virus-Scanned: amavisd-new at heinlein-support.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=brendan.scot; s=MBO0001; t=1600000242; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=tRuW+V+WefVEkxjDfu+DweLR/5Jyoz7XaiAb6acE2Sw=; b=dus1NkkC/2yLgF+9Ub3SSYHylmzGHhQlHSf5ZCSErc63ZIriovArDDvHI2ckBgFVMqjolg eleyXAgssodq1JUxmEjMJHWEocw8p1j5OoSYNUcI3lX4yv7tBXnSuM44qY6ofuBJvTvEyK BwzfQp2jHMf++jIdQ/5RGTmNsHHDQHIFgWNK5X0n18rlXEWsj/GqcYSfH2KlBaXfBVth7H SuFZ61P1UbdOwoHezpjkTF/IwFP8tTpVEWYlM97DimGmkbhiGcKBGpnNJL/WuAP9p/9MCM MQT1kOMO57Ha1rQh1KmxaP/7DMS2R2S/PBw1qUnEwIFkcOsRy61eoEbRD/MKeQ== Received: from smtp2.mailbox.org ([80.241.60.241]) by gerste.heinlein-support.de (gerste.heinlein-support.de [91.198.250.173]) (amavisd-new, port 10030) with ESMTP id q8l3pfA_tjvK; Sun, 13 Sep 2020 14:30:40 +0200 (CEST) References: <83311dc4-6e9b-e70b-e379-9993bfcd0554@brendan.scot> <20200913114019.58a6bda0@scratchpost.org> From: Brendan Tildesley Message-ID: <6c55ad27-6661-c84f-53ec-0baaa9c9ce91@brendan.scot> Date: Sun, 13 Sep 2020 22:30:50 +1000 MIME-Version: 1.0 In-Reply-To: <20200913114019.58a6bda0@scratchpost.org> Content-Type: multipart/alternative; boundary="------------04BF2234FA3FE86895F88359" Content-Language: en-US X-MBO-SPAM-Probability: X-Rspamd-Score: -5.24 / 15.00 / 15.00 X-Rspamd-Queue-Id: 49290274 X-Rspamd-UID: b38f96 X-Spam-Score: -0.7 (/) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-Spam-Score: -1.7 (-) X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+larch=yhetil.org@gnu.org Sender: "Guix-patches" X-Scanner: scn0 Authentication-Results: aspmx1.migadu.com; dkim=fail (rsa verify failed) header.d=brendan.scot header.s=MBO0001 header.b=dus1NkkC; dmarc=none; spf=pass (aspmx1.migadu.com: domain of guix-patches-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-patches-bounces@gnu.org X-Spam-Score: 0.99 X-TUID: Yhg2t6yWsCLw This is a multi-part message in MIME format. --------------04BF2234FA3FE86895F88359 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit On 13/9/20 7:40 pm, Danny Milosavljevic wrote: > On Sun, 13 Sep 2020 15:39:15 +1000 > Brendan Tildesley wrote: > >> I'm attempting to fix a bug where wrap-program produces ..X-real-real >> files by mistakenly wrapping already wrapped files. I haven't fully >> tested these because it requires rebuilding everything which takes hours >> to days and core-updates is stuck on mesa now anyway. Perhaps I'll try >> testing on master. Also there may be other places where .X-real files >> are accidentally wrapped, which will now error. > But can't a thing be wrapped once for one reason and another time for another > reason and that should be fine? Yes, perhaps I should have explained that this is still possible and works fine. When a program is wrapped a second time, it will append to the existed wrapper, rather than creating a new file and moving the old one. repeated applications of wrap-program after the first one simply append. I'll illustrate how this can go wrong though: suppose we have /bin/foo and we we are in a repl and run: (wrap-program "/bin/foo" `("BAR" = ("baz")) => /bin/.foo-real doesn't exist so /bin/foo is moved to /bin/.foo-real, a new /bin/foo is created that is a wrapper that then launches /bin.foo-real. (wrap-program "/bin/foo" `("BAR" = ("baz")) => /bin/.foo-real exists so /bin/foo is assumed to already be a wrapper so variables are appended to /bin/foo. (wrap-program "/bin/foo" `("BAR" = ("baz")) => same thing again, variables are appended ; Now suppose we then run: (wrap-program "/bin/.foo-real" `("BAR" = ("baz")) => /bin/..foo-real-real doesn't exist, so /bin/.foo-real is moved to /bin/..foo-real-real and /bin/.foo-real is created again as another wrapper. This should never be done intentionally I think, but sometimes there is code that uses (find-files dir ".") to find binaries to wrap, and this is run after a previous existing wrap phase, so the both /bin/foo and /bin/.foo-real are wrapped again. Generally everything will continue working though despite all this though. You run this to find some of these double wrapped packages: find /gnu/ -maxdepth 4 -iname '.*-real-real' So I thought it best to error whenever this happens instead of allowing it. An example of this causing an issue is when Prafulla Giri posted a patch[0] to fix a bug with Calibre. Their code ought to be correct, but it resulted in double wrapping. I created my own patch by overwriting the python-build-systems wrap phase and duplicating some code. Andreas ended up accepting my patch instead. ... Actually I just realised Prafulla's patch could have been fixed in a much simpler way by adjusting the (find-files ...) bit and avoided duplication. ... Anyway, with these patches, Prafulla's patch would have caused an error and forced them to fix it, for example, by changing (find-files "." ".") to (find-files "." (lambda (file stat) (not (wrapper? file)))) or (find-files "." (lambda (file stat) (not (string-prefix "." (basename file)))) ---------- So, the main change here is making (wrap-program ".foo-real") an error. If you cannot think of a good reason why that should ever be run, I think its good to block it. bugs that can slip through easily and lurk in the background usually not causing problems are not good in my opinion. After that has been decided we need to ensure all build systems don't misuse wrap-program that way. I notice some build systems actually only pass 'regular files, others allow symlinks or any file. I'm not really sure what the exact find-files filter should be. [0] https://lists.gnu.org/archive/html/guix-patches/2020-09/msg00219.html --------------04BF2234FA3FE86895F88359 Content-Type: text/html; charset=windows-1252 Content-Transfer-Encoding: 7bit
On 13/9/20 7:40 pm, Danny Milosavljevic wrote:
On Sun, 13 Sep 2020 15:39:15 +1000
Brendan Tildesley <mail@brendan.scot> wrote:

I'm attempting to fix a bug where wrap-program produces ..X-real-real 
files by mistakenly wrapping already wrapped files. I haven't fully 
tested these because it requires rebuilding everything which takes hours 
to days and core-updates is stuck on mesa now anyway. Perhaps I'll try 
testing on master. Also there may be other places where .X-real files 
are accidentally wrapped, which will now error.
But can't a thing be wrapped once for one reason and another time for another
reason and that should be fine?

Yes, perhaps I should have explained that this is still possible and works fine. When a program is wrapped a second time, it will append to the existed wrapper, rather than creating a new file and moving the old one. repeated applications of wrap-program after the first one simply append. I'll illustrate how this can go wrong though: suppose we have /bin/foo and we we are in a repl and run:

(wrap-program "/bin/foo" `("BAR" = ("baz")) => /bin/.foo-real doesn't exist so /bin/foo is moved to /bin/.foo-real, a new /bin/foo is created that is a wrapper that then launches /bin.foo-real.

(wrap-program "/bin/foo" `("BAR" = ("baz")) => /bin/.foo-real exists so /bin/foo is assumed to already be a wrapper so variables are appended to /bin/foo.

(wrap-program "/bin/foo" `("BAR" = ("baz")) => same thing again, variables are appended

; Now suppose we then run:

(wrap-program "/bin/.foo-real" `("BAR" = ("baz")) => /bin/..foo-real-real doesn't exist, so /bin/.foo-real is moved to /bin/..foo-real-real and /bin/.foo-real is created again as another wrapper.

This should never be done intentionally I think, but sometimes there is code that uses (find-files dir ".") to find binaries to wrap, and this is run after a previous existing wrap phase, so the both /bin/foo and /bin/.foo-real are wrapped again. Generally everything will continue working though despite all this though.

You run this to find some of these double wrapped packages:

find /gnu/ -maxdepth 4 -iname '.*-real-real'

So I thought it best to error whenever this happens instead of allowing it.

An example of this causing an issue is when Prafulla Giri posted a patch[0] to fix a bug with Calibre. Their code ought to be correct, but it resulted in double wrapping. I created my own patch by overwriting the python-build-systems wrap phase and duplicating some code. Andreas ended up accepting my patch instead.

... Actually I just realised Prafulla's patch could have been fixed in a much simpler way by adjusting the (find-files ...) bit and avoided duplication. ...

Anyway, with these patches, Prafulla's patch would have caused an error and forced them to fix it, for example, by changing

(find-files "." ".")

to

(find-files "." (lambda (file stat) (not (wrapper? file))))
or 
(find-files "." (lambda (file stat) (not (string-prefix "." (basename file))))

----------

So, the main change here is making (wrap-program ".foo-real") an error. If you cannot think of a good reason why that should ever be run, I think its good to block it. bugs that can slip through easily and lurk in the background usually not causing problems are not good in my opinion. After that has been decided we need to ensure all build systems don't misuse wrap-program that way. I notice some build systems actually only pass 'regular files, others allow symlinks or any file. I'm not really sure what the exact find-files filter should be.

[0] https://lists.gnu.org/archive/html/guix-patches/2020-09/msg00219.html


  


--------------04BF2234FA3FE86895F88359--