A "cosmetic changes" commit that removes security fixes

A "cosmetic changes" commit that removes security fixes

[Mark H Weaver]

[-- Attachment #1: Type: text/plain, Size: 566 bytes --]

Hello Guix,

Raghav Gururajan has pushed another misleading "cosmetic changes"
commit.  This one is *far* worse than the examples I gave before.
This one removes the security fixes for CVE-2018-19876 and
cairo-CVE-2020-35492 that I had applied in commit
bc16eacc99e801ac30cbe2aa649a2be3ca5c102a.

Behold, Raghav's "cosmetic changes" to our 'cairo' package:

  https://git.savannah.gnu.org/cgit/guix.git/commit/?h=wip-gnome&id=d975ed975456a2c8e855eb024b5487c4c460684a

With this in mind, does anyone else find it worrisome that Raghav has
commit access?

      Mark


[-- Attachment #2: Misleading "cosmetic changes" commit by Raghav Gururajan --]
[-- Type: text/x-patch, Size: 9525 bytes --]

From d975ed975456a2c8e855eb024b5487c4c460684a Mon Sep 17 00:00:00 2001
From: Raghav Gururajan <raghavgururajan@disroot.org>
Date: Fri, 4 Dec 2020 00:48:43 -0500
Subject: [PATCH] gnu: cairo: Make some cosmetic changes.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* gnu/packages/patches/cairo-CVE-2018-19876.patch,
gnu/packages/patches/cairo-CVE-2020-35492.patch: Remove patches.
* gnu/local.mk (dist_patch_DATA): Unregister them.
* gnu/packages/gtk.scm (cairo): Make some cosmetic changes.
[replacement]: Remove.
(cairo/fixed): Remove.

Signed-off-by: Léo Le Bouter <lle-bout@zaclys.net>
---
 gnu/local.mk                                  |  2 -
 gnu/packages/gtk.scm                          | 92 +++++++++----------
 .../patches/cairo-CVE-2018-19876.patch        | 37 --------
 .../patches/cairo-CVE-2020-35492.patch        | 49 ----------
 4 files changed, 41 insertions(+), 139 deletions(-)
 delete mode 100644 gnu/packages/patches/cairo-CVE-2018-19876.patch
 delete mode 100644 gnu/packages/patches/cairo-CVE-2020-35492.patch

diff --git a/gnu/local.mk b/gnu/local.mk
index e01e7970d2..e93ce45f5a 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -877,8 +877,6 @@ dist_patch_DATA =						\
   %D%/packages/patches/bpftrace-disable-bfd-disasm.patch	\
   %D%/packages/patches/busybox-CVE-2021-28831.patch		\
   %D%/packages/patches/byobu-writable-status.patch		\
-  %D%/packages/patches/cairo-CVE-2018-19876.patch		\
-  %D%/packages/patches/cairo-CVE-2020-35492.patch		\
   %D%/packages/patches/calibre-no-updates-dialog.patch		\
   %D%/packages/patches/calibre-remove-test-sqlite.patch		\
   %D%/packages/patches/calibre-remove-test-unrar.patch		\
diff --git a/gnu/packages/gtk.scm b/gnu/packages/gtk.scm
index 2699b97b49..ff0c1296e1 100644
--- a/gnu/packages/gtk.scm
+++ b/gnu/packages/gtk.scm
@@ -123,67 +123,57 @@ tools have full access to view and control running applications.")
 
 (define-public cairo
   (package
-   (name "cairo")
-   (version "1.16.0")
-   (replacement cairo/fixed)
-   (source (origin
-            (method url-fetch)
-            (uri (string-append "https://cairographics.org/releases/cairo-"
-                                version ".tar.xz"))
-            (sha256
-             (base32
-              "0c930mk5xr2bshbdljv005j3j8zr47gqmkry3q6qgvqky6rjjysy"))))
-   (build-system gnu-build-system)
-   (propagated-inputs
-    `(("fontconfig" ,fontconfig)
-      ("freetype" ,freetype)
-      ("glib" ,glib)
-      ("libpng" ,libpng)
-      ("libx11" ,libx11)
-      ("libxext" ,libxext)
-      ("libxrender" ,libxrender)
-      ("pixman" ,pixman)))
-   (inputs
-    `(("ghostscript" ,ghostscript)
-      ("libspectre" ,libspectre)
-      ("poppler" ,poppler)
-      ("xorgproto" ,xorgproto)
-      ("zlib" ,zlib)))
-   (native-inputs
-     `(("pkg-config" ,pkg-config)
-      ("python" ,python-wrapper)))
+    (name "cairo")
+    (version "1.16.0")
+    (source
+     (origin
+       (method url-fetch)
+       (uri
+        (string-append "https://cairographics.org/releases/cairo-"
+                       version ".tar.xz"))
+       (sha256
+        (base32 "0c930mk5xr2bshbdljv005j3j8zr47gqmkry3q6qgvqky6rjjysy"))))
+    (build-system gnu-build-system)
     (arguments
-     `(#:tests? #f  ; see http://lists.gnu.org/archive/html/bug-guix/2013-06/msg00085.html
-       #:configure-flags '("--enable-tee"      ;needed for GNU Icecat
-                           "--enable-xml"      ;for cairo-xml support
-                           "--disable-static")))
-   (synopsis "2D graphics library")
-   (description
-    "Cairo is a 2D graphics library with support for multiple output devices.
-Currently supported output targets include the X Window System (via both
-Xlib and XCB), Quartz, Win32, image buffers, PostScript, PDF, and SVG file
+     `(#:tests? #f ; see http://lists.gnu.org/archive/html/bug-guix/2013-06/msg00085.html
+       #:configure-flags
+       (list
+        "--enable-tee"                    ;needed for GNU Icecat
+        "--enable-xml"                    ;for cairo-xml support
+        "--disable-static")))
+    (native-inputs
+     `(("pkg-config" ,pkg-config)
+       ("python" ,python-wrapper)))
+    (inputs
+     `(("ghostscript" ,ghostscript)
+       ("libspectre" ,libspectre)
+       ("poppler" ,poppler)
+       ("xorgproto" ,xorgproto)
+       ("zlib" ,zlib)))
+    (propagated-inputs
+     `(("fontconfig" ,fontconfig)
+       ("freetype" ,freetype)
+       ("glib" ,glib)
+       ("libpng" ,libpng)
+       ("libx11" ,libx11)
+       ("libxext" ,libxext)
+       ("libxrender" ,libxrender)
+       ("pixman" ,pixman)))
+    (synopsis "2D graphics library")
+    (description "Cairo is a 2D graphics library with support for multiple output
+devices.  Currently supported output targets include the X Window System (via
+both Xlib and XCB), Quartz, Win32, image buffers, PostScript, PDF, and SVG file
 output.  Experimental backends include OpenGL, BeOS, OS/2, and DirectFB.
-
 Cairo is designed to produce consistent output on all output media while
 taking advantage of display hardware acceleration when available
 eg. through the X Render Extension).
-
 The cairo API provides operations similar to the drawing operators of
 PostScript and PDF.  Operations in cairo including stroking and filling cubic
 Bézier splines, transforming and compositing translucent images, and
 antialiased text rendering.  All drawing operations can be transformed by any
 affine transformation (scale, rotation, shear, etc.).")
-   (license license:lgpl2.1) ; or Mozilla Public License 1.1
-   (home-page "https://cairographics.org/")))
-
-(define cairo/fixed
-  (package
-    (inherit cairo)
-    (source (origin
-              (inherit (package-source cairo))
-              (patches (append (search-patches "cairo-CVE-2018-19876.patch"
-                                               "cairo-CVE-2020-35492.patch")
-                               (origin-patches (package-source cairo))))))))
+    (home-page "https://cairographics.org/")
+    (license license:lgpl2.1))) ; or Mozilla Public License 1.1
 
 (define-public cairo-sans-poppler
   ;; Variant used to break the dependency cycle between Poppler and Cairo.
diff --git a/gnu/packages/patches/cairo-CVE-2018-19876.patch b/gnu/packages/patches/cairo-CVE-2018-19876.patch
deleted file mode 100644
index c0fba2ecaa..0000000000
--- a/gnu/packages/patches/cairo-CVE-2018-19876.patch
+++ /dev/null
@@ -1,37 +0,0 @@
-Copied from Debian.
-
-From: Carlos Garcia Campos <cgarcia@igalia.com>
-Date: Mon, 19 Nov 2018 12:33:07 +0100
-Subject: ft: Use FT_Done_MM_Var instead of free when available in
- cairo_ft_apply_variations
-
-Fixes a crash when using freetype >= 2.9
-
-[This is considered to be security-sensitive because WebKitGTK+ sets its
-own memory allocator, which is not compatible with system free(), making
-this a remotely triggerable denial of service or memory corruption.]
-
-Origin: upstream, commit:90e85c2493fdfa3551f202ff10282463f1e36645
-Bug: https://gitlab.freedesktop.org/cairo/cairo/merge_requests/5
-Bug-Debian: https://bugs.debian.org/916389
-Bug-CVE: CVE-2018-19876
----
- src/cairo-ft-font.c | 4 ++++
- 1 file changed, 4 insertions(+)
-
-diff --git a/src/cairo-ft-font.c b/src/cairo-ft-font.c
-index 325dd61..981973f 100644
---- a/src/cairo-ft-font.c
-+++ b/src/cairo-ft-font.c
-@@ -2393,7 +2393,11 @@ skip:
- done:
-         free (coords);
-         free (current_coords);
-+#if HAVE_FT_DONE_MM_VAR
-+        FT_Done_MM_Var (face->glyph->library, ft_mm_var);
-+#else
-         free (ft_mm_var);
-+#endif
-     }
- }
- 
diff --git a/gnu/packages/patches/cairo-CVE-2020-35492.patch b/gnu/packages/patches/cairo-CVE-2020-35492.patch
deleted file mode 100644
index e8b90fa5c5..0000000000
--- a/gnu/packages/patches/cairo-CVE-2020-35492.patch
+++ /dev/null
@@ -1,49 +0,0 @@
-Copied from Debian.
-
-From 03a820b173ed1fdef6ff14b4468f5dbc02ff59be Mon Sep 17 00:00:00 2001
-From: Heiko Lewin <heiko.lewin@worldiety.de>
-Date: Tue, 15 Dec 2020 16:48:19 +0100
-Subject: [PATCH] Fix mask usage in image-compositor
-
-[trimmed test case, since not used in Debian build]
-
----
- src/cairo-image-compositor.c                |   8 ++--
-
---- cairo-1.16.0.orig/src/cairo-image-compositor.c
-+++ cairo-1.16.0/src/cairo-image-compositor.c
-@@ -2601,14 +2601,14 @@ _inplace_src_spans (void *abstract_rende
- 		    unsigned num_spans)
- {
-     cairo_image_span_renderer_t *r = abstract_renderer;
--    uint8_t *m;
-+    uint8_t *m, *base = (uint8_t*)pixman_image_get_data(r->mask);
-     int x0;
- 
-     if (num_spans == 0)
- 	return CAIRO_STATUS_SUCCESS;
- 
-     x0 = spans[0].x;
--    m = r->_buf;
-+    m = base;
-     do {
- 	int len = spans[1].x - spans[0].x;
- 	if (len >= r->u.composite.run_length && spans[0].coverage == 0xff) {
-@@ -2646,7 +2646,7 @@ _inplace_src_spans (void *abstract_rende
- 				      spans[0].x, y,
- 				      spans[1].x - spans[0].x, h);
- 
--	    m = r->_buf;
-+	    m = base;
- 	    x0 = spans[1].x;
- 	} else if (spans[0].coverage == 0x0) {
- 	    if (spans[0].x != x0) {
-@@ -2675,7 +2675,7 @@ _inplace_src_spans (void *abstract_rende
- #endif
- 	    }
- 
--	    m = r->_buf;
-+	    m = base;
- 	    x0 = spans[1].x;
- 	} else {
- 	    *m++ = spans[0].coverage;
-- 
2.31.1

Re: A "cosmetic changes" commit that removes security fixes

[Mark H Weaver]

... and here's another "cosmetic changes" commit from Raghav that
removes all of the security fixes for 'glib' that I had added:

  https://git.savannah.gnu.org/cgit/guix.git/commit/?h=wip-gnome&id=40b58074895ee510cab496655e6bec8d95abe693

Also, both of these commits were marked as:

  "Signed-off-by: Léo Le Bouter <lle-bout@zaclys.net>"

       Mark

Re: A "cosmetic changes" commit that removes security fixes

[Tobias Geerinckx-Rice]

[-- Attachment #1: Type: text/plain, Size: 447 bytes --]

Hi all,

Thanks for keeping a critical eye on WIP branches, Mark.

Raghav, can you explain why you created that commit?  What's the 
context & the goal?  Why is it on current wip-gnome?  What do you 
expect to happen to it?

Léo, all the same questions for you, plus:

Mark H Weaver writes:
> "Signed-off-by: Léo Le Bouter <lle-bout@zaclys.net>"

What does this line mean?  Please explain in some detail.

Kind regards,

T G-R

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Raghav Gururajan]

Hi All!

Sorry, I just saw this email and noticed its thread via web. I wasn't subscribed.

> Raghav, can you explain why you created that commit? What's the 
> context & the goal? Why is it on current wip-gnome? What do you 
> expect to happen to it?

The commit is not new. I cherry-picked from core-updates (993de472ed3dfe90e1c4110b6b910c1f74d243ff), which was pushed as a part of #42958.

>> "Signed-off-by: Léo Le Bouter <lle-bout@zaclys.net>"
> 
> What does this line mean? Please explain in some detail.

The cherry-pick preserves the commit message.

P.S.

Please note that I stopped making cosmetic changes long ago. I explicitly mentioned in #42958, during review, that there are two *old* 'cosmetic changes' (glib and cairo) which I had to preserve so that I don't have to rework the succeeding patches. Please refer this message (https://issues.guix.gnu.org/42958#63).

Regards,
RG.

Re: A "cosmetic changes" commit that removes security fixes

[Leo Prikler]

Hi Mark,

Am Mittwoch, den 21.04.2021, 17:11 -0400 schrieb Mark H Weaver:
> Hello Guix,
> 
> Raghav Gururajan has pushed another misleading "cosmetic changes"
> commit.  This one is *far* worse than the examples I gave before.
> This one removes the security fixes for CVE-2018-19876 and
> cairo-CVE-2020-35492 that I had applied in commit
> bc16eacc99e801ac30cbe2aa649a2be3ca5c102a.
> 
> Behold, Raghav's "cosmetic changes" to our 'cairo' package:
In particular, it is also worse than the glib example you've used,
since at least the glib one is followed up by an update.  This one is
not, at least as far as I can tell.

https://git.savannah.gnu.org/cgit/guix.git/commit/?h=wip-gnome&id=d975ed975456a2c8e855eb024b5487c4c460684a
> 
> With this in mind, does anyone else find it worrisome that Raghav has
> commit access?
> 
>       Mark
It is indeed worrying, that those patches seem to have made it to wip-
gnome with little review.  I believe we inherited this from before work
was done on savannah, as I can't seem to find them within our mailing
lists.  As a side note, that's why I make it a habit not to push any
patches, that I've edited too heavily, instead sending them back to the
mailing list in hope for another reviewer.  Even if those changes seem
merely cosmetic to me, they might have a larger impact than I can
imagine.  However, in taking more time to let patches sit on the
mailing list, I fear that I might come off as "unwilling" to those
contributors, whose work I help review, including Raghav, and also that
my involvement in some patch discussion tells other committers "don't
worry, I got this, do something else".

I don't think we need to strip Raghav's commit rights yet, but at the
same time we ought to more closely monitor what's going on in wip-
gnome.  Being 3 GNOME releases and one c-u merge late, there isn't much
room to allow for fuck-ups, and as we all know, that's when most of
them happen.

Regards,
Leo

Re: A "cosmetic changes" commit that removes security fixes

[Leo Famulari]

On Thu, Apr 22, 2021 at 12:16:13AM +0200, Leo Prikler wrote:
> However, in taking more time to let patches sit on the
> mailing list, I fear that I might come off as "unwilling" to those
> contributors, whose work I help review, including Raghav, and also that
> my involvement in some patch discussion tells other committers "don't
> worry, I got this, do something else".

That's exactly how I think of it. If a committer is reviewing some
patches and wants advice from somebody else, please CC somebody or ask
on IRC.

> Being 3 GNOME releases and one c-u merge late, there isn't much
> room to allow for fuck-ups, and as we all know, that's when most of
> them happen.

We are well aware of the dangers of rushing. It's why I did not try to
include core-updates in the upcoming release, despite it being way
overdue.

Re: A "cosmetic changes" commit that removes security fixes

[Raghav Gururajan]

[-- Attachment #1.1: Type: text/plain, Size: 1758 bytes --]

Hi Mark!

> Raghav Gururajan has pushed another misleading "cosmetic changes"
> commit.

When you brought-up the concern 
(https://lists.gnu.org/archive/html/guix-devel/2020-12/msg00008.html), 
which I am grateful for, I have worked myself to prevent that from 
happening. It was so hard for me provided that I suffer from OCD 
(clinically-diagnosed and being treated for). I never made single "Make 
cosmetic changes" patches after that discussion. These two patches you 
are referring to, was made even before our discussion, as a part of 
wip-desktop work. The patches were pushed to core-updates as a part of 
#42958. Also, during review, I clearly stated about these two cosmetic 
changes patches, in this message (https://issues.guix.gnu.org/42958#64).

> This one is *far* worse than the examples I gave before.
> This one removes the security fixes for CVE-2018-19876 and
> cairo-CVE-2020-35492 that I had applied in commit
> bc16eacc99e801ac30cbe2aa649a2be3ca5c102a.

The commit is not new. I cherry-picked from core-updates 
(993de472ed3dfe90e1c4110b6b910c1f74d243ff), which was pushed as a part 
of #42958.

> Behold, Raghav's "cosmetic changes" to our 'cairo' package:
The commit is also not new. I cherry-picked from core-updates 
(f94cdc86f644984ca83164d40b17e7eed6e22091), which was pushed as a part 
of #42958.

NOTE:
When I format-patched these patches, initially (42958), did not contain 
changes to remove CVE. IIRC, when Leo and I were working outside of 
savannah, this change was probably added when we updated glib to latest 
version.

> With this in mind, does anyone else find it worrisome that Raghav has
> commit access?

I wish you had given me the benefit of the doubt.

Regards,
RG.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Mark H Weaver]

Hi Raghav,

Raghav Gururajan <rg@raghavgururajan.name> writes:

>> Raghav Gururajan has pushed another misleading "cosmetic changes"
>> commit.
[...]
>> This one is *far* worse than the examples I gave before.
>> This one removes the security fixes for CVE-2018-19876 and
>> cairo-CVE-2020-35492 that I had applied in commit
>> bc16eacc99e801ac30cbe2aa649a2be3ca5c102a.
>
> The commit is not new. I cherry-picked from core-updates 
> (993de472ed3dfe90e1c4110b6b910c1f74d243ff), which was pushed as a part 
> of #42958.
>
>> Behold, Raghav's "cosmetic changes" to our 'cairo' package:
> The commit is also not new. I cherry-picked from core-updates 
> (f94cdc86f644984ca83164d40b17e7eed6e22091), which was pushed as a part 
> of #42958.

Those commits on 'core-updates' were digitally signed by Léo Le Bouter
<lle-bout@zaclys.net> and have the same problems: they remove security
fixes, and yet the summary lines indicate that only "cosmetic changes"
were made.

I'm sorry to say that your responses have done nothing to allay my
concerns.

       Mark

Re: A "cosmetic changes" commit that removes security fixes

[Raghav Gururajan]

[-- Attachment #1.1: Type: text/plain, Size: 683 bytes --]

Hi Mark!

> Those commits on 'core-updates' were digitally signed by Léo Le Bouter
> <lle-bout@zaclys.net> and have the same problems: they remove security
> fixes, and yet the summary lines indicate that only "cosmetic changes"
> were made.

Yeah, the commit title didn't mention the change but the commit message did.

> I'm sorry to say that your responses have done nothing to allay my
> concerns.

For glib, IIRC, we updated package to latest version and guix lint 
didn't show any more CVEs. Also, I think the change was added as part of 
the cosmetic change commit, to cleanly apply succeeding patches.

For cairo, let me get back to you.

Regards,
RG.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Raghav Gururajan]

[-- Attachment #1.1: Type: text/plain, Size: 1246 bytes --]

Hi Mark!

> For glib, IIRC, we updated package to latest version and guix lint 
> didn't show any more CVEs. Also, I think the change was added as part of 
> the cosmetic change commit, to cleanly apply succeeding patches.
> 
> For cairo, let me get back to you.

Okay, I was able to retrace. When Leo and I were working outside 
savannah, there was master --> core-updates merge. Leo made these 
changes when he committed to his repo 
(https://logs.guix.gnu.org/guix/2021-03-26.log#000811), from which I 
pulled then format-patched and sent it to guix-patches 
(https://issues.guix.gnu.org/42958#64). From guix-patches it was then 
pushed to core-updates (https://issues.guix.gnu.org/42958#67), from 
where I cherry-picked into wip-gnome.

It seems Leo made these for ungrafting. I not familiar with ungrafting, 
so I have to let Leo explain.

P.S
The commit title for these commits were initially "Ungraft and make some 
cosmetic changes.", I must have screwed up the tile while moving the 
patches. For that my apologies.

[1] 
https://git.sr.ht/~lle-bout/guix/commit/6477daa338fbf1c9edacfc3690aca77cacfe0008
[2] 
https://git.sr.ht/~lle-bout/guix/commit/a045a48dd961f0c5c3d536dcc3fd21d9c08d2d50

Regards,
RG.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Mark H Weaver]

Hi Raghav,

Raghav Gururajan <rg@raghavgururajan.name> writes:

> Okay, I was able to retrace. When Leo and I were working outside 
> savannah, there was master --> core-updates merge. Leo made these 
> changes when he committed to his repo 
> (https://logs.guix.gnu.org/guix/2021-03-26.log#000811), from which I 
> pulled then format-patched and sent it to guix-patches 
> (https://issues.guix.gnu.org/42958#64). From guix-patches it was then 
> pushed to core-updates (https://issues.guix.gnu.org/42958#67), from 
> where I cherry-picked into wip-gnome.
>
> It seems Leo made these for ungrafting. I not familiar with ungrafting, 
> so I have to let Leo explain.
>
> P.S
> The commit title for these commits were initially "Ungraft and make some 
> cosmetic changes.", I must have screwed up the tile while moving the 
> patches. For that my apologies.
>
> [1] 
> https://git.sr.ht/~lle-bout/guix/commit/6477daa338fbf1c9edacfc3690aca77cacfe0008
> [2] 
> https://git.sr.ht/~lle-bout/guix/commit/a045a48dd961f0c5c3d536dcc3fd21d9c08d2d50

Both of these patches have all of the same problems.  The only
difference is that their summary lines say "Ungraft and make some
cosmetic changes."

(1) These original summary lines are still misleading, because "ungraft"
    means to integrate the fixes from the replacement into the original,
    but here, the fixes are simply being deleted.

(2) These original commit logs are still misleading, for the same reason
    I gave in my previous reply.

(3) The 'cairo' commit still re-introduces security flaws into our
    'cairo' package.

What worries me as much as anything is that your responses so far seem
to indicate that you are failing to understand what you and Léo have
done wrong here.

       Mark

Re: A "cosmetic changes" commit that removes security fixes

[Raghav Gururajan]

[-- Attachment #1.1: Type: text/plain, Size: 1006 bytes --]

Hi Mark!

> (1) These original summary lines are still misleading, because "ungraft"
>      means to integrate the fixes from the replacement into the original,
>      but here, the fixes are simply being deleted.

I see. Now I get the idea. Thanks for explaining this.

> (2) These original commit logs are still misleading, for the same reason
>      I gave in my previous reply.
Agreed.

> (3) The 'cairo' commit still re-introduces security flaws into our
>      'cairo' package.

Agreed.

> What worries me as much as anything is that your responses so far seem
> to indicate that you are failing to understand what you and Léo have
> done wrong here.

I never said nothing wrong was done. I was trying to retrace the steps 
and putting all the information out. I was not sure why Leo added these 
changes and I failed to elicit questions at that time. Let us try to fix 
it. I don't know what else to say.

@Leo,
Shall we add a new commit to fix this?

Regards,
RG.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Mark H Weaver]

Hi Raghav,

Raghav Gururajan <rg@raghavgururajan.name> writes:

> Okay, I was able to retrace. When Leo and I were working outside 
> savannah, there was master --> core-updates merge. Leo made these 
> changes when he committed to his repo 
> (https://logs.guix.gnu.org/guix/2021-03-26.log#000811), from which I 
> pulled then format-patched and sent it to guix-patches 
> (https://issues.guix.gnu.org/42958#64). From guix-patches it was then 
> pushed to core-updates (https://issues.guix.gnu.org/42958#67), from 
> where I cherry-picked into wip-gnome.

Thank you for these links.  From the IRC log cited above, it now appears
that Léo Le Bouter <lle-bout@zaclys.net> bears primary responsibility
for these mistakes.  In particular, according to the IRC
logs, Léo wrote:

  <lle-bout> raghavgururajan: the main issues on the rebasing were about
  security fixes on cairo, gdk-pixbuf and glib
  <lle-bout> I modified the cosmetic commits to remove the graft and
  patches etc.

  <https://logs.guix.gnu.org/guix/2021-03-26.log#000950>

> It seems Leo made these for ungrafting. I not familiar with ungrafting, 
> so I have to let Leo explain.

Yes, I would very much like to hear an explanation from Léo about how
this happened.

Nonetheless, you (Raghav) also bear some responsibility for digitally
signing and pushing these misleading commits to the 'wip-gnome' branch.
At least one of the problems (the misleading summary line) should have
been obvious from a cursory glance at the commit log.

      Mark

--
Support Richard Stallman against the vicious misinformation campaign
against him and the FSF.  See <https://stallmansupport.org> for more.

Re: A "cosmetic changes" commit that removes security fixes

[Raghav Gururajan]

[-- Attachment #1.1: Type: text/plain, Size: 1189 bytes --]

Hi Mark!

> Thank you for these links.  From the IRC log cited above, it now appears
> that Léo Le Bouter <lle-bout@zaclys.net> bears primary responsibility
> for these mistakes.  In particular, according to the IRC
> logs, Léo wrote:
> 
>    <lle-bout> raghavgururajan: the main issues on the rebasing were about
>    security fixes on cairo, gdk-pixbuf and glib
>    <lle-bout> I modified the cosmetic commits to remove the graft and
>    patches etc.
> 
>    <https://logs.guix.gnu.org/guix/2021-03-26.log#000950>
Yes, Leo made these changes. But I also have to acknowledge that it was 
part of my responsibility to question any changes. My apologies for 
that. Leo was occupied with many stuff on those days (with CVE bug 
hunting etc.). Its understandable that it was a honest mistake.

> Nonetheless, you (Raghav) also bear some responsibility for digitally
> signing and pushing these misleading commits to the 'wip-gnome' branch.
> At least one of the problems (the misleading summary line) should have
> been obvious from a cursory glance at the commit log.

Yes, my apologies. I will merge fix commits from core-updates to wip-gnome.

Regards,
RG.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Mark H Weaver]

Hi Raghav,

Raghav Gururajan <rg@raghavgururajan.name> writes:

>> Thank you for these links.  From the IRC log cited above, it now appears
>> that Léo Le Bouter <lle-bout@zaclys.net> bears primary responsibility
>> for these mistakes.  In particular, according to the IRC
>> logs, Léo wrote:
>> 
>>    <lle-bout> raghavgururajan: the main issues on the rebasing were about
>>    security fixes on cairo, gdk-pixbuf and glib
>>    <lle-bout> I modified the cosmetic commits to remove the graft and
>>    patches etc.
>> 
>>    <https://logs.guix.gnu.org/guix/2021-03-26.log#000950>
> Yes, Leo made these changes. But I also have to acknowledge that it was 
> part of my responsibility to question any changes. My apologies for 
> that.

Thank you for this acknowledgment, Raghav.  Your recent responses in
this thread have been commendable.

     Regards,
       Mark

-- 
Support Richard Stallman against the vicious disinformation campaign
against him and the FSF.  See <https://stallmansupport.org> for more.

Re: A "cosmetic changes" commit that removes security fixes

[aviva]

On 4/24/21 4:09 AM, Mark H Weaver wrote:
>   Your recent responses in
> this thread have been commendable.



by who?

Re: A "cosmetic changes" commit that removes security fixes

[Leo Famulari]

On Thu, Apr 22, 2021 at 12:05:36AM -0400, Raghav Gururajan wrote:
> Okay, I was able to retrace. When Leo and I were working outside savannah,
> there was master --> core-updates merge. Leo made these changes when he
> committed to his repo
> (https://logs.guix.gnu.org/guix/2021-03-26.log#000811), from which I pulled
> then format-patched and sent it to guix-patches
> (https://issues.guix.gnu.org/42958#64). From guix-patches it was then pushed
> to core-updates (https://issues.guix.gnu.org/42958#67), from where I
> cherry-picked into wip-gnome.

Mark,

Do you know if the security fixes under discussion are necessary on
core-updates?

Raghav and Léo, is wip-gnome based on core-updates?

Re: A "cosmetic changes" commit that removes security fixes

[Mark H Weaver]

Hi Leo,

Leo Famulari <leo@famulari.name> writes:

> On Thu, Apr 22, 2021 at 12:05:36AM -0400, Raghav Gururajan wrote:
>> Okay, I was able to retrace. When Leo and I were working outside savannah,
>> there was master --> core-updates merge. Leo made these changes when he
>> committed to his repo
>> (https://logs.guix.gnu.org/guix/2021-03-26.log#000811), from which I pulled
>> then format-patched and sent it to guix-patches
>> (https://issues.guix.gnu.org/42958#64). From guix-patches it was then pushed
>> to core-updates (https://issues.guix.gnu.org/42958#67), from where I
>> cherry-picked into wip-gnome.
>
> Mark,
>
> Do you know if the security fixes under discussion are necessary on
> core-updates?

The 'cairo' fixes are certainly still needed, because there has been no
upstream stable release of 'cairo' since the version (1.16.0) on our
'master' branch.

宋文武 proposed a patch to re-apply the fixes on 'core-updates', here:

  https://lists.gnu.org/archive/html/guix-devel/2021-04/msg00361.html

A similar patch will be needed for 'wip-gnome' as well.

I'm not sure about the other packages off-hand, but both 'glib' and
'gdk-pixbuf' were ultimately updated to newer versions, so I guess it's
likely that they're okay (although I haven't verified this).

      Thanks,
        Mark

-- 
Support Richard Stallman against the vicious misinformation campaign
against him and the FSF.  See <https://stallmansupport.org> for more.

Re: A "cosmetic changes" commit that removes security fixes

[Raghav Gururajan]

[-- Attachment #1.1: Type: text/plain, Size: 174 bytes --]

Hi Leo!

> Raghav and Léo, is wip-gnome based on core-updates?

It was based on core-updates, but I recently re-created wip-gnome based 
on master.

Regards,
RG.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Mark H Weaver]

Hi Raghav,

Raghav Gururajan <rg@raghavgururajan.name> writes:

>> Those commits on 'core-updates' were digitally signed by Léo Le Bouter
>> <lle-bout@zaclys.net> and have the same problems: they remove security
>> fixes, and yet the summary lines indicate that only "cosmetic changes"
>> were made.
>
> Yeah, the commit title didn't mention the change but the commit message did.

I'm sorry, but that won't do.  There are at least three things wrong
with these commits:

(1) The summary lines were misleading, because they implied that no
    functional changes were made.

(2) The commit messages were misleading, because they failed to mention
    that security holes which had previously been fixed were now being
    re-introduced.  That wasn't at all obvious.

    Commits like these, which remove patches that had fixed security
    flaws, are fairly common: someone casually looking over the commit
    log might assume that the patches could be safely removed because a
    version update was done at the same time, rendering those patches
    obsolete.

(3) Although your 'glib' commit was immediately followed by a 'glib'
    update, rendering it harmless, your misleading 'cairo' commit left
    'cairo' vulnerable to CVE-2018-19876 and CVE-2020-35492 on our
    'core-updates' and 'wip-gnome' branches.  Those will need to be
    fixed now.

Léo Le Bouter <lle-bout@zaclys.net> is also culpable here, because he
digitally signed the misleading 'cairo' commit that's on our
'core-updates' branch, which re-introduced CVE-2018-19876 and
CVE-2020-35492.

--8<---------------cut here---------------start------------->8---
commit f94cdc86f644984ca83164d40b17e7eed6e22091
gpg: Signature made Fri 26 Mar 2021 05:13:57 PM EDT
gpg:                using RSA key 148BCB8BD80BFB16B1DE0E9145A8B1E86BCD10A6
gpg: Good signature from "Léo Le Bouter <lle-bout@zaclys.net>" [unknown]
gpg: WARNING: This key is not certified with a trusted signature!
gpg:          There is no indication that the signature belongs to the owner.
Primary key fingerprint: 148B CB8B D80B FB16 B1DE  0E91 45A8 B1E8 6BCD 10A6
Author: Raghav Gururajan <raghavgururajan@disroot.org>
Date:   Fri Dec 4 00:48:43 2020 -0500

    gnu: cairo: Make some cosmetic changes.
    
    * gnu/packages/patches/cairo-CVE-2018-19876.patch,
    gnu/packages/patches/cairo-CVE-2020-35492.patch: Remove patches.
    * gnu/local.mk (dist_patch_DATA): Unregister them.
    * gnu/packages/gtk.scm (cairo): Make some cosmetic changes.
    [replacement]: Remove.
    (cairo/fixed): Remove.
    
    Signed-off-by: Léo Le Bouter <lle-bout@zaclys.net>
--8<---------------cut here---------------end--------------->8---

https://git.sv.gnu.org/cgit/guix.git/commit/?h=core-updates&id=f94cdc86f644984ca83164d40b17e7eed6e22091

Even the most superficial skimming of this commit should have
immediately raised red flags, because the summary line is clearly
inaccurate.  It shows a lack of careful review, to put it mildly.

      Mark

Re: A "cosmetic changes" commit that removes security fixes

[宋文武]

[-- Attachment #1: Type: text/plain, Size: 1729 bytes --]

Mark H Weaver <mhw@netris.org> writes:

> Hi Raghav,
>
> Raghav Gururajan <rg@raghavgururajan.name> writes:
>
>>> Those commits on 'core-updates' were digitally signed by Léo Le Bouter
>>> <lle-bout@zaclys.net> and have the same problems: they remove security
>>> fixes, and yet the summary lines indicate that only "cosmetic changes"
>>> were made.
>>
>> Yeah, the commit title didn't mention the change but the commit message did.
>
> I'm sorry, but that won't do.  There are at least three things wrong
> with these commits:
>
> (1) The summary lines were misleading, because they implied that no
>     functional changes were made.

Yes, if the title can't summary the change, then the change should be
splited into multiple commits.
>
> (2) The commit messages were misleading, because they failed to mention
>     that security holes which had previously been fixed were now being
>     re-introduced.  That wasn't at all obvious.
>
>     Commits like these, which remove patches that had fixed security
>     flaws, are fairly common: someone casually looking over the commit
>     log might assume that the patches could be safely removed because a
>     version update was done at the same time, rendering those patches
>     obsolete.

Agree, I think we should mention explicitly that those patches are now
not needed after some code audit.
>
> (3) Although your 'glib' commit was immediately followed by a 'glib'
>     update, rendering it harmless, your misleading 'cairo' commit left
>     'cairo' vulnerable to CVE-2018-19876 and CVE-2020-35492 on our
>     'core-updates' and 'wip-gnome' branches.  Those will need to be
>     fixed now.

This patch is for core-updates:

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnu-cairo-Reintroduce-security-patches-security-fixe.patch --]
[-- Type: text/x-patch, Size: 5364 bytes --]

From 15e28e84eaea8f68b6247ab53052f0dd50a544b2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E5=AE=8B=E6=96=87=E6=AD=A6?= <iyzsong@member.fsf.org>
Date: Thu, 22 Apr 2021 19:21:51 +0800
Subject: [PATCH] gnu: cairo: Reintroduce security patches [security fixes].

Two patches were accidentally removed in commit
f94cdc86f644984ca83164d40b17e7eed6e22091.

* gnu/packages/patches/cairo-CVE-2018-19876.patch,
gnu/packages/patches/cairo-CVE-2020-35492.patch: New files.
* gnu/local.mk (dist_patch_DATA): Add them.
* gnu/packages/gtk.scm (cairo)[patches]: Apply them.
---
 gnu/local.mk                                  |  2 +
 gnu/packages/gtk.scm                          |  5 +-
 .../patches/cairo-CVE-2018-19876.patch        | 37 ++++++++++++++
 .../patches/cairo-CVE-2020-35492.patch        | 49 +++++++++++++++++++
 4 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100644 gnu/packages/patches/cairo-CVE-2018-19876.patch
 create mode 100644 gnu/packages/patches/cairo-CVE-2020-35492.patch

diff --git a/gnu/local.mk b/gnu/local.mk
index a8dd66f34a..39b2b72a42 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -880,6 +880,8 @@ dist_patch_DATA =						\
   %D%/packages/patches/bpftrace-disable-bfd-disasm.patch	\
   %D%/packages/patches/busybox-CVE-2021-28831.patch		\
   %D%/packages/patches/byobu-writable-status.patch		\
+  %D%/packages/patches/cairo-CVE-2018-19876.patch		\
+  %D%/packages/patches/cairo-CVE-2020-35492.patch		\
   %D%/packages/patches/calibre-no-updates-dialog.patch		\
   %D%/packages/patches/calibre-remove-test-sqlite.patch		\
   %D%/packages/patches/calibre-remove-test-unrar.patch		\
diff --git a/gnu/packages/gtk.scm b/gnu/packages/gtk.scm
index 9f3aea4aca..f70e667115 100644
--- a/gnu/packages/gtk.scm
+++ b/gnu/packages/gtk.scm
@@ -142,7 +142,10 @@ tools have full access to view and control running applications.")
         (string-append "https://cairographics.org/releases/cairo-"
                        version ".tar.xz"))
        (sha256
-        (base32 "0c930mk5xr2bshbdljv005j3j8zr47gqmkry3q6qgvqky6rjjysy"))))
+        (base32 "0c930mk5xr2bshbdljv005j3j8zr47gqmkry3q6qgvqky6rjjysy"))
+       (patches (search-patches
+		 "cairo-CVE-2018-19876.patch"
+		 "cairo-CVE-2020-35492.patch"))))
     (build-system glib-or-gtk-build-system)
     (outputs '("out" "doc"))
     (arguments
diff --git a/gnu/packages/patches/cairo-CVE-2018-19876.patch b/gnu/packages/patches/cairo-CVE-2018-19876.patch
new file mode 100644
index 0000000000..c0fba2ecaa
--- /dev/null
+++ b/gnu/packages/patches/cairo-CVE-2018-19876.patch
@@ -0,0 +1,37 @@
+Copied from Debian.
+
+From: Carlos Garcia Campos <cgarcia@igalia.com>
+Date: Mon, 19 Nov 2018 12:33:07 +0100
+Subject: ft: Use FT_Done_MM_Var instead of free when available in
+ cairo_ft_apply_variations
+
+Fixes a crash when using freetype >= 2.9
+
+[This is considered to be security-sensitive because WebKitGTK+ sets its
+own memory allocator, which is not compatible with system free(), making
+this a remotely triggerable denial of service or memory corruption.]
+
+Origin: upstream, commit:90e85c2493fdfa3551f202ff10282463f1e36645
+Bug: https://gitlab.freedesktop.org/cairo/cairo/merge_requests/5
+Bug-Debian: https://bugs.debian.org/916389
+Bug-CVE: CVE-2018-19876
+---
+ src/cairo-ft-font.c | 4 ++++
+ 1 file changed, 4 insertions(+)
+
+diff --git a/src/cairo-ft-font.c b/src/cairo-ft-font.c
+index 325dd61..981973f 100644
+--- a/src/cairo-ft-font.c
++++ b/src/cairo-ft-font.c
+@@ -2393,7 +2393,11 @@ skip:
+ done:
+         free (coords);
+         free (current_coords);
++#if HAVE_FT_DONE_MM_VAR
++        FT_Done_MM_Var (face->glyph->library, ft_mm_var);
++#else
+         free (ft_mm_var);
++#endif
+     }
+ }
+ 
diff --git a/gnu/packages/patches/cairo-CVE-2020-35492.patch b/gnu/packages/patches/cairo-CVE-2020-35492.patch
new file mode 100644
index 0000000000..e8b90fa5c5
--- /dev/null
+++ b/gnu/packages/patches/cairo-CVE-2020-35492.patch
@@ -0,0 +1,49 @@
+Copied from Debian.
+
+From 03a820b173ed1fdef6ff14b4468f5dbc02ff59be Mon Sep 17 00:00:00 2001
+From: Heiko Lewin <heiko.lewin@worldiety.de>
+Date: Tue, 15 Dec 2020 16:48:19 +0100
+Subject: [PATCH] Fix mask usage in image-compositor
+
+[trimmed test case, since not used in Debian build]
+
+---
+ src/cairo-image-compositor.c                |   8 ++--
+
+--- cairo-1.16.0.orig/src/cairo-image-compositor.c
++++ cairo-1.16.0/src/cairo-image-compositor.c
+@@ -2601,14 +2601,14 @@ _inplace_src_spans (void *abstract_rende
+ 		    unsigned num_spans)
+ {
+     cairo_image_span_renderer_t *r = abstract_renderer;
+-    uint8_t *m;
++    uint8_t *m, *base = (uint8_t*)pixman_image_get_data(r->mask);
+     int x0;
+ 
+     if (num_spans == 0)
+ 	return CAIRO_STATUS_SUCCESS;
+ 
+     x0 = spans[0].x;
+-    m = r->_buf;
++    m = base;
+     do {
+ 	int len = spans[1].x - spans[0].x;
+ 	if (len >= r->u.composite.run_length && spans[0].coverage == 0xff) {
+@@ -2646,7 +2646,7 @@ _inplace_src_spans (void *abstract_rende
+ 				      spans[0].x, y,
+ 				      spans[1].x - spans[0].x, h);
+ 
+-	    m = r->_buf;
++	    m = base;
+ 	    x0 = spans[1].x;
+ 	} else if (spans[0].coverage == 0x0) {
+ 	    if (spans[0].x != x0) {
+@@ -2675,7 +2675,7 @@ _inplace_src_spans (void *abstract_rende
+ #endif
+ 	    }
+ 
+-	    m = r->_buf;
++	    m = base;
+ 	    x0 = spans[1].x;
+ 	} else {
+ 	    *m++ = spans[0].coverage;
-- 
2.30.0


[-- Attachment #3: Type: text/plain, Size: 41 bytes --]



We should be more careful next time...

Re: A "cosmetic changes" commit that removes security fixes

[Mark H Weaver]

Hi,

宋文武 <iyzsong@outlook.com> writes:

> This patch is for core-updates:
> From 15e28e84eaea8f68b6247ab53052f0dd50a544b2 Mon Sep 17 00:00:00 2001
> From: 宋文武 <iyzsong@member.fsf.org>
> Date: Thu, 22 Apr 2021 19:21:51 +0800
> Subject: [PATCH] gnu: cairo: Reintroduce security patches [security fixes].
>
> Two patches were accidentally removed in commit
> f94cdc86f644984ca83164d40b17e7eed6e22091.
>
> * gnu/packages/patches/cairo-CVE-2018-19876.patch,
> gnu/packages/patches/cairo-CVE-2020-35492.patch: New files.
> * gnu/local.mk (dist_patch_DATA): Add them.
> * gnu/packages/gtk.scm (cairo)[patches]: Apply them.

Looks good to me.  Please push!

    Thanks,
      Mark

Re: A "cosmetic changes" commit that removes security fixes

[Léo Le Bouter]

[-- Attachment #1: Type: text/plain, Size: 3662 bytes --]

On Thu, 2021-04-22 at 00:08 -0400, Mark H Weaver wrote:
> Hi Raghav,
> 
> Raghav Gururajan <rg@raghavgururajan.name> writes:
> 
> > > Those commits on 'core-updates' were digitally signed by Léo Le
> > > Bouter
> > > <lle-bout@zaclys.net> and have the same problems: they remove
> > > security
> > > fixes, and yet the summary lines indicate that only "cosmetic
> > > changes"
> > > were made.
> > 
> > Yeah, the commit title didn't mention the change but the commit
> > message did.
> 
> I'm sorry, but that won't do.  There are at least three things wrong
> with these commits:
> 
> (1) The summary lines were misleading, because they implied that no
>     functional changes were made.
> 
> (2) The commit messages were misleading, because they failed to
> mention
>     that security holes which had previously been fixed were now
> being
>     re-introduced.  That wasn't at all obvious.
> 
>     Commits like these, which remove patches that had fixed security
>     flaws, are fairly common: someone casually looking over the
> commit
>     log might assume that the patches could be safely removed because
> a
>     version update was done at the same time, rendering those patches
>     obsolete.
> 
> (3) Although your 'glib' commit was immediately followed by a 'glib'
>     update, rendering it harmless, your misleading 'cairo' commit
> left
>     'cairo' vulnerable to CVE-2018-19876 and CVE-2020-35492 on our
>     'core-updates' and 'wip-gnome' branches.  Those will need to be
>     fixed now.
> 
> Léo Le Bouter <lle-bout@zaclys.net> is also culpable here, because he
> digitally signed the misleading 'cairo' commit that's on our
> 'core-updates' branch, which re-introduced CVE-2018-19876 and
> CVE-2020-35492.
> 
> --8<---------------cut here---------------start------------->8---
> commit f94cdc86f644984ca83164d40b17e7eed6e22091
> gpg: Signature made Fri 26 Mar 2021 05:13:57 PM EDT
> gpg:                using RSA key
> 148BCB8BD80BFB16B1DE0E9145A8B1E86BCD10A6
> gpg: Good signature from "Léo Le Bouter <lle-bout@zaclys.net>"
> [unknown]
> gpg: WARNING: This key is not certified with a trusted signature!
> gpg:          There is no indication that the signature belongs to
> the owner.
> Primary key fingerprint: 148B CB8B D80B FB16 B1DE  0E91 45A8 B1E8
> 6BCD 10A6
> Author: Raghav Gururajan <raghavgururajan@disroot.org>
> Date:   Fri Dec 4 00:48:43 2020 -0500
> 
>     gnu: cairo: Make some cosmetic changes.
>     
>     * gnu/packages/patches/cairo-CVE-2018-19876.patch,
>     gnu/packages/patches/cairo-CVE-2020-35492.patch: Remove patches.
>     * gnu/local.mk (dist_patch_DATA): Unregister them.
>     * gnu/packages/gtk.scm (cairo): Make some cosmetic changes.
>     [replacement]: Remove.
>     (cairo/fixed): Remove.
>     
>     Signed-off-by: Léo Le Bouter <lle-bout@zaclys.net>
> --8<---------------cut here---------------end--------------->8---
> 
> https://git.sv.gnu.org/cgit/guix.git/commit/?h=core-updates&id=f94cdc86f644984ca83164d40b17e7eed6e22091
> 
> Even the most superficial skimming of this commit should have
> immediately raised red flags, because the summary line is clearly
> inaccurate.  It shows a lack of careful review, to put it mildly.
> 
>       Mark

Hello Mark,

I don't share your analysis, the security fixes werent stripped because
glib/cairo was also updated to latest version in subsequent commits
which were pushed all at once.

Careful review was done, and that's why I signed-off and GPG-signed the
commits. Nobody was put at risk by these commits and no security fixes
were stripped.

Léo

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 4270 bytes --]


Léo Le Bouter <lle-bout@zaclys.net> writes:

> On Thu, 2021-04-22 at 00:08 -0400, Mark H Weaver wrote:
>> Hi Raghav,
>> 
>> Raghav Gururajan <rg@raghavgururajan.name> writes:
>> 
>> > > Those commits on 'core-updates' were digitally signed by Léo Le
>> > > Bouter
>> > > <lle-bout@zaclys.net> and have the same problems: they remove
>> > > security
>> > > fixes, and yet the summary lines indicate that only "cosmetic
>> > > changes"
>> > > were made.
>> > 
>> > Yeah, the commit title didn't mention the change but the commit
>> > message did.
>> 
>> I'm sorry, but that won't do.  There are at least three things wrong
>> with these commits:
>> 
>> (1) The summary lines were misleading, because they implied that no
>>     functional changes were made.
>> 
>> (2) The commit messages were misleading, because they failed to
>> mention
>>     that security holes which had previously been fixed were now
>> being
>>     re-introduced.  That wasn't at all obvious.
>> 
>>     Commits like these, which remove patches that had fixed security
>>     flaws, are fairly common: someone casually looking over the
>> commit
>>     log might assume that the patches could be safely removed because
>> a
>>     version update was done at the same time, rendering those patches
>>     obsolete.
>> 
>> (3) Although your 'glib' commit was immediately followed by a 'glib'
>>     update, rendering it harmless, your misleading 'cairo' commit
>> left
>>     'cairo' vulnerable to CVE-2018-19876 and CVE-2020-35492 on our
>>     'core-updates' and 'wip-gnome' branches.  Those will need to be
>>     fixed now.
>> 
>> Léo Le Bouter <lle-bout@zaclys.net> is also culpable here, because he
>> digitally signed the misleading 'cairo' commit that's on our
>> 'core-updates' branch, which re-introduced CVE-2018-19876 and
>> CVE-2020-35492.
>> 
>> --8<---------------cut here---------------start------------->8---
>> commit f94cdc86f644984ca83164d40b17e7eed6e22091
>> gpg: Signature made Fri 26 Mar 2021 05:13:57 PM EDT
>> gpg:                using RSA key
>> 148BCB8BD80BFB16B1DE0E9145A8B1E86BCD10A6
>> gpg: Good signature from "Léo Le Bouter <lle-bout@zaclys.net>"
>> [unknown]
>> gpg: WARNING: This key is not certified with a trusted signature!
>> gpg:          There is no indication that the signature belongs to
>> the owner.
>> Primary key fingerprint: 148B CB8B D80B FB16 B1DE  0E91 45A8 B1E8
>> 6BCD 10A6
>> Author: Raghav Gururajan <raghavgururajan@disroot.org>
>> Date:   Fri Dec 4 00:48:43 2020 -0500
>> 
>>     gnu: cairo: Make some cosmetic changes.
>>     
>>     * gnu/packages/patches/cairo-CVE-2018-19876.patch,
>>     gnu/packages/patches/cairo-CVE-2020-35492.patch: Remove patches.
>>     * gnu/local.mk (dist_patch_DATA): Unregister them.
>>     * gnu/packages/gtk.scm (cairo): Make some cosmetic changes.
>>     [replacement]: Remove.
>>     (cairo/fixed): Remove.
>>     
>>     Signed-off-by: Léo Le Bouter <lle-bout@zaclys.net>
>> --8<---------------cut here---------------end--------------->8---
>> 
>> https://git.sv.gnu.org/cgit/guix.git/commit/?h=core-updates&id=f94cdc86f644984ca83164d40b17e7eed6e22091
>> 
>> Even the most superficial skimming of this commit should have
>> immediately raised red flags, because the summary line is clearly
>> inaccurate.  It shows a lack of careful review, to put it mildly.
>> 
>>       Mark
>
> Hello Mark,
>
> I don't share your analysis, the security fixes werent stripped because
> glib/cairo was also updated to latest version in subsequent commits
> which were pushed all at once.
>
> Careful review was done, and that's why I signed-off and GPG-signed the
> commits. Nobody was put at risk by these commits and no security fixes
> were stripped.

I think the guidance is that commits should include one set of related
changes, so if the patches/replacement can be removed because the
package is being updated, those related changes should be in the same
commit. If there are other unrelated changes, they can go in other
commits.

Especially if the commits are being pushed at the same time, it's worth
making sure this happens, so that it's easier to review and look at the
changes in a sensible way.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Leo Prikler]

Am Donnerstag, den 22.04.2021, 22:01 +0200 schrieb Léo Le Bouter:
> On Thu, 2021-04-22 at 00:08 -0400, Mark H Weaver wrote:
> > Hi Raghav,
> > 
> > Raghav Gururajan <rg@raghavgururajan.name> writes:
> > 
> > > > Those commits on 'core-updates' were digitally signed by Léo Le
> > > > Bouter
> > > > <lle-bout@zaclys.net> and have the same problems: they remove
> > > > security
> > > > fixes, and yet the summary lines indicate that only "cosmetic
> > > > changes"
> > > > were made.
> > > 
> > > Yeah, the commit title didn't mention the change but the commit
> > > message did.
> > 
> > I'm sorry, but that won't do.  There are at least three things
> > wrong
> > with these commits:
> > 
> > (1) The summary lines were misleading, because they implied that no
> >     functional changes were made.
> > 
> > (2) The commit messages were misleading, because they failed to
> > mention
> >     that security holes which had previously been fixed were now
> > being
> >     re-introduced.  That wasn't at all obvious.
> > 
> >     Commits like these, which remove patches that had fixed
> > security
> >     flaws, are fairly common: someone casually looking over the
> > commit
> >     log might assume that the patches could be safely removed
> > because
> > a
> >     version update was done at the same time, rendering those
> > patches
> >     obsolete.
> > 
> > (3) Although your 'glib' commit was immediately followed by a
> > 'glib'
> >     update, rendering it harmless, your misleading 'cairo' commit
> > left
> >     'cairo' vulnerable to CVE-2018-19876 and CVE-2020-35492 on our
> >     'core-updates' and 'wip-gnome' branches.  Those will need to be
> >     fixed now.
> > 
> > Léo Le Bouter <lle-bout@zaclys.net> is also culpable here, because
> > he
> > digitally signed the misleading 'cairo' commit that's on our
> > 'core-updates' branch, which re-introduced CVE-2018-19876 and
> > CVE-2020-35492.
> > 
> > --8<---------------cut here---------------start------------->8---
> > commit f94cdc86f644984ca83164d40b17e7eed6e22091
> > gpg: Signature made Fri 26 Mar 2021 05:13:57 PM EDT
> > gpg:                using RSA key
> > 148BCB8BD80BFB16B1DE0E9145A8B1E86BCD10A6
> > gpg: Good signature from "Léo Le Bouter <lle-bout@zaclys.net>"
> > [unknown]
> > gpg: WARNING: This key is not certified with a trusted signature!
> > gpg:          There is no indication that the signature belongs to
> > the owner.
> > Primary key fingerprint: 148B CB8B D80B FB16 B1DE  0E91 45A8 B1E8
> > 6BCD 10A6
> > Author: Raghav Gururajan <raghavgururajan@disroot.org>
> > Date:   Fri Dec 4 00:48:43 2020 -0500
> > 
> >     gnu: cairo: Make some cosmetic changes.
> >     
> >     * gnu/packages/patches/cairo-CVE-2018-19876.patch,
> >     gnu/packages/patches/cairo-CVE-2020-35492.patch: Remove
> > patches.
> >     * gnu/local.mk (dist_patch_DATA): Unregister them.
> >     * gnu/packages/gtk.scm (cairo): Make some cosmetic changes.
> >     [replacement]: Remove.
> >     (cairo/fixed): Remove.
> >     
> >     Signed-off-by: Léo Le Bouter <lle-bout@zaclys.net>
> > --8<---------------cut here---------------end--------------->8---
> > 
> > https://git.sv.gnu.org/cgit/guix.git/commit/?h=core-updates&id=f94cdc86f644984ca83164d40b17e7eed6e22091
> > 
> > Even the most superficial skimming of this commit should have
> > immediately raised red flags, because the summary line is clearly
> > inaccurate.  It shows a lack of careful review, to put it mildly.
> > 
> >       Mark
> 
> Hello Mark,
> 
> I don't share your analysis, the security fixes werent stripped
> because
> glib/cairo was also updated to latest version in subsequent commits
> which were pushed all at once.
This may be the case for glib, but which commit pushes cairo to a
version, in which those security fixes are applied?

Re: A "cosmetic changes" commit that removes security fixes

[Mark H Weaver]

Hi Léo,

Léo Le Bouter <lle-bout@zaclys.net> writes:

> I don't share your analysis, the security fixes werent stripped because
> glib/cairo was also updated to latest version in subsequent commits
> which were pushed all at once.

'glib' was updated, but 'cairo' wasn't, presumably because there's no
newer stable release of 'cairo' to update to.

> Careful review was done, and that's why I signed-off and GPG-signed the
> commits. Nobody was put at risk by these commits and no security fixes
> were stripped.

Those are bold claims, given the contents of our git repository.

Here's Raghav's commit on the 'core-updates' branch, which bears your
digital signature (according to my 'git' client), where the security
fixes for CVE-2018-19876 and CVE-2020-35492 were removed, in a commit
whose summary line is "gnu: cairo: Make some cosmetic changes":

  https://git.sv.gnu.org/cgit/guix.git/commit/?h=core-updates&id=f94cdc86f644984ca83164d40b17e7eed6e22091

I have two questions for you:

(1) Do you deny that you digitally signed that commit?
(2) Do you deny that there's anything wrong with that commit?

     Thanks,
       Mark

-- 
Support Richard Stallman against the vicious misinformation campaign
against him and the FSF.  See <https://stallmansupport.org> for more.

Re: A "cosmetic changes" commit that removes security fixes

[Maxim Cournoyer]

Hi,

Mark H Weaver <mhw@netris.org> writes:

> Hi Léo,
>
> Léo Le Bouter <lle-bout@zaclys.net> writes:
>
>> I don't share your analysis, the security fixes werent stripped because
>> glib/cairo was also updated to latest version in subsequent commits
>> which were pushed all at once.
>
> 'glib' was updated, but 'cairo' wasn't, presumably because there's no
> newer stable release of 'cairo' to update to.

Actually, there *is* a "new" stable release available on their release
page, 1.17.2 [0]

According to NVD [1], that latest version has no known CVE [1].

Léo, could it be that you had planned to do this update, but it somehow
fell into the cracks?  In any case I agree with the others that it'd
have been better to ungraft/remove patches in the same commit that
updates the software to a version that incorporates the fixes, as I'm
sure you already know: it'd have prevented this kind of situation.

I also urge you to remain calm and collaborative even in the face of
criticism; as Ricardo said, escalating things will lead us nowhere good.
Honest mistakes are made and that's no problem so long as we stand ready
to apologize for them and work together for a resolution.

I see that 宋文武 has pushed a commit
(2ab4f4c950ffa7ca40271a534cb3bed997672138) to core-updates reinstating
the security patches; thanks!

Thank you,

Maxim

[0]  https://www.cairographics.org/releases/
[1]  https://nvd.nist.gov/vuln/search/results?form_type=Advanced&results_type=overview&seach_type=all&query=cpe:2.3:a:cairographics:cairo:-:*:*:*:*:*:*:*

Re: A "cosmetic changes" commit that removes security fixes

[Raghav Gururajan]

[-- Attachment #1.1: Type: text/plain, Size: 334 bytes --]

Hi Maxim!

> Actually, there *is* a "new" stable release available on their release
> page, 1.17.2

It seems 1.16.0 is the latest+stable version.

Quoting their download, "Please download one of the latest 
[releases](https://cairographics.org/releases/) in order to get an 
API-stable version of cairo."

Regards,
RG.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Maxim Cournoyer]

Hello Raghav,

Raghav Gururajan <rg@raghavgururajan.name> writes:

> Hi Maxim!
>
>> Actually, there *is* a "new" stable release available on their release
>> page, 1.17.2
>
> It seems 1.16.0 is the latest+stable version.
>
> Quoting their download, "Please download one of the latest
> [releases](https://cairographics.org/releases/) in order to get an 
> API-stable version of cairo."

Oh, indeed, sorry for the confusion.  I think I got tricked by seeing
the changelog for 1.17.2 under their releases/ directory
(https://www.cairographics.org/releases/ChangeLog.cairo-1.17.2).

Thank you,

Maxim

Re: A "cosmetic changes" commit that removes security fixes

[Raghav Gururajan]

[-- Attachment #1.1: Type: text/plain, Size: 316 bytes --]

Hi Maxim!

> Oh, indeed, sorry for the confusion.  I think I got tricked by seeing
> the changelog for 1.17.2 under their releases/ directory
> (https://www.cairographics.org/releases/ChangeLog.cairo-1.17.2).

No worries! I was confused by that too, while I was working on cairo 
package.

Regards,
RG.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Léo Le Bouter]

[-- Attachment #1: Type: text/plain, Size: 2425 bytes --]

On Fri, 2021-04-23 at 13:52 -0400, Maxim Cournoyer wrote:
> Actually, there *is* a "new" stable release available on their
> release
> page, 1.17.2 [0]
> 
> According to NVD [1], that latest version has no known CVE [1].
> 
> Léo, could it be that you had planned to do this update, but it
> somehow
> fell into the cracks?  In any case I agree with the others that it'd
> have been better to ungraft/remove patches in the same commit that
> updates the software to a version that incorporates the fixes, as I'm
> sure you already know: it'd have prevented this kind of situation.

Considering the GNOME upgrade is not finished yet, this is indeed
ongoing work. I would've never done this on master.

> 
> I also urge you to remain calm and collaborative even in the face of
> criticism; as Ricardo said, escalating things will lead us nowhere
> good.
> Honest mistakes are made and that's no problem so long as we stand
> ready
> to apologize for them and work together for a resolution.
> 

I think there is no problem in accepting criticism but there is a
certain way Mark presents criticism and I don't feel like I can respond
to it when it is written in such way. Over several emails Mark was
looking to point to people who were somehow responsible for whatever
"damage" for changes that happened on a branch nobody uses and always
contains ongoing work (core-updates), so maintaining it security-wise
is not as much of a question. The result is that we have a long thread
of people responding etc. causing a fuss over something that just needs
to be fixed rather than find whoever is somehow "responsible". I feel
like we're collectively responsible. We try our best at all times,
during this GNOME upgrade I also tried to take into account Raghav's
feelings so they do not give up and have a rewarding review experience,
I knew these commits werent great, I have written about it here: <
https://issues.guix.gnu.org/42958#67>.

> I see that 宋文武 has pushed a commit
> (2ab4f4c950ffa7ca40271a534cb3bed997672138) to core-updates
> reinstating
> the security patches; thanks!
> 

Great! Thanks for figuring this out.

> Thank you,
> 
> Maxim
> 
> [0]  https://www.cairographics.org/releases/
> [1]  
> https://nvd.nist.gov/vuln/search/results?form_type=Advanced&results_type=overview&seach_type=all&query=cpe:2.3:a:cairographics:cairo:-:*:*:*:*:*:*:*

Léo

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Leo Prikler]

Hi,

Am Freitag, den 23.04.2021, 20:50 +0200 schrieb Léo Le Bouter:
> I think there is no problem in accepting criticism but there is a
> certain way Mark presents criticism and I don't feel like I can
> respond
> to it when it is written in such way. Over several emails Mark was
> looking to point to people who were somehow responsible for whatever
> "damage" for changes that happened on a branch nobody uses and always
> contains ongoing work (core-updates), so maintaining it security-wise
> is not as much of a question. The result is that we have a long
> thread
> of people responding etc. causing a fuss over something that just
> needs
> to be fixed rather than find whoever is somehow "responsible". 
I disagree with the sentiment, that core-updates is fair game for any
kind of commit.  Now, naturally, since they cause many rebuilds it may
be harder to verify that upgrading some packages does not lead to
failure in another (especially without the CI), contributing to the
"work in progress" nature of core-updates, but this still doesn't
excuse removing security fixes.  
We all expect, that at some point we can merge core-updates "as is"
into master and commits like that call this assumption into question,
instead demanding a full review of a branch, whose patches should
already have been reviewed by the time they land.

> I feel
> like we're collectively responsible. We try our best at all times,
> during this GNOME upgrade I also tried to take into account Raghav's
> feelings so they do not give up and have a rewarding review
> experience,
> I knew these commits werent great, I have written about it here: <
> https://issues.guix.gnu.org/42958#67>;.
I think a more rewarding experience would have been to help them arrive
at a point, where such changes are no longer needed for the rest of
their patch set.  Not only would this have solved their immediate
issue, it would also have been a good learning experience and we
wouldn't need to discuss this at lengths several months later.

I have worked with Raghav before on telegram-desktop (and other
packages as well) and they were pretty patient with about 20 versions
being sent back and forth between us until we arrived at a set of
descriptions, that we could safely push.  Not nearly as many versions
would need to be sent in the case of a "cosmetic changes" patch, when I
ported their GStreamer updates to staging, I noticed that it was mostly
the indentation, that would screw things up for future patches.  I
admit, sometimes Raghav appears to "just want to get the job done
quickly", but giving in to such urges helps no one.

Regards,
Leo

Re: A "cosmetic changes" commit that removes security fixes

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 2054 bytes --]

On Fri, Apr 23, 2021 at 08:50:37PM +0200, Léo Le Bouter wrote:
> I think there is no problem in accepting criticism but there is a
> certain way Mark presents criticism and I don't feel like I can respond
> to it when it is written in such way. Over several emails Mark was
> looking to point to people who were somehow responsible for whatever
> "damage" for changes that happened on a branch nobody uses and always
> contains ongoing work (core-updates), so maintaining it security-wise
> is not as much of a question. The result is that we have a long thread
> of people responding etc. causing a fuss over something that just needs
> to be fixed rather than find whoever is somehow "responsible". I feel
> like we're collectively responsible. We try our best at all times,
> during this GNOME upgrade I also tried to take into account Raghav's
> feelings so they do not give up and have a rewarding review experience,
> I knew these commits werent great, I have written about it here: <
> https://issues.guix.gnu.org/42958#67>.

I have to agree with everybody in this thead.

The commits in question were problematic (especially on core-updates,
which is not a "WIP" branch and thus cannot be rewritten to fix past
problems). I'm not confident that the security fixes would have been
reinstated on core-updates if Mark had not asked about them.

Léo and Raghav, you need to keep learning our workflow around security
updates.  It's not okay to remove security patches and later update a
package to a fixed version in a different commit. `git rebase` is the
tool to learn for cases like this one.

However, Mark, you have way more experience, and you need to handle
these things differently. If you don't trust certain Guix contributors,
take it up with the maintainers — in private. The style of communication
you used here is ineffective and will dissuade people from contributing
to Guix. Do you want Léo and Raghav to learn and improve? Or do you want
them to leave? Remember that we all begin as beginners.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Léo Le Bouter]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

On Fri, 2021-04-23 at 15:18 -0400, Leo Famulari wrote:
> Léo and Raghav, you need to keep learning our workflow around
> security
> updates.  It's not okay to remove security patches and later update a
> package to a fixed version in a different commit. `git rebase` is the
> tool to learn for cases like this one.

I knew about this but I didnt feel like telling Raghav to do yet
another rebase. I felt like Raghav was taking on with so much already.
The rebase was specially complicated because Raghav's commit changed
indentation, git has bad quite bad UX for cases like these. At the time
I had lots of things to handle also and couldnt spend lots of time on
it myself. I didnt feel like blocking the merge of these patches for
commit history was worth it at all. Such blocking could have hindered
the GNOME upgrade effort even more. Thankfully now there's lots of
energy being put to it, at the time there wasnt anyone else than Raghav
and me.

Léo

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 1950 bytes --]

On Fri, Apr 23, 2021 at 09:33:07PM +0200, Léo Le Bouter wrote:
> I knew about this but I didnt feel like telling Raghav to do yet
> another rebase. I felt like Raghav was taking on with so much already.
> The rebase was specially complicated because Raghav's commit changed
> indentation, git has bad quite bad UX for cases like these. At the time
> I had lots of things to handle also and couldnt spend lots of time on
> it myself. I didnt feel like blocking the merge of these patches for
> commit history was worth it at all. Such blocking could have hindered
> the GNOME upgrade effort even more. Thankfully now there's lots of
> energy being put to it, at the time there wasnt anyone else than Raghav
> and me.

I'm sympathetic. There is an imbalance between the work that we want to
complete, and the time and energy that we can give to it.

And in the case of GNOME, we have already fallen short of our goals
several times, having missed multiple upgrades.

I too have felt the temptation to cut corners with Git when I know that
the final result will be "okay". But Guix is not just about the final
product (a release, or a merge). We also have the --commit option to
Guix commands, and `guix time-machine`. So the Git history is important
too.

And I have also spent several hours at a time, focused on completing
(after several restarts) a complicated rebase involving dozens of
commits. And I've done that many times.

I do think that Mark is being hyperbolic about the wip-gnome branch. The
name says "work in progress" and we don't hold those branches to a high
standard. But what happened on core-updates *must not happen again*.

For a task as large as "updating GNOME in Guix", history tells me that
it has to be a group effort. In many cases, the hardest part of a
project is coordination and leadership, not coding. I hope that this
current effort continues, and that more people decide to join.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Giovanni Biscuolo]

[-- Attachment #1: Type: text/plain, Size: 2813 bytes --]

Hello Guix,

Leo Famulari <leo@famulari.name> writes:

[...]

> And in the case of GNOME, we have already fallen short of our goals
> several times, having missed multiple upgrades.

I regret not to be able to contribute more to Guix, but please nobody
should feel guilty not to be able to keep-up with upstream's upgrading
rate (whatever rate it is), better safe than up-to-date :-)

> I too have felt the temptation to cut corners with Git when I know that
> the final result will be "okay". But Guix is not just about the final
> product (a release, or a merge). We also have the --commit option to
> Guix commands, and `guix time-machine`. So the Git history is important
> too.

Yes, please this should be stressed: Guix *is* it's official (master,
core-updates...) git repo branches.

Just to understand: /if/ at any point in time a user is able to afford
the effort to build the entire core-updates /or/ staging branch she
should be confident the result is state-of-the-art secure.  Am I wrong
with this assumption?

> And I have also spent several hours at a time, focused on completing
> (after several restarts) a complicated rebase involving dozens of
> commits. And I've done that many times.

I think this is the most expensive activity of Guix maintainers, for the
very reason that Guix *is* git

> I do think that Mark is being hyperbolic about the wip-gnome branch. The
> name says "work in progress" and we don't hold those branches to a high
> standard.

I understand your point but please consider that /unless/ a wip-branch
is private (or privately shared out-of-Guix-git) that branch it's a
pubblic collective work in progress and sometimes (seldom? often? I
really don't know) that work could be completed by someone else, so even
in wip- branches committers should exercise some degree of discipline,
especially when dealing with "commit message completeness" and more with
security related patches.  In other words, IMHO a certain degree of
safety must be assured also on wip- branches.

Probably the policy about wip-branches, whatever it is ("do what you
want" or something in line with my comments above), should be documented
in the contributing section of the Guix manual.

> But what happened on core-updates *must not happen again*.

Please no.

> For a task as large as "updating GNOME in Guix", history tells me that
> it has to be a group effort. In many cases, the hardest part of a
> project is coordination and leadership, not coding. I hope that this
> current effort continues, and that more people decide to join.

OK but please consider that /if/ Guix cannot "update GNOME in Guix" for
whatever reason, GNOME should not be updated.


Thanks! Giovanni.

-- 
Giovanni Biscuolo

Xelera IT Infrastructures

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 849 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 2888 bytes --]

On Mon, Apr 26, 2021 at 07:06:33PM +0200, Giovanni Biscuolo wrote:
> Just to understand: /if/ at any point in time a user is able to afford
> the effort to build the entire core-updates /or/ staging branch she
> should be confident the result is state-of-the-art secure.  Am I wrong
> with this assumption?

Unfortunately your assumption is incorrect.

We do not apply security updates to the core-updates branch, except what
comes via `git merge master`, which only happens in the final stages of
the cycle.

Core-updates is not expected to be "buildable", let alone "secure",
until the end of the core-updates cycle when we start to whip it into
shape.

That branch is just a place to push updates of core packages, so that we
don't duplicate effort or lose track of updates.

Nevertheless, we should never remove security patches without a
corresponding package update, done in a single atomic commit. That's not
how we work.

If there is some documentation or messaging that suggests that anyone
should ever use the core-updates branch, please let us know and we will
fix that. The only branch you should use is the master branch, unless
you are testing something as a developer
  
> Leo Famulari <leo@famulari.name> writes:
> > I do think that Mark is being hyperbolic about the wip-gnome branch. The
> > name says "work in progress" and we don't hold those branches to a high
> > standard.
> 
> I understand your point but please consider that /unless/ a wip-branch
> is private (or privately shared out-of-Guix-git) that branch it's a
> pubblic collective work in progress and sometimes (seldom? often? I
> really don't know) that work could be completed by someone else, so even
> in wip- branches committers should exercise some degree of discipline,
> especially when dealing with "commit message completeness" and more with
> security related patches.  In other words, IMHO a certain degree of
> safety must be assured also on wip- branches.
> 
> Probably the policy about wip-branches, whatever it is ("do what you
> want" or something in line with my comments above), should be documented
> in the contributing section of the Guix manual.

I did not mean to suggestthat wip-* branches should not be secure but,
again, they are only works in progress. They do not even have a stable
Git history, due to rebasing, which breaks the Guix code authentication
mechanism. So, if you try to use them, you will have to use `guix pull
--allow-downgrades` and then all bets are off in terms of security.

These branches are merely a way for developers to share their work with
each other.

> OK but please consider that /if/ Guix cannot "update GNOME in Guix" for
> whatever reason, GNOME should not be updated.

I don't understand this. It seems tautological that if we cannot update
GNOME, then GNOME should not be updated.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Giovanni Biscuolo]

[-- Attachment #1: Type: text/plain, Size: 720 bytes --]

Leo Famulari <leo@famulari.name> writes:

> On Mon, Apr 26, 2021 at 07:06:33PM +0200, Giovanni Biscuolo wrote:
>> Just to understand: /if/ at any point in time a user is able to afford
>> the effort to build the entire core-updates /or/ staging branch she
>> should be confident the result is state-of-the-art secure.  Am I wrong
>> with this assumption?
>
> Unfortunately your assumption is incorrect.

[...]

> If there is some documentation or messaging that suggests that anyone
> should ever use the core-updates branch, please let us know and we will
> fix that.

No, I simply misunderstood, sorry for the noise!

[...]

Thanks, Giovanni

-- 
Giovanni Biscuolo

Xelera IT Infrastructures

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 849 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Leo Famulari]

On Mon, Apr 26, 2021 at 11:56:22PM +0200, Giovanni Biscuolo wrote:
> No, I simply misunderstood, sorry for the noise!

Okay, and thanks for asking! It's important to clarify these things;
it's not just noise :)

This kind of knowledge is something I picked up over time, but I'm not
sure it's written down anywhere. So I added a line to the manual that I
hope can make it a little more clear to anyone looking for information:

https://git.savannah.gnu.org/cgit/guix.git/commit/?id=4de688738ce802056dadd6f785c7bdb3407dc768

Re: A "cosmetic changes" commit that removes security fixes

[Mark H Weaver]

Hi Léo,

Léo Le Bouter <lle-bout@zaclys.net> writes:

> On Fri, 2021-04-23 at 15:18 -0400, Leo Famulari wrote:
>> Léo and Raghav, you need to keep learning our workflow around
>> security updates.  It's not okay to remove security patches and later
>> update a package to a fixed version in a different commit. `git
>> rebase` is the tool to learn for cases like this one.
>
> I knew about this but I didnt feel like telling Raghav to do yet
> another rebase. I felt like Raghav was taking on with so much already.

This response is what native English speakers call a "red herring"
<https://en.wikipedia.org/wiki/Red_herring>: something that misleads or
distracts from a relevant or important question.

Why do I say that?  As I pointed out elsewhere in this thread,

  https://lists.gnu.org/archive/html/guix-devel/2021-04/msg00366.html

it appears that Raghav didn't actually remove the security fixes; it
appears that *you* did.  Therefore, I fail to see how it could have been
avoided by asking Raghav to do another rebase.

From the IRC logs cited in the message above, it appears that you took
the liberty of modifying Raghav's "cosmetic changes" commits to also
remove security fixes in the re-indented packages, while claiming in the
summary lines that you were "ungraft[ing]" cairo and glib.

  https://git.sr.ht/~lle-bout/guix/commit/a045a48dd961f0c5c3d536dcc3fd21d9c08d2d50
  https://git.sr.ht/~lle-bout/guix/commit/6477daa338fbf1c9edacfc3690aca77cacfe0008

Can you please explain what went wrong here?

      Thanks,
        Mark

-- 
Support Richard Stallman against the vicious disinformation campaign
against him and the FSF.  See <https://stallmansupport.org> for more.

Re: A "cosmetic changes" commit that removes security fixes

[Léo Le Bouter]

[-- Attachment #1: Type: text/plain, Size: 1894 bytes --]

On Sat, 2021-04-24 at 03:46 -0400, Mark H Weaver wrote:
> Hi Léo,
> 
> Léo Le Bouter <lle-bout@zaclys.net> writes:
> 
> > On Fri, 2021-04-23 at 15:18 -0400, Leo Famulari wrote:
> > > Léo and Raghav, you need to keep learning our workflow around
> > > security updates.  It's not okay to remove security patches and
> > > later
> > > update a package to a fixed version in a different commit. `git
> > > rebase` is the tool to learn for cases like this one.
> > 
> > I knew about this but I didnt feel like telling Raghav to do yet
> > another rebase. I felt like Raghav was taking on with so much
> > already.
> 
> This response is what native English speakers call a "red herring"
> <https://en.wikipedia.org/wiki/Red_herring>;: something that misleads
> or
> distracts from a relevant or important question.
> 
> Why do I say that?  As I pointed out elsewhere in this thread,
> 
>   https://lists.gnu.org/archive/html/guix-devel/2021-04/msg00366.html
> 
> it appears that Raghav didn't actually remove the security fixes; it
> appears that *you* did.  Therefore, I fail to see how it could have
> been
> avoided by asking Raghav to do another rebase.
> 
> From the IRC logs cited in the message above, it appears that you
> took
> the liberty of modifying Raghav's "cosmetic changes" commits to also
> remove security fixes in the re-indented packages, while claiming in
> the
> summary lines that you were "ungraft[ing]" cairo and glib.
> 
>   
> https://git.sr.ht/~lle-bout/guix/commit/a045a48dd961f0c5c3d536dcc3fd21d9c08d2d50
>   
> https://git.sr.ht/~lle-bout/guix/commit/6477daa338fbf1c9edacfc3690aca77cacfe0008
> 
> Can you please explain what went wrong here?
> 
>       Thanks,
>         Mark
> 

Hello Mark,

I will not answer anything to you anymore because I feel like you do
not write messages in a constructive way.

Léo

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Tobias Geerinckx-Rice]

[-- Attachment #1: Type: text/plain, Size: 418 bytes --]

Hi Léo,

> https://git.sr.ht/~lle-bout/guix/commit/a045a48dd961f0c5c3d536dcc3fd21d9c08d2d50
> https://git.sr.ht/~lle-bout/guix/commit/6477daa338fbf1c9edacfc3690aca77cacfe0008
> 
> Can you please explain what went wrong here?

Is a reasonable question, shared by all of us, not just Mark.  The 
constructive way forward is to answer it fully.  It's in your best 
interest to do so.

Kind regards,

T G-R

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Ludovic Courtès]

Hi Léo,

Tobias Geerinckx-Rice <me@tobias.gr> skribis:

>> https://git.sr.ht/~lle-bout/guix/commit/a045a48dd961f0c5c3d536dcc3fd21d9c08d2d50
>> https://git.sr.ht/~lle-bout/guix/commit/6477daa338fbf1c9edacfc3690aca77cacfe0008
>> Can you please explain what went wrong here?
>
> Is a reasonable question, shared by all of us, not just Mark.  The
> constructive way forward is to answer it fully.  It's in your best 
> interest to do so.

I concur.  Please reply as soon as you can so we can understand what
happened, restore trust, and collectively avoid such pitfalls in the
future.

Thanks in advance,
Ludo’.

Re: A "cosmetic changes" commit that removes security fixes

[Pjotr Prins]

On Mon, Apr 26, 2021 at 07:21:14PM +0200, Ludovic Courtès wrote:
> Hi Léo,
> 
> Tobias Geerinckx-Rice <me@tobias.gr> skribis:
> 
> >> https://git.sr.ht/~lle-bout/guix/commit/a045a48dd961f0c5c3d536dcc3fd21d9c08d2d50
> >> https://git.sr.ht/~lle-bout/guix/commit/6477daa338fbf1c9edacfc3690aca77cacfe0008
> >> Can you please explain what went wrong here?

To be honest, I think Leo and Raghav have been pretty clear. They made
a mistake and that can happen to all of us. Now before this thing goes
off the rails we should also recognise that everyone is different.
Which is really something to celebrate. Everyone on this thread has
made great contributions in a highly diverse environment to GNU Guix
and that is what matters, really.

We should also recognise that some people may be blunt and direct and
even personal. I don't think that is bad. It may not always be nice
and it may hurt our goals of being inclusive, but then there may be a
cultural component too. What is honest and meaningful to one may be
perceived as hurtful by another. It is somewhat unavoidable in an
international social experiment. Personally I shut up on disagreement,
as these E-mails live much too long.

Ricardo wrote a great E-mail early in this thread. I suggest we bury
the hatchet and move forward. It is clear to me that Leo and Raghav
mean well, won't make the mistake again, and this is taking too much
energy from everyone.

Pj.

Re: A "cosmetic changes" commit that removes security fixes

[Léo Le Bouter]

[-- Attachment #1: Type: text/plain, Size: 2631 bytes --]

On Mon, 2021-04-26 at 17:23 +0200, Tobias Geerinckx-Rice wrote:
> Hi Léo,
> 
> > https://git.sr.ht/~lle-bout/guix/commit/a045a48dd961f0c5c3d536dcc3fd21d9c08d2d50
> > https://git.sr.ht/~lle-bout/guix/commit/6477daa338fbf1c9edacfc3690aca77cacfe0008
> > 
> > Can you please explain what went wrong here?
> 
> Is a reasonable question, shared by all of us, not just Mark.  The 
> constructive way forward is to answer it fully.  It's in your best 
> interest to do so.
> 
> Kind regards,
> 
> T G-R

I am sorry, I will not. It's evident nothing went wrong and Mark is not
asking questions that are beneficial to anyone here besides
contributing to public shaming of people. The fix is already pushed and
thank you to the person that made it and Mark for identifying the
issue, however I don't say thank you for trying to publicly shame
people on the mailing list, both Raghav and me. At best there was an
oversight (like there's many in various commits made everyday to GNU
Guix) where I assumed the latest version of software would contain all
security fixes (as I tend to consider GNONE software such as cairo is
well maintained upstream security-wise, seems not), I don't think
there's anything more to add. I find Mark's way of communicating about
these issues not constructive and unfriendly. I think that if Mark or
anyone else's expect me to answer I think they should not phrase
criticism in a way that they accuse me or anyone else of having made a
mistake. I don't think we should find who is responsible for mistakes,
we could however ask advice on what happened to fix the mistake in case
the person that introduced it cannot. And to ever think I would act in
bad faith towards GNU Guix security when I spent entire weeks checking
and patching CVEs full time, I don't think that would make sense.

On Mon, 2021-04-26 at 19:21 +0200, Ludovic Courtès wrote:
> Hi Léo,
> 
> Tobias Geerinckx-Rice <me@tobias.gr> skribis:
> 
> > > 
https://git.sr.ht/~lle-bout/guix/commit/a045a48dd961f0c5c3d536dcc3fd21d9c08d2d50
> > > 
https://git.sr.ht/~lle-bout/guix/commit/6477daa338fbf1c9edacfc3690aca77cacfe0008
> > > Can you please explain what went wrong here?
> > 
> > Is a reasonable question, shared by all of us, not just Mark.  The
> > constructive way forward is to answer it fully.  It's in your best 
> > interest to do so.
> 
> I concur.  Please reply as soon as you can so we can understand what
> happened, restore trust, and collectively avoid such pitfalls in the
> future.
> 
> Thanks in advance,
> Ludo’.

I don't understand how trust would be lost.

Léo

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

Léo,

We maintainers have been disappointed by Marks harsh tone which do not
meet the project's communication standards, but also by your apparent
lack of will to reply constructively to legitimate criticism.

This is the next in a series of incidents.  The incidents are okay--we
we all make mistakes and that's how we learn--but we interpreted your
responses all too often as dismissive and defensive, rather than
understanding and forward-looking.  This has been causing unnecessary
friction and stress, and is not how we envision peaceful collaboration.

I'm sorry to say your commit privileges have been temporarily
suspended.  After one month, you are invited to get in touch with the
maintainers collective and discuss next steps.

You have done terrific work in Guix in the short time you've been
around: from the POWER9 port, to the many security fixes,
to tracking and reporting issues, and suggesting improvements to the
tools.  I hope you'll eventually rejoin to collaborate in the peaceful
and empathetic fashion that this project encourages.

-- 
Marius (on behalf of the maintainers collective)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Léo Le Bouter]

[-- Attachment #1: Type: text/plain, Size: 2593 bytes --]

On Wed, 2021-04-28 at 17:52 +0200, Marius Bakke wrote:
> Léo,
> 
> We maintainers have been disappointed by Marks harsh tone which do
> not
> meet the project's communication standards, but also by your apparent
> lack of will to reply constructively to legitimate criticism.
> 
> This is the next in a series of incidents.  The incidents are okay
> --we
> we all make mistakes and that's how we learn--but we interpreted your
> responses all too often as dismissive and defensive, rather than
> understanding and forward-looking.  This has been causing unnecessary
> friction and stress, and is not how we envision peaceful
> collaboration.
> 
> I'm sorry to say your commit privileges have been temporarily
> suspended.  After one month, you are invited to get in touch with the
> maintainers collective and discuss next steps.
> 
> You have done terrific work in Guix in the short time you've been
> around: from the POWER9 port, to the many security fixes,
> to tracking and reporting issues, and suggesting improvements to the
> tools.  I hope you'll eventually rejoin to collaborate in the
> peaceful
> and empathetic fashion that this project encourages.
> 

Hello!

To make a statement about this on the public mailing list also, I think
such suspension is largely unfair and unjustified.

It seems I am expected to do peaceful collaboration with people who are
not writing messages in a non-violent manner, and I refuse to engage
further in that context. I do not want to answer Mark or anyone else
who does not write in a friendly way. If that means I cannot be a GNU
Guix contributor then I will not be any longer. It means for me that
GNU Guix is not a safe place for me to contribute to.

I think if anyone expects me to not answer in a dismissive or defensive
way then also they must think of the message they're writing if it is
encouraging a confrontational response or peaceful collaboration. I
don't feel like I have been hindering peaceful collaboration at any
time, since everything I've done in GNU Guix was collaborative work.
With many people in the GNU Guix community everything goes well and is
very peaceful, when there's problems it's with specific people who also
happen to write messages in a way that generates confrontation. I
cannot and I refuse to collaborate with people who are not being
friendly and do not care about the emotions of the humans they're
communicating with, it's the reason I come to GNU Guix in the first
place, but to me it appears it's not the right place for that either
now.

Léo

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Leo Prikler]

Am Donnerstag, den 29.04.2021, 11:13 +0200 schrieb Léo Le Bouter:
> On Wed, 2021-04-28 at 17:52 +0200, Marius Bakke wrote:
> > Léo,
> > 
> > We maintainers have been disappointed by Marks harsh tone which do
> > not
> > meet the project's communication standards, but also by your
> > apparent
> > lack of will to reply constructively to legitimate criticism.
> > 
> > This is the next in a series of incidents.  The incidents are okay
> > --we
> > we all make mistakes and that's how we learn--but we interpreted
> > your
> > responses all too often as dismissive and defensive, rather than
> > understanding and forward-looking.  This has been causing
> > unnecessary
> > friction and stress, and is not how we envision peaceful
> > collaboration.
> > 
> > I'm sorry to say your commit privileges have been temporarily
> > suspended.  After one month, you are invited to get in touch with
> > the
> > maintainers collective and discuss next steps.
> > 
> > You have done terrific work in Guix in the short time you've been
> > around: from the POWER9 port, to the many security fixes,
> > to tracking and reporting issues, and suggesting improvements to
> > the
> > tools.  I hope you'll eventually rejoin to collaborate in the
> > peaceful
> > and empathetic fashion that this project encourages.
> > 
> 
> Hello!
> 
> To make a statement about this on the public mailing list also, I
> think
> such suspension is largely unfair and unjustified.
I personally disagree.  While their tone may have been questionable, I
find it important that both committers and non-committers are able to
call changes into question, everything else would not be democratic. 
Ideally, that would happen during review, but this thread has clearly
indicated that the review process failed in this instance.  Perhaps
informing you privately first is more fair, but pointless assignments
of guilt aside it is an issue that we should all be aware of and learn
from, so as to not repeat it.

> It seems I am expected to do peaceful collaboration with people who
> are
> not writing messages in a non-violent manner, and I refuse to engage
> further in that context. I do not want to answer Mark or anyone else
> who does not write in a friendly way. If that means I cannot be a GNU
> Guix contributor then I will not be any longer. It means for me that
> GNU Guix is not a safe place for me to contribute to.
Even if unfriendly, we are not attacking you on any non-technical
grounds, but instead asking you to do self-criticism and to learn from
your mistake.  You can (and should) call the tone in which this is done
into question, but this does not absolve you from your duty to ensure
package quality standards.

> I think if anyone expects me to not answer in a dismissive or
> defensive
> way then also they must think of the message they're writing if it is
> encouraging a confrontational response or peaceful collaboration. I
> don't feel like I have been hindering peaceful collaboration at any
> time, since everything I've done in GNU Guix was collaborative work.
> With many people in the GNU Guix community everything goes well and
> is
> very peaceful, when there's problems it's with specific people who
> also
> happen to write messages in a way that generates confrontation. I
> cannot and I refuse to collaborate with people who are not being
> friendly and do not care about the emotions of the humans they're
> communicating with, it's the reason I come to GNU Guix in the first
> place, but to me it appears it's not the right place for that either
> now.
The criticisms pointed at you comes not just from Mark, but others as
well.  Others, who I would argue, potentially phrase them in a less
confrontational manner.  Leaving them unanswered just because Mark's
tone was inadequate is in my opinion not justified.

Regards,
Leo

Re: A "cosmetic changes" commit that removes security fixes

[Léo Le Bouter]

[-- Attachment #1: Type: text/plain, Size: 4875 bytes --]

On Thu, 2021-04-29 at 13:46 +0200, Leo Prikler wrote:
> Am Donnerstag, den 29.04.2021, 11:13 +0200 schrieb Léo Le Bouter:
> > On Wed, 2021-04-28 at 17:52 +0200, Marius Bakke wrote:
> > > Léo,
> > > 
> > > We maintainers have been disappointed by Marks harsh tone which
> > > do
> > > not
> > > meet the project's communication standards, but also by your
> > > apparent
> > > lack of will to reply constructively to legitimate criticism.
> > > 
> > > This is the next in a series of incidents.  The incidents are
> > > okay
> > > --we
> > > we all make mistakes and that's how we learn--but we interpreted
> > > your
> > > responses all too often as dismissive and defensive, rather than
> > > understanding and forward-looking.  This has been causing
> > > unnecessary
> > > friction and stress, and is not how we envision peaceful
> > > collaboration.
> > > 
> > > I'm sorry to say your commit privileges have been temporarily
> > > suspended.  After one month, you are invited to get in touch with
> > > the
> > > maintainers collective and discuss next steps.
> > > 
> > > You have done terrific work in Guix in the short time you've been
> > > around: from the POWER9 port, to the many security fixes,
> > > to tracking and reporting issues, and suggesting improvements to
> > > the
> > > tools.  I hope you'll eventually rejoin to collaborate in the
> > > peaceful
> > > and empathetic fashion that this project encourages.
> > > 
> > 
> > Hello!
> > 
> > To make a statement about this on the public mailing list also, I
> > think
> > such suspension is largely unfair and unjustified.
> I personally disagree.  While their tone may have been questionable,
> I
> find it important that both committers and non-committers are able to
> call changes into question, everything else would not be democratic. 
> Ideally, that would happen during review, but this thread has clearly
> indicated that the review process failed in this instance.  Perhaps
> informing you privately first is more fair, but pointless assignments
> of guilt aside it is an issue that we should all be aware of and
> learn
> from, so as to not repeat it.
> 
> > It seems I am expected to do peaceful collaboration with people who
> > are
> > not writing messages in a non-violent manner, and I refuse to
> > engage
> > further in that context. I do not want to answer Mark or anyone
> > else
> > who does not write in a friendly way. If that means I cannot be a
> > GNU
> > Guix contributor then I will not be any longer. It means for me
> > that
> > GNU Guix is not a safe place for me to contribute to.
> Even if unfriendly, we are not attacking you on any non-technical
> grounds, but instead asking you to do self-criticism and to learn
> from
> your mistake.  You can (and should) call the tone in which this is
> done
> into question, but this does not absolve you from your duty to ensure
> package quality standards.
> 
> > I think if anyone expects me to not answer in a dismissive or
> > defensive
> > way then also they must think of the message they're writing if it
> > is
> > encouraging a confrontational response or peaceful collaboration. I
> > don't feel like I have been hindering peaceful collaboration at any
> > time, since everything I've done in GNU Guix was collaborative
> > work.
> > With many people in the GNU Guix community everything goes well and
> > is
> > very peaceful, when there's problems it's with specific people who
> > also
> > happen to write messages in a way that generates confrontation. I
> > cannot and I refuse to collaborate with people who are not being
> > friendly and do not care about the emotions of the humans they're
> > communicating with, it's the reason I come to GNU Guix in the first
> > place, but to me it appears it's not the right place for that
> > either
> > now.
> The criticisms pointed at you comes not just from Mark, but others as
> well.  Others, who I would argue, potentially phrase them in a less
> confrontational manner.  Leaving them unanswered just because Mark's
> tone was inadequate is in my opinion not justified.
> 
> Regards,
> Leo
> 

Hello Leo,

If the criticism is well phrased in the first place, then I have no
issue to address it and go on constructively. The very problem is that
it wasnt well phrased. The rest cannot work great. I am unable to
collaborate in these conditions, I don't think this can change, I am
not even willing to make such change, that is, to accept collaboration
with people who do not care about communicating in a caring way. I
think it is contradictory to GNU Guix spirit to expect me to do that,
and I cannot emotionally do so, even if I wanted to. I have had too
much suffering due to these situations before, I don't want this to
repeat here and now.

Léo

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Arun Isaac]

[-- Attachment #1: Type: text/plain, Size: 642 bytes --]


Hi Guix,

I didn't want to get involved in this argument, but I feel I must
register a dissenting opinion.

> I'm sorry to say your commit privileges have been temporarily
> suspended.  After one month, you are invited to get in touch with the
> maintainers collective and discuss next steps.

I think this suspension goes too far and doesn't help de-escalate the
issue. I think Léo should be cut some slack since it was Mark who opened
poorly with public shaming. I can completely understand why Léo is
responding defensively.

Guix has been a very friendly welcoming community. Let's keep it that
way.

Regards,
Arun

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 524 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Pierre Neidhardt]

[-- Attachment #1: Type: text/plain, Size: 68 bytes --]

I agree with Arun.

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Pjotr Prins]

Peeps, 

I am not a core maintainer, but it should be obvious that core
maintainers would not take a decision to revoke commit rights lightly.
Since it hardly happens is it now a loss of face on both sides which
it should not be.

Marius representing the core maintainers clearly wrote: This is the
next in a series of incidents. I.e., it is not only about Mark and his
communication. Please read carefully. This was not an ad hoc decision.

I think *everyone* will be happy to give Léo his rights back provided
he plays by the rules of the Guix project.

I think it is a good policy to revoke rights on any serious complaint
about quality of work.  

Also, no one really needs commit rights to contribute. That is what core
maintainers are for.

Pj.

Re: A "cosmetic changes" commit that removes security fixes

[Pierre Neidhardt]

[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]

Hi Pjotr,

I haven't really followed the issue, so I couldn't say whether the
decision taken by the core maintainers was right or not.

However, I find that your message is insightful in that it raises a few
questions on _how_ this decision was taken.

> I am not a core maintainer, but it should be obvious that core
> maintainers would not take a decision to revoke commit rights lightly.

I trust that it is the case, but being the devil's advocate, I could
argue that from reading this thread does not make it obvious.  Maybe the
decision process should be made more transparent?

Reading between the lines, I feel that some discussion happened behind
closed curtains between some maintainers.  Correct me if I'm wrong.
I don't know if this is ideal in such circumstances, but if it is, then
it is probably a good idea to mention it.

Another question one could ask: why just the core maintainers actually?
Shouldn't everyone be involved?  Maybe the right answer is "no" here,
and if so, I believe we should explain why in the community guidelines.
Lest the community present an image where a few would benefit from
arbitrary privileges.  It'd be nice to explicit these and the reason
behind the various roles found among the members of the community.

Last, maybe a more important question: if core maintainers are entrusted
to take executive decisions about the community members, what about
executive decisions about the core maintainers themselves?  Are there
such provisions?  Example: what if a core maintainers misbehaves?  Can
they see there privileges revoked?  How?  Is this documented?

> Marius representing the core maintainers clearly wrote: This is the
> next in a series of incidents.

Considering this is the main cause for the decision, I believe it's
important to detail it with references.  "a series of incidents" is too
vague and in isolation, it does not seem to justify the decision very
well.  It seems necessary to me to recap the whole series of points
that led to the decision.



So maybe there are some issues we could address with regard to the
structural organization of the Guix community, which could help making
it increasingly more welcoming, peaceful and strong.

Food for thoughts! :)

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Pjotr Prins]

On Fri, Apr 30, 2021 at 07:40:36PM +0200, Pierre Neidhardt wrote:
> I trust that it is the case, but being the devil's advocate, I could
> argue that from reading this thread does not make it obvious.  Maybe the
> decision process should be made more transparent?

Let's not make this a big thing. I am not a core committer - don't
want to be one. What I know is that commit rights are given to people
who are expected to play by the rules of the project.

Is that hard to understand?

When someone does not play by the rules and is unapologetic - and it
happens multiple times - it is completely justified to take away those
rights.

Being a core committer is *not* a badge of honour. It does not give
special privileges beyond what is expected. It does not give you
money, a degree or a knighthood. It only allows you to push code
directly where it interferes with the work of others :)

Everyone will be *happy* to give the commit rights again, provided the
person plays by the rules.

Pj.

Re: A "cosmetic changes" commit that removes security fixes

[Arun Isaac]

[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]


Hi everyone,

This decision aside, I share some of the general concerns raised by
Pierre about core maintainership and the behind closed doors decision
making process.

> Being a core committer is *not* a badge of honour. It does not give
> special privileges beyond what is expected. It does not give you
> money, a degree or a knighthood. It only allows you to push code
> directly where it interferes with the work of others :)

We may like to imagine that being a core maintainer is not a badge of
honor, but in reality, it *is* a badge of honor. A core maintainer is
not just a regular participant any more than the President is just a
public servant. If core maintainership is not to be associated with
power and honor, we should put in place processes to achieve that. In
that regard, Guix has done better than most other free software
projects. For example, we don't assign people as maintainers (or owners)
of specific packages or groups of packages like other distributions
have. Still, I'm sure there's more we can do.

Cheers!
Arun

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 524 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Pjotr Prins]

On Sat, May 01, 2021 at 12:53:43PM +0530, Arun Isaac wrote:
> We may like to imagine that being a core maintainer is not a badge of
> honor, but in reality, it *is* a badge of honor. A core maintainer is
> not just a regular participant any more than the President is just a
> public servant. If core maintainership is not to be associated with
> power and honor, we should put in place processes to achieve that. 

It is an interesting perspective. One problem I see with assuming a
badge of honor is that it will make it much harder to decide on giving
commit rights because it will have to be based on merit/selection and
make even harder to revoke them. Are you suggesting we elect
committers?

At this point, my personal assumption about the procedure and what I
have seen is that if someone is promising and active they get commit
rights - i.e., to make it easier on everyone who is maintaining Guix.
It is about flow of work and convenience. 

A core committer is arguably not even a core maintainer.

Nothing makes me think a core committer is comparable to an elected
president.

The Debian project is much larger than Guix and has unwieldy selection
criteria for people joining certain levels. Is that what we want? One
of the attractions of Guix to me is that it is a flat project and, as
far as I can tell, we have mostly avoided regular politics distracting
us from real work. We have channels so people can do what they want.
No reason to meddle with the core project if you are not so inclined.

I would vote to keep the Guix project structure as simple and flat as
possible without overheads. Nimble is the word. Want to build your own
cathedral? Start a channel. 

Pj.

PS: I am not a core committer and not a core maintainer. All opinions
are my own.

Re: A "cosmetic changes" commit that removes security fixes

[Pierre Neidhardt]

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

Pjotr Prins <pjotr.public12@thebird.nl> writes:

> Being a core committer is *not* a badge of honour. It does not give
> special privileges beyond what is expected.

But this may not be true.  Quite a few people have expressed otherwise
over the years.

Maybe an issue is that much of the organizational structure of Guix is
implicit.  The roles are not well defined and as a result, they may
reach beyond what is commonly understood.

Some time ago, I was discussing the quetsion of implicit structure with
Ludo, who sent me this essay:

  https://www.jofreeman.com/joreen/tyranny.htm

I found it enlightening and quite relevant to the matter at hand.

Even if in the end my concerns end up being unjustified, I find it good
practice to always question the status quo: maybe something better could
come out of it! :)

Cheers!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Yasuaki Kudo]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

Hello!

I don't know the details of the case at all but let met mention this:
https://communityrule.info/
It comes from the world of worker cooperatives and I think them "rules of the community" is discussed a lot there as well 😄

Cheers,
Yasu

> On May 1, 2021, at 18:16, Pierre Neidhardt <mail@ambrevar.xyz> wrote:
> 
> Pjotr Prins <pjotr.public12@thebird.nl> writes:
> 
>> Being a core committer is *not* a badge of honour. It does not give
>> special privileges beyond what is expected.
> 
> But this may not be true.  Quite a few people have expressed otherwise
> over the years.
> 
> Maybe an issue is that much of the organizational structure of Guix is
> implicit.  The roles are not well defined and as a result, they may
> reach beyond what is commonly understood.
> 
> Some time ago, I was discussing the quetsion of implicit structure with
> Ludo, who sent me this essay:
> 
>  https://www.jofreeman.com/joreen/tyranny.htm
> 
> I found it enlightening and quite relevant to the matter at hand.
> 
> Even if in the end my concerns end up being unjustified, I find it good
> practice to always question the status quo: maybe something better could
> come out of it! :)
> 
> Cheers!
> 
> -- 
> Pierre Neidhardt
> https://ambrevar.xyz/

[-- Attachment #2: Type: text/html, Size: 2190 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Pierre Neidhardt]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

Hi!

Yasuaki Kudo <yasu@yasuaki.com> writes:

> I don't know the details of the case at all but let met mention this:
> https://communityrule.info/
> It comes from the world of worker cooperatives and I think them "rules of the community" is discussed a lot there as well 😄

Thanks for sharing, I didn't know about this!
I think it's a great starting point.  I like these ones in particular:

    https://communityrule.info/create/?r=consensus
    https://communityrule.info/create/?r=jury

communityrule.info helps identifying different types of governance, with
different strategies to various decision problems.

Let's note this down and bring it back up in a thread that's more
focused on the governance question of Guix.

Cheers!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Giovanni Biscuolo]

[-- Attachment #1: Type: text/plain, Size: 4199 bytes --]

Hi Pierre,

Pierre Neidhardt <mail@ambrevar.xyz> writes:

> I haven't really followed the issue,

I have, very carefully ;-)

> so I couldn't say whether the decision taken by the core maintainers
> was right or not.

From my point of view it was /but/ this is *not* relevant: what's
relevant here is that /if/ we trust Guix maintainers (I do) when they
give commit access rights to people, whe /have/ to trust them when they
revoke those rights.  We /should/ disccus /if/ the rules and best
practices to have and maintain the commit acces are well documented:
please make proposals (patches wellcome :-) ) but please we have to keep
trusting Guix maintainers (that is a collective of very competent
people).

[...]

>> I am not a core maintainer, but it should be obvious that core
>> maintainers would not take a decision to revoke commit rights lightly.
>
> I trust that it is the case, but being the devil's advocate,

Please don't:
«Why the Devil's Advocate Doesn't Help Reach the Truth»
https://www.gnu.org/philosophy/devils-advocate.html

--8<---------------cut here---------------start------------->8---

The devil achieves that by twisting my words: presenting a misleading
context in which my words appear to mean something other than what I
intended.

--8<---------------cut here---------------end--------------->8---

;-)

[...]

> Another question one could ask: why just the core maintainers
> actually?  Shouldn't everyone be involved?  Maybe the right answer is
> "no" here, and if so, I believe we should explain why in the community
> guidelines.

Guix is a GNU project and AFAIU GNU project management is well
documented:

https://www.gnu.org/gnu/gnu-structure.html

I don't know if Guix project needs specific «GNU Guix structure»
documentation but /if/ the answer is yes it should complement the
official GNU one, not replace it, IMHO.

BTW I see Guix contributors with commit access as "package maintanance
assistants" delegated by maintainers to make some technical decisions:

--8<---------------cut here---------------start------------->8---

The maintainers of a package often recruit others to contribute to its
development, and delegate some technical decisions to them. However,
the maintainers retain authority over the whole of the package so they
can carry out their responsibility to the GNU Project.

--8<---------------cut here---------------end--------------->8---

Please we should always consider that GNU maintainers are the persons
that carry out the responsibility to the GNU project, not contributors
with commit access.

Maybe the contributing section of Guix manual should mention it and link
the relevant GNU project's documents: do you think it'd be useful?

> Lest the community present an image where a few would benefit from
> arbitrary privileges.

...or seen from /the other side of the moon/: a few carry out the
precious work to be /responsible/ to do a good, practical job of
developing and maintaining Guix according to the GNU project's mission
and general decision.  If you want call it /arbitrary privilege/ but I
have a different point of view :-D

The "community" (whatever this means) should acknowledge that
contributing also means to be responsible toward other users of free
software: this needs competence in the specific matter (also domain
specific), discipline (i.e. properly documenting changes in commit
messages) and commitment to a set of common shared rules (documented in
Guix and GNU project manual).

[...]

> Last, maybe a more important question: if core maintainers are
> entrusted to take executive decisions about the community members,
> what about executive decisions about the core maintainers themselves?

Maintainers are appointed by the GNU project.

> Are there such provisions?  Example: what if a core maintainers
> misbehaves?  Can they see there privileges revoked?  How?  Is this
> documented?

«GNU Software Evaluation»
https://www.gnu.org/help/evaluation.html#whatmeans

Does this answer your question?

[...]

Happy hacking! Gio'

-- 
Giovanni Biscuolo

Xelera IT Infrastructures

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 849 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Pierre Neidhardt]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

Hi Giovanni,

> Guix is a GNU project and AFAIU GNU project management is well
> documented:
>
> https://www.gnu.org/gnu/gnu-structure.html

This applies to GNU at the top level, but it does not specify how
sub-projects (referred to as "packages" in the aforementioned document)
are governed locally.  This question is mostly left unanswered as I
understand it.

Cheers!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Bengt Richter]

Hi Pierre,

On +2021-05-03 09:25:21 +0200, Pierre Neidhardt wrote:
> Hi Giovanni,
> 
> > Guix is a GNU project and AFAIU GNU project management is well
> > documented:
> >
> > https://www.gnu.org/gnu/gnu-structure.html
> 
> This applies to GNU at the top level, but it does not specify how
> sub-projects (referred to as "packages" in the aforementioned document)
> are governed locally.  This question is mostly left unanswered as I
> understand it.
>

You may find some clues here:
    https://www.gnu.org/help/evaluation.html#whatmeans
And links to more :)

> Cheers!
> 
> -- 
> Pierre Neidhardt
> https://ambrevar.xyz/

-- 
Regards,
Bengt Richter

Re: A "cosmetic changes" commit that removes security fixes

[Pierre Neidhardt]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

Hi Bengt,

>> This applies to GNU at the top level, but it does not specify how
>> sub-projects (referred to as "packages" in the aforementioned document)
>> are governed locally.  This question is mostly left unanswered as I
>> understand it.
>>
>
> You may find some clues here:
>     https://www.gnu.org/help/evaluation.html#whatmeans
> And links to more :)

Thanks for sharing.  I've read it and it does not seem to be concerned
with the question of governance.

At the bottom, I found this long manual which may offer more discussion:

  https://www.gnu.org/prep/maintain/

But one has to dissect it first :)

Cheers!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Ludovic Courtès]

Hi,

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

> Thanks for sharing.  I've read it and it does not seem to be concerned
> with the question of governance.

For the record, you can read about Guix roles and responsibilities at:

  https://guix.gnu.org/en/blog/2019/gnu-guix-maintainer-collective-expands/

The “Contributing” section of the manual discusses specific policies:

  https://guix.gnu.org/manual/en/html_node/Contributing.html

Evidently, some aspects have yet to be formalized.

HTH,
Ludo’.

Re: A "cosmetic changes" commit that removes security fixes

[Pierre Neidhardt]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

Hi Ludo,

Thanks for the links, it's a good starting point.

> For the record, you can read about Guix roles and responsibilities at:
>
>   https://guix.gnu.org/en/blog/2019/gnu-guix-maintainer-collective-expands/

I think a good next step would be to store this information at a more
accessible location on https://guix.gnu.org.  A visitor looking for this
kind of information may not think to browse the blog entries.  It's
also not convenient, since it gets buried deeper and deeper as articles
get published.

> The “Contributing” section of the manual discusses specific policies:
>
>   https://guix.gnu.org/manual/en/html_node/Contributing.html

This page mentions the code of conduct, which gives some guidelines on
communication, but as I understand it, it does not say anything about
structure or governance.

The code of conduct itself could be expanded.  For a start, we could
address some characteristic communication issues that have occurred on
the mailing list over time.  For example, we could add a section on
"Accountability != blame", to paraphrase Jelle.
Another section (a very important one in my opinion) could be about
giving the benefit of the doubt and _ask why_ before policing someone.

Food for thoughts!

Cheers!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Arun Isaac]

[-- Attachment #1: Type: text/plain, Size: 720 bytes --]


Hi Guix,

>> I'm sorry to say your commit privileges have been temporarily
>> suspended.  After one month, you are invited to get in touch with the
>> maintainers collective and discuss next steps.
>
> I think this suspension goes too far and doesn't help de-escalate the
> issue. I think Léo should be cut some slack since it was Mark who opened
> poorly with public shaming. I can completely understand why Léo is
> responding defensively.

It looks like I made this statement based on this instance only and
without considering earlier instances. I normally don't read most mail
on guix-devel. It's too high volume for me. Sorry about that. Please
consider my statement withdrawn.

Thanks,
Arun

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 524 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Léo Le Bouter]

[-- Attachment #1: Type: text/plain, Size: 3168 bytes --]

On Fri, 2021-04-23 at 15:18 -0400, Leo Famulari wrote:
> I have to agree with everybody in this thead.
> 
> The commits in question were problematic (especially on core-updates,
> which is not a "WIP" branch and thus cannot be rewritten to fix past
> problems). I'm not confident that the security fixes would have been
> reinstated on core-updates if Mark had not asked about them.
> 
> Léo and Raghav, you need to keep learning our workflow around
> security
> updates.  It's not okay to remove security patches and later update a
> package to a fixed version in a different commit. `git rebase` is the
> tool to learn for cases like this one.

I do not think here that Raghav and myself should somehow be framed as
people having to learn more and that would be the reason for these
issues. To talk about myself, I think the main difference here is that
Mark and myself consider different things to have value when
contributing to GNU Guix. Mark tends to consider the technicality of
contributing to GNU Guix, that the code be well tested, that every
change be made in a very rigorous way. I tend to also consider these
things but also consider other things like how people feel when they
contribute to GNU Guix, do they feel discouraged or rewarded by their
contributions, I find that it can be tiring and very discouraging to
respond back and forth to many many review comments, and at some point,
even if things have some rough edges, I tend to prefer rewarding a
contributor for their work than insist the commit history should be
perfect or something. I also stopped upholding myself to high rigorous
standards at all times, also because I think it is not good for my
mental health. I tend to move the responsability of rigorous testing
into tools, I think putting testing/checking into tools is at the same
time good for mental health and inclusive because it means also
everyone can check their own changes and correct errors. Having tools
to check things is less stressful for everyone, I discovered that
aspect after I learned Rust and I think it really is the way to go. I
think there is an aspect of contribution where people feel stressed and
doubt themselves and that's what keeps them away from contribution, if
we have tools then those problems tend to disappear because the tool
acts as a stopgap, the tool can also be collectively improved as soon
as more best practices are discovered by the community. Rust does this
with clippy lint rules, the borrow checker and the very well made
error/warning reporting of the compiler. I think relying more on tools
even if we never can do so fully and less on individual accountability
is better here.

> 
> However, Mark, you have way more experience, and you need to handle
> these things differently. If you don't trust certain Guix
> contributors,
> take it up with the maintainers — in private. The style of
> communication
> you used here is ineffective and will dissuade people from
> contributing
> to Guix. Do you want Léo and Raghav to learn and improve? Or do you
> want
> them to leave? Remember that we all begin as beginners.

Léo

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: A "cosmetic changes" commit that removes security fixes

[Andreas Enge]

Hello Léo,

Am Mon, Apr 26, 2021 at 09:31:18PM +0200 schrieb Léo Le Bouter:
> also consider other things like how people feel when they
> contribute to GNU Guix, do they feel discouraged or rewarded by their
> contributions

indeed that is an important aspect.

> I find that it can be tiring and very discouraging to
> respond back and forth to many many review comments, and at some point,
> even if things have some rough edges, I tend to prefer rewarding a
> contributor for their work than insist the commit history should be
> perfect or something. I also stopped upholding myself to high rigorous
> standards at all times, also because I think it is not good for my
> mental health.

I agree that it can be a bit tiring, but at the same time, I think that
the high quality of Guix is a very important feature, that sets it apart
from some other distros. It is definitely one of the reasons I am using
it and have been contributing to it. And it has been one of the defining
features of Guix from the start that we try to avoid rough edges as much
as possible. Like building texlive from source instead of wrapping Debian
binaries, to give just one example. Or striving for bootstrappability,
which can be seen as removing rough edges at the expense of extraordinary
efforts.

Luckily, I do not think that enjoying contributing and keeping high quality
standards are in contradiction.

> I tend to move the responsability of rigorous testing
> into tools, I think putting testing/checking into tools is at the same
> time good for mental health and inclusive because it means also
> everyone can check their own changes and correct errors.

Tooling can definitely be helpful with this. We have "guix lint", and if
you have ideas for improved tools, these would be very welcome.

Andreas