On Mon, Jan 24, 2022 at 3:09 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx> 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 ();
}
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
>
>
-- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/crash-utility
