[Crash-utility] [PATCH 1/2] Change functions in extensions/echo.c to be static

[Liu Tao]

Hello Kazu,

Thanks for reviewing the patch. I agree with your modifications, which
is much better.

Thanks,
Tao Liu

On Fri, Apr 2, 2021 at 8:45 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab at nec.com>
wrote:

> -----Original Message-----
> > If a same extension(Eg: extensions/trace.so) with two different names
> are loaded by
> > "extend" command twice, it sometimes segfaults crash.
> >
> > It's because crash uses RTLD_NOW|RTLD_GLOBAL flags of dlopen to load an
> extension.
> > RTDL_GLOBAL will make symbols defined by this shared object available for
> > symbol resolution of subsequently loaded shared objects. So symbols in
> subsequently
> > loaded extensions are overwritten by the former loaded one with a same
> name.
> > Not only can it cause segfaults, but some other unexpected behaviours.
>
> The phenomenon of the first paragraph is an example and a rare situation if
> not on purpose, please let me change the order and generalize a bit more
> here:
> ---
> The crash utility uses RTLD_NOW|RTLD_GLOBAL flags ... with the same name.
>
> This can cause unexpected behaviors when loading two extension modules that
> have a symbol with the same name.  For example, we can reproduce a
> segmentation
> violation by loading the current trace.so extension module with two
> different
> names.
> ---
>
> I'll edit when applying.  Otherwise, the patch looks good to me.
> Please wait for another ack.
>
> Acked-by: Kazuhito Hagio <k-hagio-ab at nec.com>
>
> Thanks,
> Kazu
>
> >
> > This patch changes functions within extensions/echo.c to be static, and
> documents
> > the issue in code comments, for extensions developers who takes echo.c
> as reference.
> >
> > Signed-off-by: Tao Liu <ltao at redhat.com>
> > ---
> >  extensions/echo.c | 22 ++++++++++++++--------
> >  1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/extensions/echo.c b/extensions/echo.c
> > index ff1e09a..f4bb88f 100644
> > --- a/extensions/echo.c
> > +++ b/extensions/echo.c
> > @@ -17,19 +17,25 @@
> >
> >  #include "defs.h"      /* From the crash source top-level directory */
> >
> > -void echo_init(void);    /* constructor function */
> > -void echo_fini(void);    /* destructor function (optional) */
> > +static void echo_init(void);    /* constructor function */
> > +static void echo_fini(void);    /* destructor function (optional) */
> >
> > -void cmd_echo(void);     /* Declare the commands and their help data. */
> > -char *help_echo[];
> > +static void cmd_echo(void);     /* Declare the commands and their help
> data. */
> > +static char *help_echo[];
> >
> > +/*
> > + * Please making the functions and global variables static within your
> > + * extension if you don't want to make them visiable to subsequently
> > + * loaded extensions. Otherwise, non-static symbols within 2 extensions
> and
> > + * have a same name can cause confliction.
> > + */
> >  static struct command_table_entry command_table[] = {
> >          { "echo", cmd_echo, help_echo, 0},          /* One or more
> commands, */
> >          { NULL },                                     /* terminated by
> NULL, */
> >  };
> >
> >
> > -void __attribute__((constructor))
> > +static void __attribute__((constructor))
> >  echo_init(void) /* Register the command set. */
> >  {
> >          register_extension(command_table);
> > @@ -39,7 +45,7 @@ echo_init(void) /* Register the command set. */
> >   *  This function is called if the shared object is unloaded.
> >   *  If desired, perform any cleanups here.
> >   */
> > -void __attribute__((destructor))
> > +static void __attribute__((destructor))
> >  echo_fini(void) { }
> >
> >
> > @@ -49,7 +55,7 @@ echo_fini(void) { }
> >   *  other crash commands for usage of the myriad of utility routines
> available
> >   *  to accomplish what your task.
> >   */
> > -void
> > +static void
> >  cmd_echo(void)
> >  {
> >          int c;
> > @@ -94,7 +100,7 @@ cmd_echo(void)
> >   *
> >   */
> >
> > -char *help_echo[] = {
> > +static char *help_echo[] = {
> >          "echo",                        /* command name */
> >          "echoes back its arguments",   /* short description */
> >          "arg ...",                     /* argument synopsis, or " " if
> none */
> > --
> > 2.29.2
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20210402/be8b9815/attachment.htm>