Re: Bug#385784: aptitude: crash with basic_string::_S_construct NULL not valid [patch]
On Mon, Sep 04, 2006 at 07:25:07AM -0700, Daniel Burrows wrote:
> On Sun, Sep 03, 2006 at 10:25:52PM -0700, Ross Boylan <ross@betterworld.us> was heard to say:
> > 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:
> > > > 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.
>
> \0s are not NULL pointers.
I know.
> TokSplitString replaces delimiters in the
> original string with \0, then writes pointers into the original string into
> list. When it encounters the end of the original string, it writes a NULL
> pointer to indicate the end of the list.
>
> The third parameter to TokSplitString is the length of the list of char*s
> it gets passed. If there isn't enough room in the list for all the tokens
> it found, it clobbers the last token (by writing NULL over it), then returns
> false.
>
> > 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]);
>
> That will still allow crashes if you happen to get a second line
> containing only the word "status". Yes, it's unlikely, but why not
> solve the crash directly and not have to guess?
I think neither of our patches handles all cases. list[1]==NULL (your
patch) may fail if there is a `:' in the line, but the line is not
otherwise in the expected format.
As you wrote, probably the format of messages reported out of dpkg
needs to change for a real solution.
Ross
Reply to: