Bug#781858: apt: dangling pointer crash

[Tomasz Buchert <tomasz@debian.org>]

On 06/04/15 14:04, Chris Bainbridge wrote:
> [...]
>
> In which case using strdup to create a new string for each assignment
> will leak memory?

:D Of course! I focused so much on not breaking ABI & API that I forgot
about this... Another try is attached.

Tomasz

From c45e7f6a2065a5127dcff9f9b5f12db95d9437a3 Mon Sep 17 00:00:00 2001
From: Tomasz Buchert <tomasz@debian.org>
Date: Mon, 6 Apr 2015 15:38:13 +0200
Subject: [PATCH] Fix Mode use

---
 apt-pkg/acquire-item.cc | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 253cbda..2ff088d 100644
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -50,6 +50,26 @@
 
 using namespace std;
 
+// xstrdup - Duplicate a C string					/*{{{*/
+// ---------------------------------------------------------------------
+/* Makes a copy of str and stores it in a pointer pointed by pdst.
+   The previous value (if exists) is freed. If src is NULL then
+   the previous value is only freed (so it works like free on *pdst). */
+static void xstrdup(const char **pdst, const char *src)
+{
+   const char *dst = *pdst;
+   if (dst)
+      free((void *)dst);
+   dst = NULL;
+   if (src) {
+      dst = strdup(src);
+      if (!dst)
+         abort();
+   }
+   *pdst = dst;
+}
+
+									/*}}}*/
 // Acquire::Item::Item - Constructor					/*{{{*/
 // ---------------------------------------------------------------------
 /* */
@@ -66,6 +86,7 @@ pkgAcquire::Item::Item(pkgAcquire *Owner) : Owner(Owner), FileSize(0),
 /* */
 pkgAcquire::Item::~Item()
 {
+   xstrdup(&Mode, NULL);
    Owner->Remove(this);
 }
 									/*}}}*/
@@ -756,7 +777,7 @@ void pkgAcqIndexDiffs::Done(string Message,unsigned long long Size,string Md5Has
       Local = true;
       Desc.URI = "rred:" + FinalFile;
       QueueURI(Desc);
-      Mode = "rred";
+      xstrdup(&Mode, "rred");
       return;
    } 
 
@@ -874,7 +895,7 @@ void pkgAcqIndexMergeDiffs::Done(string Message,unsigned long long Size,string M
       Local = true;
       Desc.URI = "rred:" + FinalFile;
       QueueURI(Desc);
-      Mode = "rred";
+      xstrdup(&Mode, "rred");
       return;
    }
    // success in download/apply all diffs, clean up
@@ -1128,7 +1149,7 @@ void pkgAcqIndex::Done(string Message,unsigned long long Size,string Hash,
       DestFile += ".decomp";
       Desc.URI = "copy:" + FileName;
       QueueURI(Desc);
-      Mode = "copy";
+      xstrdup(&Mode, "copy");
       return;
    }
 
@@ -1194,8 +1215,7 @@ void pkgAcqIndex::Done(string Message,unsigned long long Size,string Hash,
    Desc.URI = decompProg + ":" + FileName;
    QueueURI(Desc);
 
-   // FIXME: this points to a c++ string that goes out of scope
-   Mode = decompProg.c_str();
+   xstrdup(&Mode, decompProg.c_str());
 }
 									/*}}}*/
 // AcqIndexTrans::pkgAcqIndexTrans - Constructor			/*{{{*/
@@ -1470,7 +1490,7 @@ void pkgAcqMetaIndex::Done(string Message,unsigned long long Size,string Hash,	/
          AuthPass = true;
          Desc.URI = "gpgv:" + SigFile;
          QueueURI(Desc);
-         Mode = "gpgv";
+         xstrdup(&Mode, "gpgv");
 	 return;
       }
    }
-- 
2.1.4

Attachment: signature.asc
Description: Digital signature