Arch Linux

harrigan

Member

Registered: 2018-11-09

Posts: 1

patching in prepare() function

I'm trying to learn about the details of PKGBUILD and am looking at the patch PKGBUILD as an example.

It looks like prepare() explicitly applies some of the patches listed in source, but not all.  Here is source:

source=("https://ftp.gnu.org/gnu/$pkgname/$pkgname-$pkgver.tar.xz"{,.sig}
        "https://github.com/mirror/patch/commit/ … 5e6a.patch" # CVE-2018-6951
        "http://git.savannah.gnu.org/cgit/patch. … ca5909c26d" # CVE-2018-1000156
        "https://github.com/mirror/patch/commit/ … c300.patch" # CVE-2018-6952
        "http://git.savannah.gnu.org/cgit/patch. … 68edbed7ee" # Fix memory leaks introduced in CVE-2018-1000165
        "http://git.savannah.gnu.org/cgit/patch. … be04c55727"
        )

And here is prepare():

prepare() {
  cd $pkgname-$pkgver
  # apply patch from the source array (should be a pacman feature)
  local filename
  for filename in "${source[@]}"; do
    if [[ "$filename" =~ \.patch$ ]]; then
      echo "Applying patch ${filename##*/}"
      patch -p1 -N -i "$srcdir/${filename##*/}"
    fi
  done
  :
}

So prepare() appears to be applying CVE-2018-6951 and CVE-2018-6952 because their entries in source end in ".patch" but not the other patches.

My questions are,

1) How are the other patches getting applied?
2) How do I decide which patches need to be applied in prepare() and which patches don't?

loqs

Member

Registered: 2014-03-06

Posts: 18,151

Re: patching in prepare() function

1) How are the other patches getting applied?

They are not.

2) How do I decide which patches need to be applied in prepare() and which patches don't?

If you mean what patches to include then that is your / the package maintainers decision.
If you mean what patches to patch in prepare then all of them unless there is a specific reason otherwise.

Trilby

Inspector Parrot

Registered: 2011-11-29

Posts: 30,330

Website

Re: patching in prepare() function

1) How are the other patches getting applied?

They are not.

This is correct.  But it is sufficiently odd that it is likely a bug.  I suspect it was the PKGBUILD author's intent to apply all of them, but their loop fails to do that.  If the other patches are not intended to be applied, they should be removed from the source array.

(edit: and as this is Allan's package, my hypothesis is not a novel one: Allan Broke It!)
(edit 2: oops, he's the submitter.  All this patching has been done by new maintainers)

Last edited by Trilby (2018-11-09 18:32:44)

---

"UNIX is simple and coherent" - Dennis Ritchie; "GNU's Not Unix" - Richard Stallman

loqs

Member

Registered: 2014-03-06

Posts: 18,151

Re: patching in prepare() function

Just tried applying the three patches that are not currently applied and they all fail to apply cleanly.
Edit:

# Maintainer: Sébastien Luttringer <seblu@archlinux.org>
# Contributor: Allan McRae <allan@archlinux.org>
# Contributor: judd <jvinet@zeroflux.org>

pkgname=patch
pkgver=2.7.6
pkgrel=5
pkgdesc='A utility to apply patch files to original sources'
arch=('x86_64')
url='https://www.gnu.org/software/patch/'
license=('GPL')
groups=('base-devel')
depends=('glibc' 'attr')
makedepends=('ed')
optdepends=('ed: for patch -e functionality')
validpgpkeys=('259B3792B3D6D319212CC4DCD5BF9FEB0313653A') # Andreas Gruenbacher
source=("https://ftp.gnu.org/gnu/$pkgname/$pkgname-$pkgver.tar.xz"{,.sig}
        "https://github.com/mirror/patch/commit/f290f48a621867084884bfff87f8093c15195e6a.patch" # CVE-2018-6951
        "https://github.com/mirror/patch/commit/9c986353e420ead6e706262bf204d6e03322c300.patch" # CVE-2018-6952
        "123eaff0d5d1aebe128295959435b9ca5909c26d.patch" # CVE-2018-1000156
        "19599883ffb6a450d2884f081f8ecf68edbed7ee.patch" # Fix memory leaks introduced in CVE-2018-1000165
        "369dcccdfa6336e5a873d6d63705cfbe04c55727.patch"
        )
md5sums=('78ad9937e4caadcba1526ef1853730d5'
         'SKIP'
         '7e34fc859ccc07b235a8b01b043ff456'
         'aa8ac1e3dccbd523143b01e9f60b06e8'
         '053db340c2f856b8e2cb0c4fecc64a17'
         '7b06474bfbf7863d4af9869fb9bd972c'
         '2a307f4854e2dbe3de3b0a3eb613602b')

prepare() {
  cd $pkgname-$pkgver
  # apply patch from the source array (should be a pacman feature)
  local filename
  for filename in "${source[@]}"; do
    if [[ "$filename" =~ \.patch$ ]]; then
      echo "Applying patch ${filename##*/}"
      patch -p1 -N -i "$srcdir/${filename##*/}"
    fi
  done
  :
}

build() {
  cd $pkgname-$pkgver
  ./configure --prefix=/usr
  make
}

check() {
  cd $pkgname-$pkgver
  make check
}

package() {
  cd $pkgname-$pkgver
  make DESTDIR="$pkgdir" install
}

# vim:set ts=2 sw=2 et:

123eaff0d5d1aebe128295959435b9ca5909c26d.patch

diff --git a/src/pch.c b/src/pch.c
index c8ec983..4fd5a05 100644
--- a/src/pch.c
+++ b/src/pch.c
@@ -33,6 +33,7 @@
 # include <io.h>
 #endif
 #include <safe.h>
+#include <sys/wait.h>
 
 #define INITHUNKMAX 125			/* initial dynamic allocation size */
 
@@ -2114,7 +2115,7 @@ pch_swap (void)
     }
     if (p_efake >= 0) {			/* fix non-freeable ptr range */
 	if (p_efake <= i)
-	    n = p_end - p_ptrn_lines;
+	    n = p_end - i + 1;
 	else
 	    n = -i;
 	p_efake += n;
@@ -2389,22 +2390,28 @@ do_ed_script (char const *inname, char const *outname,
     static char const editor_program[] = EDITOR_PROGRAM;
 
     file_offset beginning_of_this_line;
-    FILE *pipefp = 0;
     size_t chars_read;
+    FILE *tmpfp = 0;
+    char const *tmpname;
+    int tmpfd;
+    pid_t pid;
+
+    if (! dry_run && ! skip_rest_of_patch)
+      {
+	/* Write ed script to a temporary file.  This causes ed to abort on
+	   invalid commands such as when line numbers or ranges exceed the
+	   number of available lines.  When ed reads from a pipe, it rejects
+	   invalid commands and treats the next line as a new command, which
+	   can lead to arbitrary command execution.  */
+
+	tmpfd = make_tempfile (&tmpname, 'e', NULL, O_RDWR | O_BINARY, 0);
+	if (tmpfd == -1)
+	  pfatal ("Can't create temporary file %s", quotearg (tmpname));
+	tmpfp = fdopen (tmpfd, "w+b");
+	if (! tmpfp)
+	  pfatal ("Can't open stream for file %s", quotearg (tmpname));
+      }
 
-    if (! dry_run && ! skip_rest_of_patch) {
-	int exclusive = *outname_needs_removal ? 0 : O_EXCL;
-	assert (! inerrno);
-	*outname_needs_removal = true;
-	copy_file (inname, outname, 0, exclusive, instat.st_mode, true);
-	sprintf (buf, "%s %s%s", editor_program,
-		 verbosity == VERBOSE ? "" : "- ",
-		 outname);
-	fflush (stdout);
-	pipefp = popen(buf, binary_transput ? "wb" : "w");
-	if (!pipefp)
-	  pfatal ("Can't open pipe to %s", quotearg (buf));
-    }
     for (;;) {
 	char ed_command_letter;
 	beginning_of_this_line = file_tell (pfp);
@@ -2415,14 +2422,14 @@ do_ed_script (char const *inname, char const *outname,
 	}
 	ed_command_letter = get_ed_command_letter (buf);
 	if (ed_command_letter) {
-	    if (pipefp)
-		if (! fwrite (buf, sizeof *buf, chars_read, pipefp))
+	    if (tmpfp)
+		if (! fwrite (buf, sizeof *buf, chars_read, tmpfp))
 		    write_fatal ();
 	    if (ed_command_letter != 'd' && ed_command_letter != 's') {
 	        p_pass_comments_through = true;
 		while ((chars_read = get_line ()) != 0) {
-		    if (pipefp)
-			if (! fwrite (buf, sizeof *buf, chars_read, pipefp))
+		    if (tmpfp)
+			if (! fwrite (buf, sizeof *buf, chars_read, tmpfp))
 			    write_fatal ();
 		    if (chars_read == 2  &&  strEQ (buf, ".\n"))
 			break;
@@ -2435,13 +2442,49 @@ do_ed_script (char const *inname, char const *outname,
 	    break;
 	}
     }
-    if (!pipefp)
+    if (!tmpfp)
       return;
-    if (fwrite ("w\nq\n", sizeof (char), (size_t) 4, pipefp) == 0
-	|| fflush (pipefp) != 0)
+    if (fwrite ("w\nq\n", sizeof (char), (size_t) 4, tmpfp) == 0
+	|| fflush (tmpfp) != 0)
       write_fatal ();
-    if (pclose (pipefp) != 0)
-      fatal ("%s FAILED", editor_program);
+
+    if (lseek (tmpfd, 0, SEEK_SET) == -1)
+      pfatal ("Can't rewind to the beginning of file %s", quotearg (tmpname));
+
+    if (! dry_run && ! skip_rest_of_patch) {
+	int exclusive = *outname_needs_removal ? 0 : O_EXCL;
+	*outname_needs_removal = true;
+	if (inerrno != ENOENT)
+	  {
+	    *outname_needs_removal = true;
+	    copy_file (inname, outname, 0, exclusive, instat.st_mode, true);
+	  }
+	sprintf (buf, "%s %s%s", editor_program,
+		 verbosity == VERBOSE ? "" : "- ",
+		 outname);
+	fflush (stdout);
+
+	pid = fork();
+	if (pid == -1)
+	  pfatal ("Can't fork");
+	else if (pid == 0)
+	  {
+	    dup2 (tmpfd, 0);
+	    execl ("/bin/sh", "sh", "-c", buf, (char *) 0);
+	    _exit (2);
+	  }
+	else
+	  {
+	    int wstatus;
+	    if (waitpid (pid, &wstatus, 0) == -1
+	        || ! WIFEXITED (wstatus)
+		|| WEXITSTATUS (wstatus) != 0)
+	      fatal ("%s FAILED", editor_program);
+	  }
+    }
+
+    fclose (tmpfp);
+    safe_unlink (tmpname);
 
     if (ofp)
       {
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 6b6df63..16f8693 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -32,6 +32,7 @@ TESTS = \
 	crlf-handling \
 	dash-o-append \
 	deep-directories \
+	ed-style \
 	empty-files \
 	false-match \
 	fifo \

19599883ffb6a450d2884f081f8ecf68edbed7ee.patch

diff --git a/src/common.h b/src/common.h
index ec50b40..22238b5 100644
--- a/src/common.h
+++ b/src/common.h
@@ -94,10 +94,12 @@ XTERN char const *origsuff;
 XTERN char const * TMPINNAME;
 XTERN char const * TMPOUTNAME;
 XTERN char const * TMPPATNAME;
+XTERN char const * TMPEDNAME;
 
 XTERN bool TMPINNAME_needs_removal;
 XTERN bool TMPOUTNAME_needs_removal;
 XTERN bool TMPPATNAME_needs_removal;
+XTERN bool TMPEDNAME_needs_removal;
 
 #ifdef DEBUGGING
 XTERN int debug;
diff --git a/src/patch.c b/src/patch.c
index 0fe6d72..30fa019 100644
--- a/src/patch.c
+++ b/src/patch.c
@@ -1999,6 +1999,7 @@ cleanup (void)
   remove_if_needed (TMPINNAME, &TMPINNAME_needs_removal);
   remove_if_needed (TMPOUTNAME, &TMPOUTNAME_needs_removal);
   remove_if_needed (TMPPATNAME, &TMPPATNAME_needs_removal);
+  remove_if_needed (TMPEDNAME, &TMPEDNAME_needs_removal);
   remove_if_needed (TMPREJNAME, &TMPREJNAME_needs_removal);
   output_files (NULL);
 }
diff --git a/src/pch.c b/src/pch.c
index 4fd5a05..3223a55 100644
--- a/src/pch.c
+++ b/src/pch.c
@@ -2392,7 +2392,6 @@ do_ed_script (char const *inname, char const *outname,
     file_offset beginning_of_this_line;
     size_t chars_read;
     FILE *tmpfp = 0;
-    char const *tmpname;
     int tmpfd;
     pid_t pid;
 
@@ -2404,12 +2403,13 @@ do_ed_script (char const *inname, char const *outname,
 	   invalid commands and treats the next line as a new command, which
 	   can lead to arbitrary command execution.  */
 
-	tmpfd = make_tempfile (&tmpname, 'e', NULL, O_RDWR | O_BINARY, 0);
+	tmpfd = make_tempfile (&TMPEDNAME, 'e', NULL, O_RDWR | O_BINARY, 0);
 	if (tmpfd == -1)
-	  pfatal ("Can't create temporary file %s", quotearg (tmpname));
+	  pfatal ("Can't create temporary file %s", quotearg (TMPEDNAME));
+	TMPEDNAME_needs_removal = true;
 	tmpfp = fdopen (tmpfd, "w+b");
 	if (! tmpfp)
-	  pfatal ("Can't open stream for file %s", quotearg (tmpname));
+	  pfatal ("Can't open stream for file %s", quotearg (TMPEDNAME));
       }
 
     for (;;) {
@@ -2484,7 +2484,6 @@ do_ed_script (char const *inname, char const *outname,
     }
 
     fclose (tmpfp);
-    safe_unlink (tmpname);
 
     if (ofp)
       {

369dcccdfa6336e5a873d6d63705cfbe04c55727.patch

diff --git a/src/patch.c b/src/patch.c
index 30fa019..026e992 100644
--- a/src/patch.c
+++ b/src/patch.c
@@ -236,6 +236,7 @@ main (int argc, char **argv)
 	    }
 	  remove_if_needed (TMPOUTNAME, &TMPOUTNAME_needs_removal);
 	}
+      remove_if_needed (TMPEDNAME, &TMPEDNAME_needs_removal);
 
       if (! skip_rest_of_patch && ! file_type)
 	{
diff --git a/src/pch.c b/src/pch.c
index 3223a55..f5434b3 100644
--- a/src/pch.c
+++ b/src/pch.c
@@ -2449,7 +2449,7 @@ do_ed_script (char const *inname, char const *outname,
       write_fatal ();
 
     if (lseek (tmpfd, 0, SEEK_SET) == -1)
-      pfatal ("Can't rewind to the beginning of file %s", quotearg (tmpname));
+      pfatal ("Can't rewind to the beginning of file %s", quotearg (TMPEDNAME));
 
     if (! dry_run && ! skip_rest_of_patch) {
 	int exclusive = *outname_needs_removal ? 0 : O_EXCL;
diff --git a/tests/ed-style b/tests/ed-style
new file mode 100644
index 0000000..504e6e5
--- /dev/null
+++ b/tests/ed-style
@@ -0,0 +1,71 @@
+# Copyright (C) 2018 Free Software Foundation, Inc.
+#
+# Copying and distribution of this file, with or without modification,
+# in any medium, are permitted without royalty provided the copyright
+# notice and this notice are preserved.
+
+. $srcdir/test-lib.sh
+
+require cat
+use_local_patch
+use_tmpdir
+
+# ==============================================================
+
+cat > ed1.diff <<EOF
+0a
+foo
+.
+EOF
+
+check 'patch -e foo -i ed1.diff' <<EOF
+EOF
+
+check 'cat foo' <<EOF
+foo
+EOF
+
+cat > ed2.diff <<EOF
+1337a
+r !echo bar
+,p
+EOF
+
+check 'patch -e foo -i ed2.diff > /dev/null 2> /dev/null || echo "Status: $?"' <<EOF
+Status: 2
+EOF
+
+check 'cat foo' <<EOF
+foo
+EOF
+
+# Test the case where one ed-style patch modifies several files
+
+cat > ed3.diff <<EOF
+--- foo
++++ foo
+1c
+bar
+.
+--- baz
++++ baz
+0a
+baz
+.
+EOF
+
+# Apparently we can't create a file with such a patch, while it works fine
+# when the file name is provided on the command line
+cat > baz <<EOF
+EOF
+
+check 'patch -e -i ed3.diff' <<EOF
+EOF
+
+check 'cat foo' <<EOF
+bar
+EOF
+
+check 'cat baz' <<EOF
+baz
+EOF

Last edited by loqs (2018-11-09 20:09:51)

eschwartz

Fellow

Registered: 2014-08-08

Posts: 4,097

Re: patching in prepare() function

Patching in a loop like that is bad style and I seriously recommend against it, for exactly this reason.

Don't use rough, inaccurate heuristics to fail to detect the things you need to do. There is a *reason* that despite the package maintainer claiming

# apply patch from the source array (should be a pacman feature)

we have not added any such feature to pacman! Because it's bad and makes you lazy.

...

Obviously, not all Devs/TUs agree. But given this thread, I'm going to claim I'm right.

---

Managing AUR repos The Right Way -- aurpublish (now a standalone tool)