Re: Bug#385784: aptitude: crash with basic_string::_S_construct NULL not valid [patch]
On Sun, Sep 03, 2006 at 09:50:22PM -0700, Daniel Burrows wrote:
> On Sun, Sep 03, 2006 at 07:41:42PM -0700, Ross Boylan <ross@betterworld.us> was heard to say:
> > In
> > vector<struct DpkgState> &states = PackageOps[pkg];
> > pkg = 0, and is of type char *. It is created by this code further
> > up; note the comments in the original:
> >
> > ------------------------------------
> > /* dpkg sends strings like this:
> > 'status: <pkg>: <pkg qstate>'
> > errors look like this:
> > 'status: /var/cache/apt/archives/krecipes_0.8.1-0ubuntu1_i386.deb : error : trying to overwrite `/usr/share/doc/kde/HTML/en/krecipes/krectip.png', which is also in package krecipes-data
> > and conffile-prompt like this
> > 'status: conffile-prompt: conffile : 'current-conffile' 'new-conffile' useredited distedited
> >
> > */
> > char* list[5];
> > if(!TokSplitString(':', line, list, sizeof(list)/sizeof(list[0])))
> > // FIXME: dpkg sends multiline error messages sometimes (see
> > // #374195 for a example. we should support this by
> > // either patching dpkg to not send multiline over the
> > // statusfd or by rewriting the code here to deal with
> > // it. for now we just ignore it and not crash
> > continue;
> > char *pkg = list[1];
> > ---------------------------------------------------
> >
> > (gdb) p list
> > $2 = {0xafccf9c9 "reinstall it before attempting a removal.", 0x0,
> > 0xafccf9e6 "g a removal.", 0xafccf9ee "val.", 0x0}
> > (gdb) ptype list
> > type = char *[5]
> > (gdb) p line
> > $3 = " reinstall it before attempting a removal.\000\000e is in a very bad inconsistent state - you should", '\0' <repeats 929 times>
> > (gdb) ptype line
> > type = char [1024]
> >
> > So the second line has overwritten the buffer ("line"), which still
> > holds the tail of the first line's message. It looks as if the
> > problem is that 1) the error is multiline and 2) it is not in the
> > multiline format that the work-around dealt with.
It's peculiar that it's not expected, because 374195 reports on the
same "very bad inconsistent state" error I encountered. At a guess,
the different number of characters in the package name is causing
things to line up badly (e.g., the double \000 in line), leading to
the null value.
>
> I believe the problem here is that the code assumes that all strings
> contain a colon character (':'). If a line does *not* contain a colon
> character, the first entry of "list" is set to the whole line, and the
> second entry is set to NULL to mark the end of the split list.
>
My different take was that the if(!TokSplitString...) test was
expecting a false value if there weren't exactly as many tokens as
expected. TokSplitString only returns false if there are too many
fields. Also, note that TokSplitString scribbles \0's into the
original line at the delimeters.
At any rate, I came up with a slightly different patch. With this
patch, the aptitude operation fails to work without crashing. In
other words, it seems to solve the problem. However, I haven't even
verified that aptitude continues to function under normal conditions
with this patch.
--- apt-0.6.45/apt-pkg/deb/dpkgpm.cc 2006-07-25 04:19:00.000000000 -0700
+++ apt-0.6.45rb/apt-pkg/deb/dpkgpm.cc 2006-09-03 21:33:07.000000000 -0700
@@ -632,4 +632,13 @@
// it. for now we just ignore it and not crash
continue;
+
+ /* The previous code only becomes active if there are more than
+ the expected number of : separated fields.
+ I think the next test may match intent better, but it may
+ be too broad, and it may not work in all locales.
+ Ross Boylan 2006-09-03 */
+ if(strncmp(list[0],"status",strlen("status")) != 0)
+ continue;
+
char *pkg = list[1];
char *action = _strstrip(list[2]);
> I've attached a quick and dirty patch to fix this. It would be nice
> if you could confirm that it works before cleaning up your system.
>
> The notes in the referred-to bug indicate that a leading space (" ")
> marks continuation lines. It would be nice to fix apt for this, but
> it's not clear to me that it's possible -- we'd never be able to tell
> the difference between end-of-record (with dpkg waiting for a response)
> and a record where we just haven't seen the next character! I think
> the appropriate thing to do is to fix dpkg to either use an unambiguous
> record separator, or (ideally) to not split lines when writing to the
> status fd. I can't find a dpkg bug about this, has one been filed?
The comments
// #374195 for a example. we should support this by
// either patching dpkg to not send multiline over the
// statusfd or by rewriting the code here to deal with
// it. for now we just ignore it and not crash
seem to indicate some uncertainty about whether changing dpkg's output
was the way to go on this. But your analysis about the need for a
dpkg change sounds reasonable.
> --- dpkgpm.cc.orig 2006-09-03 21:48:47.000000000 -0700
> +++ dpkgpm.cc 2006-09-03 21:49:24.000000000 -0700
> @@ -631,6 +631,11 @@
> // statusfd or by rewriting the code here to deal with
> // it. for now we just ignore it and not crash
> continue;
> + if(list[1] == NULL)
> + // This indicates that the line contained no colon characters
> + // and is probably a multiline error message (so the whole line
> + // is in list[0]). As above, don't crash. -- dnb
> + continue;
> char *pkg = list[1];
> char *action = _strstrip(list[2]);
>
I think getting a NULL may be a special case even of this special case.
Ross
P.S. My concern about localization invalidating some of the tests, if
valid, would affect a lot of tests other than the one I added.
Reply to: