[PATCH] tar bombs and muscle

[PATCH] tar bombs and muscle

[Ben Woodcroft]

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

Hi,

There is a somewhat popular bioinformatics program muscle whose download 
tgz is a tar bomb. The bomb moniker seems especially appropriate here, 
since it made the gnu-build-system error out, and patching 
gnu-build-system requires a lot of rebuilding. In the attached patches I 
fixed gnu-build-system so that the "chdir" is omitted when there is no 
directory to chdir into, and then added muscle itself.

Is it OK in these rare instances to put the archive contents into the 
directory as-is, or is something more complex like making a directory 
and moving everything there more appropriate?

I imagine it might be best to let this slide into the next core-updates.

Thanks,
ben

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-gnu-Add-muscle.patch --]
[-- Type: text/x-patch; name="0002-gnu-Add-muscle.patch", Size: 1995 bytes --]

From 9400f88a9f70d47f1e835a98842b894e640d0e4a Mon Sep 17 00:00:00 2001
From: Ben Woodcroft <donttrustben@gmail.com>
Date: Sat, 16 Jan 2016 22:12:23 +1000
Subject: [PATCH 2/2] gnu: Add muscle.

* gnu/packages/bioinformatics.scm (muscle): New variable.
---
 gnu/packages/bioinformatics.scm | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/gnu/packages/bioinformatics.scm b/gnu/packages/bioinformatics.scm
index a905ccf..4a7a2c2 100644
--- a/gnu/packages/bioinformatics.scm
+++ b/gnu/packages/bioinformatics.scm
@@ -2176,6 +2176,37 @@ the ability to go from raw sequences to the generation of visualization tools
 to describe ecological α and β diversity measurements.")
     (license license:gpl3)))
 
+(define-public muscle
+  (package
+    (name "muscle")
+    (version "3.8.1551")
+    (source (origin
+              (method url-fetch)
+              (uri (string-append
+                    "http://www.drive5.com/muscle/muscle_src_"
+                    version ".tar.gz"))
+              (sha256
+               (base32
+                "0bj8kj7sdizy3987zx6w7axihk40fk8rn76mpbqqjcnd64i5a367"))))
+    (build-system gnu-build-system)
+    (arguments
+     `(#:make-flags (list "LDLIBS = -lm")
+       #:tests? #f ; no tests
+       #:phases
+       (modify-phases %standard-phases
+         (delete 'configure)
+         (replace 'install
+           (lambda* (#:key outputs #:allow-other-keys)
+             (let* ((out (assoc-ref outputs "out"))
+                    (bin (string-append out "/bin")))
+               (install-file "muscle" bin)))))))
+    (home-page "http://www.drive5.com/muscle")
+    (synopsis "Multiple sequence alignment program")
+    (description
+     "MUSCLE aims to be a fast and accurate multiple sequence
+alignment program for nucleotide and protein sequences.")
+    (license license:public-domain)))
+
 (define-public orfm
   (package
     (name "orfm")
-- 
2.6.3


[-- Attachment #3: 0001-build-Accept-source-archives-that-do-not-contain-a-d.patch --]
[-- Type: text/x-patch, Size: 1187 bytes --]

From 9e272374a3531d1df6d36e595dad0b715731e4cd Mon Sep 17 00:00:00 2001
From: Ben Woodcroft <donttrustben@gmail.com>
Date: Sat, 16 Jan 2016 22:02:22 +1000
Subject: [PATCH 1/2] build: Accept source archives that do not contain a
 directory.

* guix/build/gnu-build-system.scm (unpack): Do not attempt to change
directory after extracting archive if the archive does not contain any
directories.
---
 guix/build/gnu-build-system.scm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/guix/build/gnu-build-system.scm b/guix/build/gnu-build-system.scm
index ff7646b..f1a84ef 100644
--- a/guix/build/gnu-build-system.scm
+++ b/guix/build/gnu-build-system.scm
@@ -142,7 +142,10 @@ working directory."
       (and (if (string-suffix? ".zip" source)
                (zero? (system* "unzip" source))
                (zero? (system* "tar" "xvf" source)))
-           (chdir (first-subdirectory ".")))))
+           (let ((subdirectory (first-subdirectory ".")))
+             (if subdirectory
+                 (chdir (first-subdirectory "."))
+                 #t)))))
 
 ;; See <http://bugs.gnu.org/17840>.
 (define* (patch-usr-bin-file #:key native-inputs inputs
-- 
2.6.3

Re: [PATCH] tar bombs and muscle

[Ben Woodcroft]

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



On 17/01/16 11:30, Ben Woodcroft wrote:
> Hi,
>
> There is a somewhat popular bioinformatics program muscle whose 
> download tgz is a tar bomb. The bomb moniker seems especially 
> appropriate here, since it made the gnu-build-system error out, and 
> patching gnu-build-system requires a lot of rebuilding. In the 
> attached patches
Oops, I wasn't coming off master, so those packages are not apply-able. 
Attached is better.

[-- Attachment #2: 0001-build-Accept-source-archives-that-do-not-contain-a-d.patch --]
[-- Type: text/x-patch, Size: 1187 bytes --]

From 530d81289ef5cab7010209fe0604a82b73459e4c Mon Sep 17 00:00:00 2001
From: Ben Woodcroft <donttrustben@gmail.com>
Date: Sat, 16 Jan 2016 22:02:22 +1000
Subject: [PATCH 1/2] build: Accept source archives that do not contain a
 directory.

* guix/build/gnu-build-system.scm (unpack): Do not attempt to change
directory after extracting archive if the archive does not contain any
directories.
---
 guix/build/gnu-build-system.scm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/guix/build/gnu-build-system.scm b/guix/build/gnu-build-system.scm
index ff7646b..f1a84ef 100644
--- a/guix/build/gnu-build-system.scm
+++ b/guix/build/gnu-build-system.scm
@@ -142,7 +142,10 @@ working directory."
       (and (if (string-suffix? ".zip" source)
                (zero? (system* "unzip" source))
                (zero? (system* "tar" "xvf" source)))
-           (chdir (first-subdirectory ".")))))
+           (let ((subdirectory (first-subdirectory ".")))
+             (if subdirectory
+                 (chdir (first-subdirectory "."))
+                 #t)))))
 
 ;; See <http://bugs.gnu.org/17840>.
 (define* (patch-usr-bin-file #:key native-inputs inputs
-- 
2.6.3


[-- Attachment #3: 0002-gnu-Add-muscle.patch --]
[-- Type: text/x-patch, Size: 1930 bytes --]

From 567ddebd4f9a8d9e0b5681dae1ec639987c69064 Mon Sep 17 00:00:00 2001
From: Ben Woodcroft <donttrustben@gmail.com>
Date: Sat, 16 Jan 2016 22:12:23 +1000
Subject: [PATCH 2/2] gnu: Add muscle.

* gnu/packages/bioinformatics.scm (muscle): New variable.
---
 gnu/packages/bioinformatics.scm | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/gnu/packages/bioinformatics.scm b/gnu/packages/bioinformatics.scm
index 314d0ad..f9d8068 100644
--- a/gnu/packages/bioinformatics.scm
+++ b/gnu/packages/bioinformatics.scm
@@ -2070,6 +2070,37 @@ RNA-Seq, the MISO model uses Bayesian inference to compute the probability
 that a read originated from a particular isoform.")
     (license license:gpl2)))
 
+(define-public muscle
+  (package
+    (name "muscle")
+    (version "3.8.1551")
+    (source (origin
+              (method url-fetch)
+              (uri (string-append
+                    "http://www.drive5.com/muscle/muscle_src_"
+                    version ".tar.gz"))
+              (sha256
+               (base32
+                "0bj8kj7sdizy3987zx6w7axihk40fk8rn76mpbqqjcnd64i5a367"))))
+    (build-system gnu-build-system)
+    (arguments
+     `(#:make-flags (list "LDLIBS = -lm")
+       #:tests? #f ; no tests
+       #:phases
+       (modify-phases %standard-phases
+         (delete 'configure)
+         (replace 'install
+           (lambda* (#:key outputs #:allow-other-keys)
+             (let* ((out (assoc-ref outputs "out"))
+                    (bin (string-append out "/bin")))
+               (install-file "muscle" bin)))))))
+    (home-page "http://www.drive5.com/muscle")
+    (synopsis "Multiple sequence alignment program")
+    (description
+     "MUSCLE aims to be a fast and accurate multiple sequence
+alignment program for nucleotide and protein sequences.")
+    (license license:public-domain)))
+
 (define-public orfm
   (package
     (name "orfm")
-- 
2.6.3

Re: [PATCH] tar bombs and muscle

[Eric Bavier]

On Sun, 17 Jan 2016 11:30:03 +1000
Ben Woodcroft <b.woodcroft@uq.edu.au> wrote:

> There is a somewhat popular bioinformatics program muscle whose download 
> tgz is a tar bomb. The bomb moniker seems especially appropriate here, 
> since it made the gnu-build-system error out, and patching 
> gnu-build-system requires a lot of rebuilding. In the attached patches I 
> fixed gnu-build-system so that the "chdir" is omitted when there is no 
> directory to chdir into, and then added muscle itself.

See https://lists.gnu.org/archive/html/guix-devel/2016-01/msg00165.html

I think it might be what you're looking for.

`~Eric

Re: [PATCH] tar bombs and muscle

[Ben Woodcroft]

On 17/01/16 14:29, Eric Bavier wrote:
> On Sun, 17 Jan 2016 11:30:03 +1000
> Ben Woodcroft <b.woodcroft@uq.edu.au> wrote:
>
>> There is a somewhat popular bioinformatics program muscle whose download
>> tgz is a tar bomb. The bomb moniker seems especially appropriate here,
>> since it made the gnu-build-system error out, and patching
>> gnu-build-system requires a lot of rebuilding. In the attached patches I
>> fixed gnu-build-system so that the "chdir" is omitted when there is no
>> directory to chdir into, and then added muscle itself.
> See https://lists.gnu.org/archive/html/guix-devel/2016-01/msg00165.html
>
> I think it might be what you're looking for.
It is thanks, I'd not seen that. I wonder if an error message mentioning 
that might be in order, given it is easy to test for. Anyway, I'll wait 
until that change is merged.

ben

Re: [PATCH] tar bombs and muscle

[Ricardo Wurmus]

Ben Woodcroft <b.woodcroft@uq.edu.au> writes:

> There is a somewhat popular bioinformatics program muscle whose download 
> tgz is a tar bomb. The bomb moniker seems especially appropriate here, 
> since it made the gnu-build-system error out, and patching 
> gnu-build-system requires a lot of rebuilding. In the attached patches I 
> fixed gnu-build-system so that the "chdir" is omitted when there is no 
> directory to chdir into, and then added muscle itself.

Maybe I misunderstood, but couldn’t you just replace the “unpack” phase
for this package?

~~ Ricardo

Re: [PATCH] tar bombs and muscle

[Mathieu Lirzin]

Hi,

Ben Woodcroft <b.woodcroft@uq.edu.au> writes:

> There is a somewhat popular bioinformatics program muscle whose
> download tgz is a tar bomb. The bomb moniker seems especially
> appropriate here, since it made the gnu-build-system error out, and
> patching gnu-build-system requires a lot of rebuilding. In the
> attached patches

IMO distributing a tar bomb is a bug, So I would prefer Guix not to work
around it silently.  If it is rare, replacing the unpack phase manually
should be enough.  However If it is common, we could add a procedure in
(guix build utils) to avoid repetition of the same chunk of code.

WDYT?

--
Mathieu Lirzin

Re: [PATCH] tar bombs and muscle

[Pjotr Prins]

On Sun, Jan 17, 2016 at 10:30:56AM +0100, Mathieu Lirzin wrote:
> Hi,
> 
> Ben Woodcroft <b.woodcroft@uq.edu.au> writes:
> 
> > There is a somewhat popular bioinformatics program muscle whose
> > download tgz is a tar bomb. The bomb moniker seems especially
> > appropriate here, since it made the gnu-build-system error out, and
> > patching gnu-build-system requires a lot of rebuilding. In the
> > attached patches
> 
> IMO distributing a tar bomb is a bug, So I would prefer Guix not to work
> around it silently.  If it is rare, replacing the unpack phase manually
> should be enough.  However If it is common, we could add a procedure in
> (guix build utils) to avoid repetition of the same chunk of code.

It is rare these days.

Re: [PATCH] tar bombs and muscle

[Ben Woodcroft]

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



On 17/01/16 14:29, Eric Bavier wrote:
> On Sun, 17 Jan 2016 11:30:03 +1000
> Ben Woodcroft <b.woodcroft@uq.edu.au> wrote:
>
>> There is a somewhat popular bioinformatics program muscle whose download
>> tgz is a tar bomb. The bomb moniker seems especially appropriate here,
>> since it made the gnu-build-system error out, and patching
>> gnu-build-system requires a lot of rebuilding. In the attached patches I
>> fixed gnu-build-system so that the "chdir" is omitted when there is no
>> directory to chdir into, and then added muscle itself.
> See https://lists.gnu.org/archive/html/guix-devel/2016-01/msg00165.html
>
> I think it might be what you're looking for.

Now that url-fetch/tarbomb is available for general use, I've updated 
this patch and added a trivial test case. Thanks in advance for review.

ben

[-- Attachment #2: 0001-gnu-Add-muscle.patch --]
[-- Type: text/x-patch, Size: 2126 bytes --]

From 014be68f9e82847761f47f0363fa106bf708f14c Mon Sep 17 00:00:00 2001
From: Ben Woodcroft <donttrustben@gmail.com>
Date: Sat, 16 Jan 2016 22:12:23 +1000
Subject: [PATCH] gnu: Add muscle.

* gnu/packages/bioinformatics.scm (muscle): New variable.
---
 gnu/packages/bioinformatics.scm | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/gnu/packages/bioinformatics.scm b/gnu/packages/bioinformatics.scm
index 899ce1c..6e51dde 100644
--- a/gnu/packages/bioinformatics.scm
+++ b/gnu/packages/bioinformatics.scm
@@ -2548,6 +2548,40 @@ RNA-Seq, the MISO model uses Bayesian inference to compute the probability
 that a read originated from a particular isoform.")
     (license license:gpl2)))
 
+(define-public muscle
+  (package
+    (name "muscle")
+    (version "3.8.1551")
+    (source (origin
+              (method url-fetch/tarbomb)
+              (file-name (string-append name "-" version ".tar.gz"))
+              (uri (string-append
+                    "http://www.drive5.com/muscle/muscle_src_"
+                    version ".tar.gz"))
+              (sha256
+               (base32
+                "0bj8kj7sdizy3987zx6w7axihk40fk8rn76mpbqqjcnd64i5a367"))))
+    (build-system gnu-build-system)
+    (arguments
+     `(#:make-flags (list "LDLIBS = -lm")
+       #:phases
+       (modify-phases %standard-phases
+         (delete 'configure)
+         (replace 'check
+           ;; There are no tests, so just test if it runs.
+           (lambda _ (zero? (system* "./muscle" "-version"))))
+         (replace 'install
+           (lambda* (#:key outputs #:allow-other-keys)
+             (let* ((out (assoc-ref outputs "out"))
+                    (bin (string-append out "/bin")))
+               (install-file "muscle" bin)))))))
+    (home-page "http://www.drive5.com/muscle")
+    (synopsis "Multiple sequence alignment program")
+    (description
+     "MUSCLE aims to be a fast and accurate multiple sequence alignment
+program for nucleotide and protein sequences.")
+    (license license:public-domain)))
+
 (define-public orfm
   (package
     (name "orfm")
-- 
2.6.3

Re: [PATCH] tar bombs and muscle

[Leo Famulari]

On Sun, Apr 24, 2016 at 10:04:01AM +1000, Ben Woodcroft wrote:
> * gnu/packages/bioinformatics.scm (muscle): New variable.

> +              (file-name (string-append name "-" version ".tar.gz"))

Once fetched, the source code is a directory rather than a tarball, so
I think it's best to omit the last component of the string.

> +         (replace 'install
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             (let* ((out (assoc-ref outputs "out"))
> +                    (bin (string-append out "/bin")))
> +               (install-file "muscle" bin)))))))

It only creates the one executable?

> +    (license license:public-domain)))

Wow, they really don't make it easy to find this information. Can you
add a comment saying that it's in 'usage.cpp'? Unless I missed something
obvious...

Otherwise, looks good to me!

Re: [PATCH] tar bombs and muscle

[Ben Woodcroft]

On 30/04/16 12:13, Leo Famulari wrote:
> On Sun, Apr 24, 2016 at 10:04:01AM +1000, Ben Woodcroft wrote:
>> * gnu/packages/bioinformatics.scm (muscle): New variable.
>> +              (file-name (string-append name "-" version ".tar.gz"))
> Once fetched, the source code is a directory rather than a tarball, so
> I think it's best to omit the last component of the string.
>
>> +         (replace 'install
>> +           (lambda* (#:key outputs #:allow-other-keys)
>> +             (let* ((out (assoc-ref outputs "out"))
>> +                    (bin (string-append out "/bin")))
>> +               (install-file "muscle" bin)))))))
> It only creates the one executable?
>
>> +    (license license:public-domain)))
> Wow, they really don't make it easy to find this information. Can you
> add a comment saying that it's in 'usage.cpp'? Unless I missed something
> obvious...
>
> Otherwise, looks good to me!
Pushed with those changes. Thanks.