[Crash-utility] Crash-utility Digest, Vol 179, Issue 9

[Mathias Krause]

Am 11.08.20 um 15:21 schrieb lijiang:
>>> [...]
> 
> I just know the details of the offset_table from Dave Anderson. The above changes will
> break the extension modules that have been previously compiled, because the OFFSET()
> values will be different.
> 
> Would you mind correcting it and putting the new entries at end of the offset_table?

Sure, will do. I wasn't aware of the conflict with already compiled
extensions. Thanks for making me aware of that!

Maybe we can try to "version" the offset structures so existing
extensions won't simply use wrong offsets but error out.

Something like this, maybe? (untested, not even compiled):

| diff --git a/defs.h b/defs.h
| index d7adb23b86d5..8a4f68a7a5ee 100644
| --- a/defs.h
| +++ b/defs.h
| @@ -2312,7 +2312,7 @@ struct array_table {
|   *  The following set of macros can only be used with pre-intialized
fields
|   *  in the offset table, size table or array_table.
|   */
| -#define OFFSET(X)         (OFFSET_verify(offset_table.X, (char
*)__FUNCTION__, __FILE__, __LINE__, #X))
| +#define OFFSET(X)         (OFFSET_verify(offset_table.X, (char
*)__FUNCTION__, __FILE__, __LINE__, #X, sizeof(struct offset_table)))
|  #define SIZE(X)            (SIZE_verify(size_table.X, (char
*)__FUNCTION__, __FILE__, __LINE__, #X))
|  #define INVALID_OFFSET     (-1)
|  #define INVALID_MEMBER(X)  (offset_table.X == INVALID_OFFSET)
| @@ -5220,7 +5220,7 @@ void clear_text_value_cache(void);
|  void dump_numargs_cache(void);
|  int patch_kernel_symbol(struct gnu_request *);
|  struct syment *generic_machdep_value_to_symbol(ulong, ulong *);
| -long OFFSET_verify(long, char *, char *, int, char *);
| +long OFFSET_verify(long, char *, char *, int, char *, size_t);
|  long SIZE_verify(long, char *, char *, int, char *);
|  long OFFSET_option(long, long, char *, char *, int, char *, char *);
|  long SIZE_option(long, long, char *, char *, int, char *, char *);
| diff --git a/symbols.c b/symbols.c
| index 3b1f08af43ff..3d0d0032ab85 100644
| --- a/symbols.c
| +++ b/symbols.c
| @@ -13069,18 +13069,26 @@ SIZE_option(long size1, long size2, char
*func, char *file, int line,
|   *  to turn it off instead?
|   */
|  long
| -OFFSET_verify(long offset, char *func, char *file, int line, char *item)
| +OFFSET_verify(long offset, char *func, char *file, int line, char *item,
| +              size_t table_size)
|  {
|         char errmsg[BUFSIZE];
|
| +       if (table_size != sizeof(struct offset_table)) {
| +               sprintf(errmsg, "offset table size mismatch: used %zu,
but is %zu",
| +                       table_size, sizeof(struct offset_table));
| +               goto err;
| +       }
| +
|         if (!(pc->flags & DATADEBUG))
|                 return offset;
|
|         if (offset < 0) {
|                 void *retaddr[NUMBER_STACKFRAMES] = { 0 };
| -               SAVE_RETURN_ADDRESS(retaddr);
|                 sprintf(errmsg, "invalid structure member offset: %s",
|                         item);
| +err:
| +               SAVE_RETURN_ADDRESS(retaddr);
|                 datatype_error(retaddr, errmsg, func, file, line);
|         }
|         return offset;

We would need to extend it to cover all the other tables too, like
size_table, array_table, kernel_table, etc. We just need to find proper
places to hook these checks.

Another sensible place might be register_extension() as that gets called
during extension initialization. We would need make it a macro so it'll
catch sizes that are used for compiling the actual extension. We then
need to verify they match with the sizes used by crash. Something like
this maybe -- again, completely untested:

| diff --git a/defs.h b/defs.h
| index d7adb23b86d5..0f87d75a8e76 100644
| --- a/defs.h
| +++ b/defs.h
| @@ -2283,6 +2283,13 @@ struct array_table {
|         int vm_numa_stat;
|  };
|
| +struct table_sizes {
| +       size_t offset_table;
| +       size_t size_table;
| +       size_t array_table;
| +       //...
| +};
| +
|  /*
|   *  The following set of macros use gdb to determine structure, union,
|   *  or member sizes/offsets.  They should be used only during
initialization
| @@ -5544,7 +5551,17 @@ void check_stack_overflow(void);
|  /*
|   *  extensions.c
|   */
| -void register_extension(struct command_table_entry *);
| +#define register_extension(cte) \
| +       do { \
| +               static const struct table_sizes ts = { \
| +                       .offset_table = sizeof(struct offset_table), \
| +                       .size_table = sizeof(struct size_table), \
| +                       .array_table = sizeof(struct array_table), \
| +               }; \
| +               _register_extension(cte, &ts); \
| +       } while (0)
| +
| +void _register_extension(struct command_table_entry *, const struct
table_sizes *);
|  void dump_extension_table(int);
|  void load_extension(char *);
|  void unload_extension(char *);
| diff --git a/extensions.c b/extensions.c
| index d23b1e3cc26f..e64c2541010a 100644
| --- a/extensions.c
| +++ b/extensions.c
| @@ -545,6 +545,17 @@ unload_extension(char *lib)
|                 error(INFO, "%s: not loaded\n", lib);
|  }
|
| +static void verify_table_sizes(const struct table_sizes *tab_sizes)
| +{
| +       if (tab_sizes == NULL)
| +               return;
| +
| +       if (tab_sizes->offset_table != sizeof(struct offset_table))
| +               // error out
| +
| +       // more size tests...
| +}
| +
|  /*
|   *  Register the command_table as long as there are no command namespace
|   *  clashes with the currently-existing command set.  Also delete any
aliases
| @@ -556,10 +567,13 @@ unload_extension(char *lib)
|   *  REGISTERED bit in the "current" extension_table structure flags.
|   */
|  void
| -register_extension(struct command_table_entry *command_table)
| +_register_extension(struct command_table_entry *command_table,
| +               const struct table_sizes *tab_sizes)
|  {
|         struct command_table_entry *cp;
|
| +       verify_table_sizes(tab_sizes);
| +
|         pc->curext->flags |= NO_MINIMAL_COMMANDS;
|
|          for (cp = command_table; cp->name; cp++) {
| @@ -604,6 +618,14 @@ register_extension(struct command_table_entry
*command_table)
|         pc->curext->flags |= REGISTERED;             /* Mark of
approval */
|  }
|
| +/* for backwards compatibility with existing extensions */
| +#undef register_extension
| +void
| +register_extension(struct command_table_entry *command_table)
| +{
| +       _register_extension(command_table, NULL);
| +}
| +
|  /*
|   *  Hooks for sial.
|   */

But, maybe, this is all just band aid around real versioning of the data
structures. We can simply have a global define that is the version
number of crash's current definition of the extensions ABI. If we change
some data structures, we just increment that number, making all existing
extensions fail -- for good! Recompiling extensions should be required
if the ABI changes, so trying to make it /maybe/ backwards compatible is
just calling for trouble, IMHO. We should really error out in this case
and not rely on developers to remember and keep the ordering.

What do you think?

> In addition, can you help to add the new entries to the dump_offset_table()? 

Sure. Will send a v2 with the minimal changes (adding new members to the
end and to dump_offset_table()) next week.

> Sorry for this.

No worries, that's what reviews are made for!


Thanks,
Mathias