Control: fixed -1 1.1~exp4 Hi, On Mon, Apr 06, 2015 at 03:44:01PM +0200, Tomasz Buchert wrote: > :D Of course! I focused so much on not breaking API & ABI, that I forgot > about it. The problem, which is also why this got a FIXME instead of a fix back then is that you not only have the problem of keeping the ABI which is already hard, but also the API – and the API doesn't indicate that this string is stored in the heap and needs to be freed. In fact, it goes as far as explicitly stating that it doesn't (which is why you were forced to cast so much). That is all fine if all the code accessing and setting it would be under our control, but it isn't. Nothing really stops a libapt user to implement its own acquire items (and e.g. aptitude does this), which very well could set Mode as well (I think aptitude doesn't) which in the best case means leaked memory and in the worst case libapt is trying to free non-heap memory in the items destructor later on resulting in a crash. The apt/experimental version does away with this problem by moving to a std::string under a different name and deprecating Mode – which gets assigned the c_str of the new std::string and hence isn't running out of scope anymore¹ while also keeping the now deprecated Mode working. I have a ¹ here as in an implementation theoretical way the std::string which is stored in Mode isn't running out of scope in many compiler implementations of std::string as they deploy copy-on-write, and we don't change, so the string we us here comes from the global config which is "always" in scope. I know that is a lot of if's and very brittle, but that worked since Aug 2009 for e.g. apts progress reporting (where Mode is used) – even through back then I really had no idea and just got lucky as this line was part of one of my first contributions (and my c++ background was equally thin - not that the later is much better now) … [you can 'easily' verify this by e.g. printing Mode in the item destructor. The output is correct. Now go and append something to the decompProg string before assigning it to Mode and you will notice that the result is unpredictable garbage. All hail to optimization! ;) ] I wonder what is so special about aptdaemon that it has problems now - so can someone please verify that this is really the problem and not just the first thing someone stumbled over while trying to find a culprit (no blame, it would be my first bet, too)? Anyway, if that is really a problem we can fix that in a more compatible way: Instead of assigning the decompProg string, we can go with "decomp" or "unpack" or similar such. Its used only for display proposes anyway and if a user sees a "bzip2 21kB/42kB" or a "decomp 21/kB/42kB"² should not matter much (trivial diff attached). Best regards David Kalnischkies ² 'decomp' mostly because apt has a tendency to use incomprehensible strings here – or does any "normal" user know what "rred" is? Most who see it think its a typo for 'read' after all. ;)
diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 253cbda..a603479 100644
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -1194,8 +1194,10 @@ 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();
+ if (compExt == "uncompressed")
+ Mode = "copy";
+ else
+ Mode = "decomp";
}
/*}}}*/
// AcqIndexTrans::pkgAcqIndexTrans - Constructor /*{{{*/
Attachment:
signature.asc
Description: Digital signature