On Mon, Jan 24, 2022 at 3:09 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@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
>
>