On Tue, Jan 25, 2022 at 9:01 AM HAGIO KAZUHITO(萩尾 一仁) <[email protected]>
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
[email protected]
https://listman.redhat.com/mailman/listinfo/crash-utility
