Use error reporting instead of assert in rred patching

The rest of our code uses return-code errors and it isn't that nice to
crash the rred method on bad patches anyway, so we move to properly
reporting errors in our usual way which incidently also helps writing
a fuzzer for it.

This is not really security relevant though as the patches passed hash
verification while they were downloaded, so an attacker has to overtake a
trusted repository first which gives plenty better options of attack.

1 files changed, 88 insertions, 68 deletions

diff --git a/methods/rred.cc b/methods/rred.cc
index 981364a9e..f53f05ad5 100644
--- a/methods/rred.cc
+++ b/methods/rred.cc
@@ -7,12 +7,15 @@
 
 #include <config.h>
 
+#ifndef APT_EXCLUDE_RRED_METHOD_CODE
 #include "aptmethod.h"
 #include <apt-pkg/configuration.h>
+#include <apt-pkg/init.h>
+#endif
+
 #include <apt-pkg/error.h>
 #include <apt-pkg/fileutl.h>
 #include <apt-pkg/hashes.h>
-#include <apt-pkg/init.h>
 #include <apt-pkg/strutl.h>
 
 #include <apt-private/private-cmndline.h>
@@ -23,7 +26,7 @@
 #include <vector>
 #include <stddef.h>
 
-#include <assert.h>
+#include <cassert>
 #include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -33,7 +36,9 @@
 
 #include <apti18n.h>
 
-#define BLOCK_SIZE (512*1024)
+#ifndef APT_MEMBLOCK_SIZE
+#define APT_MEMBLOCK_SIZE (512*1024)
+#endif
 
 static bool ShowHelp(CommandLine &)
 {
@@ -66,9 +71,9 @@ class MemBlock {
    char *start;
    size_t size;
    char *free;
-   MemBlock *next;
+   MemBlock *next = nullptr;
 
-   explicit MemBlock(size_t size) : size(size), next(NULL)
+   explicit MemBlock(size_t size) : size(size)
    {
       free = start = new char[size];
    }
@@ -77,11 +82,7 @@ class MemBlock {
 
    public:
 
-   MemBlock(void) {
-      free = start = new char[BLOCK_SIZE];
-      size = BLOCK_SIZE;
-      next = NULL;
-   }
+   MemBlock() : MemBlock(APT_MEMBLOCK_SIZE) {}
 
    ~MemBlock() {
       delete [] start;
@@ -100,11 +101,9 @@ class MemBlock {
 	 for (MemBlock *k = this; k; k = k->next) {
 	    if (k->free == last) {
 	       if (len <= k->avail()) {
-		  char *n = k->add(src, len);
-		  assert(last == n);
-		  if (last == n)
-		     return NULL;
-		  return n;
+		  char * const n = k->add(src, len);
+		  assert(last == n); // we checked already that the block is big enough, so a new one shouldn't be used
+		  return (last == n) ? nullptr : n;
 	       } else {
 		  break;
 	       }
@@ -119,10 +118,10 @@ class MemBlock {
    char *add(char *src, size_t len) {
       if (len > avail()) {
 	 if (!next) {
-	    if (len > BLOCK_SIZE)  {
+	    if (len > APT_MEMBLOCK_SIZE)  {
 	       next = new MemBlock(len);
 	    } else {
-	       next = new MemBlock;
+	       next = new MemBlock();
 	    }
 	 }
 	 return next->add(src, len);
@@ -155,24 +154,27 @@ struct Change {
    }
 
    /* actually, don't write <lines> lines from <add> */
-   void skip_lines(size_t lines)
+   bool skip_lines(size_t lines)
    {
       while (lines > 0) {
 	 char *s = (char*) memchr(add, '\n', add_len);
-	 assert(s != NULL);
+	 if (s == nullptr)
+	    return _error->Error("No line left in add_len data to skip (1)");
 	 s++;
 	 add_len -= (s - add);
 	 add_cnt--;
 	 lines--;
 	 if (add_len == 0) {
-	    add = NULL;
-	    assert(add_cnt == 0);
-	    assert(lines == 0);
+	    add = nullptr;
+	    if (add_cnt != 0 || lines != 0)
+	       return _error->Error("No line left in add_len data to skip (2)");
 	 } else {
 	    add = s;
-	    assert(add_cnt > 0);
+	    if (add_cnt == 0)
+	       return _error->Error("No line left in add_len data to skip (3)");
 	 }
       }
+      return true;
    }
 };
 
@@ -183,7 +185,8 @@ class FileChanges {
 
    bool pos_is_okay(void) const
    {
-#ifdef POSDEBUG
+#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
+      // this isn't unsafe, it is just a moderately expensive check we want to avoid normally
       size_t cpos = 0;
       std::list<struct Change>::const_iterator x;
       for (x = changes.begin(); x != where; ++x) {
@@ -208,33 +211,40 @@ class FileChanges {
    std::list<struct Change>::reverse_iterator rbegin(void) { return changes.rbegin(); }
    std::list<struct Change>::reverse_iterator rend(void) { return changes.rend(); }
 
-   void add_change(Change c) {
+   bool add_change(Change c) {
       assert(pos_is_okay());
-      go_to_change_for(c.offset);
-      assert(pos + where->offset == c.offset);
+      if (not go_to_change_for(c.offset) ||
+	  pos + where->offset != c.offset)
+	 return false;
       if (c.del_cnt > 0)
-	 delete_lines(c.del_cnt);
-      assert(pos + where->offset == c.offset);
+	 if (not delete_lines(c.del_cnt))
+	    return false;
+      if (pos + where->offset != c.offset)
+	 return false;
       if (c.add_len > 0) {
 	 assert(pos_is_okay());
 	 if (where->add_len > 0)
-	    new_change();
-	 assert(where->add_len == 0 && where->add_cnt == 0);
+	    if (not new_change())
+	       return false;
+	 if (where->add_len != 0 || where->add_cnt != 0)
+	    return false;
 
 	 where->add_len = c.add_len;
 	 where->add_cnt = c.add_cnt;
 	 where->add = c.add;
       }
       assert(pos_is_okay());
-      merge();
-      assert(pos_is_okay());
+      if (not merge())
+	 return false;
+      return pos_is_okay();
    }
 
    private:
-   void merge(void)
+   bool merge(void)
    {
       while (where->offset == 0 && where != changes.begin()) {
-	 left();
+	 if (not left())
+	    return false;
       }
       std::list<struct Change>::iterator next = where;
       ++next;
@@ -253,53 +263,56 @@ class FileChanges {
 	    ++next;
 	 }
       }
+      return true;
    }
 
-   void go_to_change_for(size_t line)
+   bool go_to_change_for(size_t line)
    {
       while(where != changes.end()) {
 	 if (line < pos) {
-	    left();
+	    if (not left())
+	       return false;
 	    continue;
 	 }
 	 if (pos + where->offset + where->add_cnt <= line) {
-	    right();
+	    if (not right())
+	       return false;
 	    continue;
 	 }
 	 // line is somewhere in this slot
 	 if (line < pos + where->offset) {
 	    break;
 	 } else if (line == pos + where->offset) {
-	    return;
+	    return true;
 	 } else {
-	    split(line - pos);
-	    right();
-	    return;
+	    if (not split(line - pos))
+	       return false;
+	    return right();
 	 }
       }
       /* it goes before this patch */
-      insert(line-pos);
+      return insert(line-pos);
    }
 
-   void new_change(void) { insert(where->offset); }
+   bool new_change(void) { return insert(where->offset); }
 
-   void insert(size_t offset)
+   bool insert(size_t offset)
    {
       assert(pos_is_okay());
-      assert(where == changes.end() || offset <= where->offset);
+      if (where != changes.end() && offset > where->offset)
+	 return false;
       if (where != changes.end())
 	 where->offset -= offset;
       changes.insert(where, Change(offset));
       --where;
-      assert(pos_is_okay());
+      return pos_is_okay();
    }
 
-   void split(size_t offset)
+   bool split(size_t offset)
    {
       assert(pos_is_okay());
-
-      assert(where->offset < offset);
-      assert(offset < where->offset + where->add_cnt);
+      if (where->offset >= offset || offset >= where->offset + where->add_cnt)
+	 return false;
 
       size_t keep_lines = offset - where->offset;
 
@@ -307,27 +320,29 @@ class FileChanges {
 
       where->del_cnt = 0;
       where->offset = 0;
-      where->skip_lines(keep_lines);
+      if (not where->skip_lines(keep_lines))
+	 return false;
 
       before.add_cnt = keep_lines;
       before.add_len -= where->add_len;
 
       changes.insert(where, before);
       --where;
-      assert(pos_is_okay());
+      return pos_is_okay();
    }
 
-   void delete_lines(size_t cnt)
+   bool delete_lines(size_t cnt)
    {
-      std::list<struct Change>::iterator x = where;
       assert(pos_is_okay());
+      std::list<struct Change>::iterator x = where;
       while (cnt > 0)
       {
 	 size_t del;
 	 del = x->add_cnt;
 	 if (del > cnt)
 	    del = cnt;
-	 x->skip_lines(del);
+	 if (not x->skip_lines(del))
+	    return false;
 	 cnt -= del;
 
 	 ++x;
@@ -342,21 +357,21 @@ class FileChanges {
 	 where->del_cnt += del;
 	 cnt -= del;
       }
-      assert(pos_is_okay());
+      return pos_is_okay();
    }
 
-   void left(void) {
+   bool left(void) {
       assert(pos_is_okay());
       --where;
       pos -= where->offset + where->add_cnt;
-      assert(pos_is_okay());
+      return pos_is_okay();
    }
 
-   void right(void) {
+   bool right(void) {
       assert(pos_is_okay());
       pos += where->offset + where->add_cnt;
       ++where;
-      assert(pos_is_okay());
+      return pos_is_okay();
    }
 };
 
@@ -378,7 +393,7 @@ class Patch {
    static void dump_rest(FileFd &o, FileFd &i,
 	 Hashes * const start_hash, Hashes * const end_hash)
    {
-      char buffer[BLOCK_SIZE];
+      char buffer[APT_MEMBLOCK_SIZE];
       unsigned long long l = 0;
       while (i.Read(buffer, sizeof(buffer), &l)) {
 	 if (l ==0  || !retry_fwrite(buffer, l, o, start_hash, end_hash))
@@ -389,7 +404,7 @@ class Patch {
    static void dump_lines(FileFd &o, FileFd &i, size_t n,
 	 Hashes * const start_hash, Hashes * const end_hash)
    {
-      char buffer[BLOCK_SIZE];
+      char buffer[APT_MEMBLOCK_SIZE];
       while (n > 0) {
 	 if (i.ReadLine(buffer, sizeof(buffer)) == NULL)
 	    buffer[0] = '\0';
@@ -402,7 +417,7 @@ class Patch {
 
    static void skip_lines(FileFd &i, int n, Hashes * const start_hash)
    {
-      char buffer[BLOCK_SIZE];
+      char buffer[APT_MEMBLOCK_SIZE];
       while (n > 0) {
 	 if (i.ReadLine(buffer, sizeof(buffer)) == NULL)
 	    buffer[0] = '\0';
@@ -422,7 +437,7 @@ class Patch {
 
    bool read_diff(FileFd &f, Hashes * const h)
    {
-      char buffer[BLOCK_SIZE];
+      char buffer[APT_MEMBLOCK_SIZE];
       bool cmdwanted = true;
 
       Change ch(std::numeric_limits<size_t>::max());
@@ -478,7 +493,8 @@ class Patch {
 		  ch.add = NULL;
 		  ch.add_cnt = 0;
 		  ch.add_len = 0;
-		  filechanges.add_change(ch);
+		  if (not filechanges.add_change(ch))
+		     return _error->Error("Parsing patchfile %s failed: Delete command could not be added to changes", f.Name().c_str());
 		  break;
 	       default:
 		  return _error->Error("Parsing patchfile %s failed: Unknown command", f.Name().c_str());
@@ -486,7 +502,8 @@ class Patch {
 	 } else { /* !cmdwanted */
 	    if (strcmp(buffer, ".\n") == 0) {
 	       cmdwanted = true;
-	       filechanges.add_change(ch);
+	       if (not filechanges.add_change(ch))
+		  return _error->Error("Parsing patchfile %s failed: Data couldn't be added for command (1)", f.Name().c_str());
 	    } else {
 	       char *last = NULL;
 	       char *add;
@@ -500,7 +517,8 @@ class Patch {
 		  ch.add_cnt++;
 	       } else {
 		  if (ch.add) {
-		     filechanges.add_change(ch);
+		     if (not filechanges.add_change(ch))
+			return _error->Error("Parsing patchfile %s failed: Data couldn't be added for command (2)", f.Name().c_str());
 		     ch.del_cnt = 0;
 		  }
 		  ch.offset += ch.add_cnt;
@@ -575,6 +593,7 @@ class Patch {
    }
 };
 
+#ifndef APT_EXCLUDE_RRED_METHOD_CODE
 class RredMethod : public aptMethod {
    private:
       bool Debug;
@@ -859,3 +878,4 @@ int main(int argc, const char *argv[])
    input.Close();
    return DispatchCommandLine(CmdL, {});
 }
+#endif