Hi Hauke
On Sat, 2 Nov 2019 at 00:07, Hauke Mehrtens <ha...@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 <ha...@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@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
