]> git.saurik.com Git - apt.git/commitdiff
add more parsing error checking for rred
authorDavid Kalnischkies <david@kalnischkies.de>
Sun, 7 Jun 2015 00:17:15 +0000 (02:17 +0200)
committerDavid Kalnischkies <david@kalnischkies.de>
Tue, 9 Jun 2015 10:57:36 +0000 (12:57 +0200)
The rred parser is very accepting regarding 'invalid' files. Given that
we can't trust the input it might be a bit too relaxed. In any case,
checking for more errors can't hurt given that we support only a very
specific subset of ed commands.

methods/rred.cc
test/integration/test-method-rred [new file with mode: 0755]
test/integration/test-pdiff-usage

index 3da33c1267ba6414cc0e1dfafdfb8018c7d7f932..81ecf855318610a33a42089034901d8f7fe2b320 100644 (file)
@@ -21,6 +21,7 @@
 #include <vector>
 
 #include <assert.h>
+#include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -35,7 +36,7 @@ class MemBlock {
    char *start;
    size_t size;
    char *free;
-   struct MemBlock *next;
+   MemBlock *next;
 
    MemBlock(size_t size) : size(size), next(NULL)
    {
@@ -116,7 +117,7 @@ struct Change {
    size_t add_len; /* bytes */
    char *add;
 
-   Change(int off)
+   Change(size_t off)
    {
       offset = off;
       del_cnt = add_cnt = add_len = 0;
@@ -388,30 +389,37 @@ class Patch {
 
    public:
 
-   void read_diff(FileFd &f, Hashes * const h)
+   bool read_diff(FileFd &f, Hashes * const h)
    {
       char buffer[BLOCK_SIZE];
       bool cmdwanted = true;
 
-      Change ch(0);
-      while(f.ReadLine(buffer, sizeof(buffer)))
-      {
+      Change ch(std::numeric_limits<size_t>::max());
+      if (f.ReadLine(buffer, sizeof(buffer)) == NULL)
+        return _error->Error("Reading first line of patchfile %s failed", f.Name().c_str());
+      do {
         if (h != NULL)
            h->Add(buffer);
         if (cmdwanted) {
            char *m, *c;
            size_t s, e;
-           s = strtol(buffer, &m, 10);
-           if (m == buffer) {
-              s = e = ch.offset + ch.add_cnt;
-              c = buffer;
-           else if (*m == ',') {
-              m++;
+           errno = 0;
+           s = strtoul(buffer, &m, 10);
+           if (unlikely(m == buffer || s == ULONG_MAX || errno != 0))
+              return _error->Error("Parsing patchfile %s failed: Expected an effected line start", f.Name().c_str());
+           else if (*m == ',') {
+              ++m;
               e = strtol(m, &c, 10);
+              if (unlikely(m == c || e == ULONG_MAX || errno != 0))
+                 return _error->Error("Parsing patchfile %s failed: Expected an effected line end", f.Name().c_str());
+              if (unlikely(e < s))
+                 return _error->Error("Parsing patchfile %s failed: Effected lines end %lu is before start %lu", f.Name().c_str(), e, s);
            } else {
               e = s;
               c = m;
            }
+           if (s > ch.offset)
+              return _error->Error("Parsing patchfile %s failed: Effected line is after previous effected line", f.Name().c_str());
            switch(*c) {
               case 'a':
                  cmdwanted = false;
@@ -422,6 +430,8 @@ class Patch {
                  ch.del_cnt = 0;
                  break;
               case 'c':
+                 if (unlikely(s == 0))
+                    return _error->Error("Parsing patchfile %s failed: Change command can't effect line zero", f.Name().c_str());
                  cmdwanted = false;
                  ch.add = NULL;
                  ch.add_cnt = 0;
@@ -430,6 +440,8 @@ class Patch {
                  ch.del_cnt = e - s + 1;
                  break;
               case 'd':
+                 if (unlikely(s == 0))
+                    return _error->Error("Parsing patchfile %s failed: Delete command can't effect line zero", f.Name().c_str());
                  ch.offset = s - 1;
                  ch.del_cnt = e - s + 1;
                  ch.add = NULL;
@@ -437,9 +449,11 @@ class Patch {
                  ch.add_len = 0;
                  filechanges.add_change(ch);
                  break;
+              default:
+                 return _error->Error("Parsing patchfile %s failed: Unknown command", f.Name().c_str());
            }
         } else { /* !cmdwanted */
-           if (buffer[0] == '.' && buffer[1] == '\n') {
+           if (strcmp(buffer, ".\n") == 0) {
               cmdwanted = true;
               filechanges.add_change(ch);
            } else {
@@ -465,7 +479,8 @@ class Patch {
               }
            }
         }
-      }
+      } while(f.ReadLine(buffer, sizeof(buffer)));
+      return true;
    }
 
    void write_diff(FILE *f)
@@ -601,14 +616,14 @@ class RredMethod : public pkgAcqMethod {
                  << std::endl;
 
            FileFd p;
+           Hashes patch_hash(I->ExpectedHashes);
            // all patches are compressed, even if the name doesn't reflect it
-           if (p.Open(patch_name, FileFd::ReadOnly, FileFd::Gzip) == false) {
-              std::cerr << "Could not open patch file " << patch_name << std::endl;
+           if (p.Open(patch_name, FileFd::ReadOnly, FileFd::Gzip) == false ||
+                 patch.read_diff(p, &patch_hash) == false)
+           {
               _error->DumpErrors(std::cerr);
-              abort();
+              return false;
            }
-           Hashes patch_hash(I->ExpectedHashes);
-           patch.read_diff(p, &patch_hash);
            p.Close();
            HashStringList const hsl = patch_hash.GetHashStringList();
            if (hsl != I->ExpectedHashes)
@@ -624,7 +639,6 @@ class RredMethod : public pkgAcqMethod {
         FILE *out = fopen(Itm->DestFile.c_str(), "w");
 
         Hashes hash(Itm->ExpectedHashes);
-
         patch.apply_against_file(out, inp, &hash);
 
         fclose(out);
@@ -657,6 +671,16 @@ class RredMethod : public pkgAcqMethod {
         return true;
       }
 
+      bool Configuration(std::string Message)
+      {
+        if (pkgAcqMethod::Configuration(Message) == false)
+           return false;
+
+        DropPrivsOrDie();
+
+        return true;
+      }
+
    public:
       RredMethod() : pkgAcqMethod("2.0",SingleInstance | SendConfig), Debug(false) {}
 };
@@ -685,7 +709,11 @@ int main(int argc, char **argv)
         _error->DumpErrors(std::cerr);
         exit(1);
       }
-      patch.read_diff(p, NULL);
+      if (patch.read_diff(p, NULL) == false)
+      {
+        _error->DumpErrors(std::cerr);
+        exit(2);
+      }
    }
 
    if (just_diff) {
diff --git a/test/integration/test-method-rred b/test/integration/test-method-rred
new file mode 100755 (executable)
index 0000000..a8de3ea
--- /dev/null
@@ -0,0 +1,194 @@
+#!/bin/sh
+set -e
+
+TESTDIR=$(readlink -f $(dirname $0))
+. $TESTDIR/framework
+
+setupenvironment
+configarchitecture 'i386'
+
+echo 'Package: coolstuff
+Version: 0.8.15
+Description: collection of stuff
+ A lot, too much to iterate all, but at least this:
+ - stuff
+ - more stuff
+ - even more stuff
+ .
+ And a cow.
+
+Package: oldstuff
+Version: 0-1
+Description: collection of outdated stuff
+ A lot, but of no use nowadays, but at least this:
+ - stuff
+ - more stuff
+ - even more stuff
+ .
+ And a dog.' > Packages
+
+testrred() {
+       msgtest "$1" "$2"
+       if [ -z "$3" ]; then
+               echo -n '' > Packages.ed
+       else
+               echo "$3" > Packages.ed
+       fi
+       rred() {
+               cat Packages | runapt "${METHODSDIR}/rred" "$@"
+       }
+       testsuccessequal "$4" --nomsg rred -f Packages.ed
+}
+
+testrred 'Remove' 'first line' '1d' "$(tail -n +2 ./Packages)"
+testrred 'Remove' 'empty line' '10d' "$(head -n 9 ./Packages)
+$(tail -n 9 ./Packages)"
+testrred 'Remove' 'line in a paragraph' '5d' "$(head -n 4 ./Packages)
+$(tail -n 14 ./Packages)"
+testrred 'Remove' 'last line' '19d' "$(head -n -1 ./Packages)"
+testrred 'Remove' 'multiple single lines' '17d
+7d' "$(sed -e '/^ - even more stuff$/ d' ./Packages)"
+testrred 'Remove' 'first paragraph' '1,10d' "$(tail -n 9 ./Packages)"
+testrred 'Remove' 'a few lines in the middle' '5,14d' "$(head -n 4 ./Packages)
+$(tail -n 5 ./Packages)"
+testrred 'Remove' 'second paragraph' '10,19d' "$(head -n 9 ./Packages)"
+testrred 'Mass Remove' 'all stuff lines' '15,17d
+13d
+11d
+5,7d
+3d
+1d' "$(sed '/stuff/ d' ./Packages)"
+
+testrred 'Single line add' 'first line' '0a
+Format: 3.0 (native)
+.' "Format: 3.0 (native)
+$(cat ./Packages)"
+testrred 'Single line add' 'last line' '19a
+Multi-Arch: foreign
+.' "$(cat ./Packages)
+Multi-Arch: foreign"
+testrred 'Single line add' 'middle' '9a
+Multi-Arch: foreign
+.' "$(head -n 9 ./Packages)
+Multi-Arch: foreign
+$(tail -n 10 ./Packages)"
+
+testrred 'Multi line add' 'first line' '0a
+Format: 3.0 (native)
+Source: apt
+.' "Format: 3.0 (native)
+Source: apt
+$(cat ./Packages)"
+testrred 'Multi line add' 'last line' '19a
+Multi-Arch: foreign
+Homepage: https://debian.org
+.' "$(cat ./Packages)
+Multi-Arch: foreign
+Homepage: https://debian.org"
+testrred 'Multi line add' 'middle' '9a
+Multi-Arch: foreign
+Homepage: https://debian.org
+.' "$(head -n 9 ./Packages)
+Multi-Arch: foreign
+Homepage: https://debian.org
+$(tail -n 10 ./Packages)"
+
+testrred 'Single line change' 'first line' '1c
+Package: supercoolstuff
+.' "Package: supercoolstuff
+$(tail -n +2 ./Packages)"
+testrred 'Single line change' 'in the middle' '9c
+ And a super cow.
+.' "$(head -n 8 ./Packages)
+ And a super cow.
+$(tail -n 10 ./Packages)"
+testrred 'Single line change' 'an empty line' '10c
+
+.' "$(head -n 9 ./Packages)
+
+$(tail -n 9 ./Packages)"
+testrred 'Single line change' 'a spacy line' '10c
+         
+.' "$(head -n 9 ./Packages)
+         
+$(tail -n 9 ./Packages)"
+testrred 'Single line change' 'last line' '19c
+ And a cat.
+.' "$(head -n -1 ./Packages)
+ And a cat."
+
+testrred 'Multi line change' 'exchange' '5,7c
+ - good stuff
+ - more good stuff
+ - even more good stuff
+.' "$(head -n 4 ./Packages)
+ - good stuff
+ - more good stuff
+ - even more good stuff
+$(tail -n 12 ./Packages)"
+testrred 'Multi line change' 'less' '5,7c
+ - good stuff
+ - more good stuff
+.' "$(head -n 4 ./Packages)
+ - good stuff
+ - more good stuff
+$(tail -n 12 ./Packages)"
+testrred 'Multi line change' 'more' '5,7c
+ - good stuff
+ - more good stuff
+ - even more good stuff
+ - bonus good stuff
+.' "$(head -n 4 ./Packages)
+ - good stuff
+ - more good stuff
+ - even more good stuff
+ - bonus good stuff
+$(tail -n 12 ./Packages)"
+
+failrred() {
+       msgtest 'Failure caused by' "$1"
+       echo "$2" > Packages.ed
+       rred() {
+               cat Packages | runapt "${METHODSDIR}/rred" "$@"
+       }
+       testfailure --nomsg rred -f Packages.ed
+}
+
+failrred 'Bogus content' '<html>
+</html>'
+
+# not a problem per-se, but we want our parser to be really strict
+failrred 'Empty patch file' ''
+failrred 'Empty line patch file' '
+'
+failrred 'Empty line before command' '
+1d'
+failrred 'Empty line after command' '1d
+'
+failrred 'Empty line between commands' '17d
+
+7d'
+failrred 'Empty spaces lines before command' '            
+1d'
+failrred 'Empty spaces lines after command' '1d
+          '
+failrred 'Empty spaces lines between commands' '17d
+          
+7d'
+
+# the line before the first one can't be deleted/changed
+failrred 'zero line delete' '0d'
+failrred 'zero line change' '0c
+Package: supercoolstuff
+.'
+# and this makes no sense at all
+failrred 'negative line delete' '-1d'
+failrred 'negative line change' '-1c
+Package: supercoolstuff
+.'
+failrred 'negative line add' '-1a
+Package: supercoolstuff
+.'
+failrred 'Wrong order of commands' '7d
+17d'
+failrred 'End before start' '7,6d'
index 73df61895d5a802a01a8024f708aa14d8183fd5e..7a9f6496b5c8beda81363b02acce08661a03ae84 100755 (executable)
@@ -165,7 +165,8 @@ SHA256-History:
 SHA256-Patches:
  e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 19722 2010-08-18-2013.28
  $(sha256sum $PATCHFILE | cut -d' ' -f 1) $(stat -c%s $PATCHFILE) $(basename $PATCHFILE)" > $PATCHINDEX
-       echo 'I am Mallory and I change files' >> $PATCHFILE
+       # needs to look like a valid command, otherwise the parser will fail before hashes are checked
+       echo '1d' >> $PATCHFILE
        cat $PATCHFILE | gzip > ${PATCHFILE}.gz
        generatereleasefiles '+1hour'
        signreleasefiles