build: Add -Wclobbered to detect problems with longjmp

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.

The setjmp() manage says the following:
----------------------------------------------------------------------
The  compiler  may  optimize  variables into registers, and longjmp()
may restore the values of other registers in addition to the stack
pointer and program counter.  Consequently, the values of automatic
variables are unspecified after a call to longjmp() if they meet all the
following criteria:
* they are local to the function that made the corresponding setjmp()
  call;
* their values are changed between the calls to setjmp() and longjmp();
  and
* they are not declared as volatile.
---------------------------------------------------------------------

The -Wclobbered compiler option warns about all variables which are
written after setjmp() was called, not all of them could cause problems,
but to make sure to catch all real problems add this warning and fix all
occurrences of this warning.

This also activates a compiler warning which should warn us in such
cases.
This could fix some potential problems in error paths like the one
reported in CVE-2019-15513.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 170eb0b7b8876e1500c6ca559b8637c35ab82b8e..578c0212779a78070dd7225335259e88b2ff8906 100644 (file)
--- 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 386167db4cd1500b56f80d0e3d29f82594cafa2a..52ebe3beb3b6af196d3ef38fea6d6f1b67b84ffe 100644 (file)
--- 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 7333e48d51f7d7912a221d9e42fb86becf3d9668..321b66b02eb3a79e1b74ade1327c10099167da88 100644 (file)
--- 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 78efbafe1c06572b3ae46e5e72457d659f0ea52b..41a87028eefc267b3b3b0ea2cfe61a482df6b693 100644 (file)
--- 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);