[OpenWrt-Devel] [PATCH uci 2/2] build: Add -Wclobbered to detect problems with longjmp

[Yousong Zhou]

Hi Hauke

On Sat, 2 Nov 2019 at 00:07, Hauke Mehrtens <hauke at hauke-m.de> wrote:
>
> When we jump back to a save point in UCI_THROW() with longjmp all the
> registers will be reset to the old values when we called UCI_TRAP_SAVE()
> last time, but the memory is not restored. This will revert all the
> variables which are stored in registers, but not the variables stored on
> the stack.
>
> Mark all the variables which the compiler could put into a register as
> volatile to store them safely on the stack and make sure they have the
> defined current values also after longjmp was called.
>
> This also activates a compiler warning which should warn us in such
> cases.
> This could fix some potential problem in error paths like the one
> reported in CVE-2019-15513.
>
> Signed-off-by: Hauke Mehrtens <hauke at hauke-m.de>

Not sure I understand the internals right.  It seems to me a few
changes below may not be necessary.  The -Wclobber check can produce
false-positives right?  Are these changes made mainly for "better safe
than regret"?

Regards,
                yousong

> ---
>  CMakeLists.txt |  2 +-
>  delta.c        | 20 ++++++++++----------
>  file.c         | 11 ++++++-----
>  list.c         |  4 ++--
>  4 files changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 170eb0b..578c021 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -3,7 +3,7 @@ cmake_minimum_required(VERSION 2.6)
>  PROJECT(uci C)
>
>  SET(CMAKE_SHARED_LIBRARY_LINK_C_FLAGS "")
> -ADD_DEFINITIONS(-Os -Wall -Werror --std=gnu99 -g3 -I. -DUCI_PREFIX="${CMAKE_INSTALL_PREFIX}")
> +ADD_DEFINITIONS(-Os -Wall -Werror -Wclobbered --std=gnu99 -g3 -I. -DUCI_PREFIX="${CMAKE_INSTALL_PREFIX}")
>
>  OPTION(UCI_DEBUG "debugging support" OFF)
>  OPTION(UCI_DEBUG_TYPECAST "typecast debugging support" OFF)
> diff --git a/delta.c b/delta.c
> index 386167d..52ebe3b 100644
> --- a/delta.c
> +++ b/delta.c
> @@ -100,7 +100,7 @@ int uci_set_savedir(struct uci_context *ctx, const char *dir)
>  {
>         char *sdir;
>         struct uci_element *e, *tmp;
> -       bool exists = false;
> +       volatile bool exists = false;
>
>         UCI_HANDLE_ERR(ctx);
>         UCI_ASSERT(ctx, dir != NULL);
> @@ -259,7 +259,7 @@ error:
>  static int uci_parse_delta(struct uci_context *ctx, FILE *stream, struct uci_package *p)
>  {
>         struct uci_parse_context *pctx;
> -       int changes = 0;
> +       volatile int changes = 0;
>
>         /* make sure no memory from previous parse attempts is leaked */
>         uci_cleanup(ctx);
> @@ -294,8 +294,8 @@ error:
>  /* returns the number of changes that were successfully parsed */
>  static int uci_load_delta_file(struct uci_context *ctx, struct uci_package *p, char *filename, FILE **f, bool flush)
>  {
> -       FILE *stream = NULL;
> -       int changes = 0;
> +       FILE *volatile stream = NULL;
> +       volatile int changes = 0;
>
>         UCI_TRAP_SAVE(ctx, done);
>         stream = uci_open_stream(ctx, filename, NULL, SEEK_SET, flush, false);
> @@ -317,8 +317,8 @@ __private int uci_load_delta(struct uci_context *ctx, struct uci_package *p, boo
>  {
>         struct uci_element *e;
>         char *filename = NULL;
> -       FILE *f = NULL;
> -       int changes = 0;
> +       FILE *volatile f = NULL;
> +       volatile int changes = 0;
>
>         if (!p->has_delta)
>                 return 0;
> @@ -419,9 +419,9 @@ done:
>
>  int uci_revert(struct uci_context *ctx, struct uci_ptr *ptr)
>  {
> -       char *package = NULL;
> -       char *section = NULL;
> -       char *option = NULL;
> +       char *volatile package = NULL;
> +       char *volatile section = NULL;
> +       char *volatile option = NULL;
>
>         UCI_HANDLE_ERR(ctx);
>         uci_expand_ptr(ctx, ptr, false);
> @@ -463,7 +463,7 @@ error:
>
>  int uci_save(struct uci_context *ctx, struct uci_package *p)
>  {
> -       FILE *f = NULL;
> +       FILE *volatile f = NULL;
>         char *filename = NULL;
>         struct uci_element *e, *tmp;
>         struct stat statbuf;
> diff --git a/file.c b/file.c
> index 7333e48..321b66b 100644
> --- a/file.c
> +++ b/file.c
> @@ -721,10 +721,10 @@ static void uci_file_commit(struct uci_context *ctx, struct uci_package **packag
>  {
>         struct uci_package *p = *package;
>         FILE *f1, *f2 = NULL;
> -       char *name = NULL;
> -       char *path = NULL;
> +       char *volatile name = NULL;
> +       char *volatile path = NULL;
>         char *filename = NULL;
> -       bool do_rename = false;
> +       volatile bool do_rename = false;
>         int fd;
>
>         if (!p->path) {
> @@ -881,12 +881,13 @@ static char **uci_list_config_files(struct uci_context *ctx)
>         return configs;
>  }
>
> -static struct uci_package *uci_file_load(struct uci_context *ctx, const char *name)
> +static struct uci_package *uci_file_load(struct uci_context *ctx,
> +                                        const char *volatile name)
>  {
>         struct uci_package *package = NULL;
>         char *filename;
>         bool confdir;
> -       FILE *file = NULL;
> +       FILE *volatile file = NULL;
>
>         switch (name[0]) {
>         case '.':
> diff --git a/list.c b/list.c
> index 78efbaf..41a8702 100644
> --- a/list.c
> +++ b/list.c
> @@ -623,8 +623,8 @@ int uci_add_list(struct uci_context *ctx, struct uci_ptr *ptr)
>  {
>         /* NB: UCI_INTERNAL use means without delta tracking */
>         bool internal = ctx && ctx->internal;
> -       struct uci_option *prev = NULL;
> -       const char *value2 = NULL;
> +       struct uci_option *volatile prev = NULL;
> +       const char *volatile value2 = NULL;
>
>         UCI_HANDLE_ERR(ctx);
>
> --
> 2.20.1
>
>
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel at lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel

_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel