[Crash-utility] [PATCH] GDB: fix completion related libstdc++ assert

[lijiang]

On Mon, Jan 24, 2022 at 3:09 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab at nec.com>
wrote:

> -----Original Message-----
> >       >       > +--- gdb-10.2/gdb/ada-lang.c.orig
> >       >       > ++++ gdb-10.2/gdb/ada-lang.c
> >       >       > +@@ -997,7 +997,7 @@ ada_fold_name (gdb::string_view
> name)
> >       >       > +   int len = name.size ();
> >       >       > +   GROW_VECT (fold_buffer, fold_buffer_size, len + 1);
> >       >       > +
> >       >       > +-  if (name[0] == '\'')
> >       >       > ++  if (name.size () > 0 && name[0] == '\'')
> >       >       > +     {
> >       >       > +       strncpy (fold_buffer, name.data () + 1, len - 2);
> >       >       > +       fold_buffer[len - 2] = '\000';
> >       >       > +@@ -1006,7 +1006,7 @@ ada_fold_name (gdb::string_view
> name)
> >       >       > +     {
> >       >       > +       int i;
> >       >       > +
> >       >       > +-      for (i = 0; i <= len; i += 1)
> >       >       > ++      for (i = 0; i < len; i++)
> >       >       > +         fold_buffer[i] = tolower (name[i]);
> >       >
> >       >       According to 2ccee230f830 ("Fix off-by-one error in
> ada_fold_name"),
> >       >       please add this:
> >       >
> >       >       +      fold_buffer[i] = '\0';
> >       >
> >       >
> >       >
> >       > No,  the above change will definitely cause a core dump because
> it assigns the value '\0' to a
> > null pointer
> >       > when the name string is null.
> >
> >       Hmm, I'm not familiar with gdb source, could you explain a little
> more?
> >       The following is the function with your patch.
> >
> >       static char *
> >       ada_fold_name (gdb::string_view name)
> >       {
> >         static char *fold_buffer = NULL;
> >         static size_t fold_buffer_size = 0;
> >
> >         int len = name.size ();
> >         GROW_VECT (fold_buffer, fold_buffer_size, len + 1);
> >
> >         if (name.size () > 0 && name[0] == '\'')
> >           {
> >             strncpy (fold_buffer, name.data () + 1, len - 2);
> >             fold_buffer[len - 2] = '\000';
> >           }
> >         else
> >           {
> >             int i;
> >
> >             for (i = 0; i < len; i++)
> >               fold_buffer[i] = tolower (name[i]);
> >           }
> >
> >         return fold_buffer;
> >       }
> >
> >       The GROW_VECT() looks to alloc 1 byte to fold_buffer if
> name.size() is zero.
> >
> >
> >
> > Theoretically yes, but you could notice that the definition of
> 'fold_buffer_size', actually it is a static
> > variable, the ada_fold_name() may be called many times, but variable
> 'fold_buffer_size' is only initialized
> > to zero once, it means that the value of 'fold_buffer_size' may not be
> zero(>1) in the subsequent calls.
> > For this case, the GROW_VECT() will never allocate new memory.
>
> Thanks, but I noticed that the fold_buffer is also a static variable,
> it looks like once it's allocated, never be freed.  So it always has
> 1 byte at least?
>

The above implementation of ada_fold_name() is incomprehensible, I followed
up the latest upstream changes, the implementation has been improved and
simply uses std::string as the variable type('fold_storage'). Furthermore,
crash tools will also have minor changes.  How about the following changes?

static const char *
ada_fold_name (gdb::string_view name)
{
  static std::string fold_storage;

  if (!name.empty () && name[0] == '\'')
    fold_storage = gdb::to_string (name.substr (1, name.size () - 2));
  else
    {
      fold_storage = gdb::to_string (name);
      for (int i = 0; i < name.size (); i += 1)
        fold_storage[i] = tolower (fold_storage[i]);
    }

  return fold_storage.c_str ();
}

Thanks
Lianbo


Thanks,
> Kazu
>
> >
> > See the definition of GROW_VECT():
> >
> > #define GROW_VECT(v, s, m)                                    \
> >    if ((s) < (m)) (v) = (char *) grow_vect (v, &(s), m, sizeof *(v));
> >
> > /* Assuming VECT points to an array of *SIZE objects of size
> >    ELEMENT_SIZE, grow it to contain at least MIN_SIZE objects,
> >    updating *SIZE as necessary and returning the (new) array.  */
> >
> > static void *
> > grow_vect (void *vect, size_t *size, size_t min_size, int element_size)
> > {
> >   if (*size < min_size)
> >     {
> >       *size *= 2;
> >       if (*size < min_size)
> >         *size = min_size;
> >       vect = xrealloc (vect, *size * element_size);
> >     }
> >   return vect;
> > }
> >
> >
> > Hope this helps.
> >
> > Thanks
> > Lianbo
> >
> >
> >       Then len is zero, and nothing is done in the for loop, and
> fold_buffer[i]
> >       (== fold_buffer[0]) can be set '\0', I thought.
> >
> >       >       +      fold_buffer[i] = '\0';
> >
> >       And as far as I've tried, no abort occured.
> >
> >       Thanks,
> >       Kazu
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20220124/b55939b6/attachment.htm>