On Tue, Jan 25, 2022 at 9:01 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx> wrote:
Hi Lianbo,
-----Original Message-----
> -----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 think that it's not so rare, I also sometimes use a static buffer for a
temporary string in a frequently called function, e.g. in crash-cacheutils [1].
In such cases, a static buffer generally takes better time efficiency rather
than space efficiency. Although [1] uses a static fixed-length buffer, if the
length of input strings was not limited, I might use the style of ada_fold_name().
[1] https://github.com/k-hagio/crash-cacheutils/blob/master/cacheutils.c#L184
-> get_dentry_name()
> 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?
hmm, personally I'd prefer to simply backport only the two small patches
6a780b676637 and 2ccee230f830. The gdb commit 5f9febe0f6ef doesn't fix any
bug, the two patches are enough to fix the issue for now.
Unfortunately it doesn't work, so I have to look into the details of why it failed. Maybe it could be related to the build issues or gdb, or other defects? It's strange.
BTW: Have you reproduced this issue with my flags?
Thanks.
Lianbo
Thanks,
Kazu
-- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/crash-utility
