Re: [OpenWrt-Devel] [PATCH uci 2/2] build: Add -Wclobbered to detect problems with longjmp
Hauke Mehrtens [2019-11-07 23:51:50]: Hi, > How do we want to go forward with these patches? as noone provided better fix for those warnings(or proved them wrong) yet, then I would simply move forward. Your changes pass all unit tests and CI compile tests so: Acked-by: Petr Štetiar -- ynezz ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH uci 2/2] build: Add -Wclobbered to detect problems with longjmp
On 11/7/19 9:51 AM, Petr Štetiar wrote: > Hauke Mehrtens [2019-11-07 00:26:23]: > > Hi, > >>> I didn't looked deeper, but GCC 6,7,8,9 on x86/64 reports following: >>> >>> list.c:626:21: error: variable ‘prev’ might be clobbered by ‘longjmp’ or >>> ‘vfork’ [-Werror=clobbered] >>> 626 | struct uci_option *prev = NULL; >>> | ^~~~ >>> >>> list.c:627:14: error: variable ‘value2’ might be clobbered by ‘longjmp’ or >>> ‘vfork’ [-Werror=clobbered] >>> 627 | const char *value2 = NULL; >>> | ^~ >> >> I think I also saw these problems in my manual review, but the >> -Wclobbered did not complain about them for me with gcc 8.3 on MIPS. > > Interesting, out of curiosity which MIPS? BTW I see a consistent gcc 8.3 > behaviour[1] across all CI testing targets (ath79, malta be, imx6, armada > a-53) with -Wextra and without your patch applied. You need to click on the > "[x] Failed" button in order to see the build failure. > > 1. https://gitlab.com/ynezz/openwrt-uci/pipelines/94051766/builds I also saw these and they are fixed in my patch by adding a volatile which is the correct fix for these problems. It was something else where I saw some potential problem in the review, but GCC was not warning about, as far as I know. How do we want to go forward with these patches? Hauke signature.asc Description: OpenPGP digital signature ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH uci 2/2] build: Add -Wclobbered to detect problems with longjmp
Hauke Mehrtens [2019-11-07 00:26:23]: Hi, > > I didn't looked deeper, but GCC 6,7,8,9 on x86/64 reports following: > > > > list.c:626:21: error: variable ‘prev’ might be clobbered by ‘longjmp’ or > > ‘vfork’ [-Werror=clobbered] > > 626 | struct uci_option *prev = NULL; > > | ^~~~ > > > > list.c:627:14: error: variable ‘value2’ might be clobbered by ‘longjmp’ or > > ‘vfork’ [-Werror=clobbered] > > 627 | const char *value2 = NULL; > > | ^~ > > I think I also saw these problems in my manual review, but the > -Wclobbered did not complain about them for me with gcc 8.3 on MIPS. Interesting, out of curiosity which MIPS? BTW I see a consistent gcc 8.3 behaviour[1] across all CI testing targets (ath79, malta be, imx6, armada a-53) with -Wextra and without your patch applied. You need to click on the "[x] Failed" button in order to see the build failure. 1. https://gitlab.com/ynezz/openwrt-uci/pipelines/94051766/builds -- ynezz ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH uci 2/2] build: Add -Wclobbered to detect problems with longjmp
On 11/5/19 1:27 AM, Petr Štetiar wrote:
> Hi,
>
> Hauke Mehrtens [2019-11-01 17:06:34]:
>
>> +ADD_DEFINITIONS(-Os -Wall -Werror -Wclobbered --std=gnu99 -g3 -I.
>> -DUCI_PREFIX="${CMAKE_INSTALL_PREFIX}")
>
> is there any reason to not use -Wextra directly?
I was looking on how we could prevent to have a similar problem as the
one described in CVE-2019-15513 and found this warning which should have
warned us about this problem. First I was trying to understand this CVE
and then I wanted to learn from it to prevent such problems next time.
I support adding -Wextra it is even better.
>
> list.c:140:11: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> file.c:572:51: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> file.c:850:15: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> file.c:865:15: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> delta.c:199:6: error: this statement may fall through
> [-Werror=implicit-fallthrough=]
> parse.c:80:12: error: this statement may fall through
> [-Werror=implicit-fallthrough=]
> parse.c:81:12: error: this statement may fall through
> [-Werror=implicit-fallthrough=]
> file.c:572:51: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> file.c:850:15: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> file.c:865:15: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> delta.c:199:6: error: this statement may fall through
> [-Werror=implicit-fallthrough=]
> parse.c:80:12: error: this statement may fall through
> [-Werror=implicit-fallthrough=]
> parse.c:81:12: error: this statement may fall through
> [-Werror=implicit-fallthrough=]
> ucimap.c:146:16: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> ucimap.c:151:17: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> ucimap.c:243:34: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> ucimap.c:247:9: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> ucimap.c:254:39: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> ucimap.c:258:9: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> ucimap.c:285:34: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> ucimap.c:363:17: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> ucimap.c:563:12: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> ucimap.c:753:18: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
> ucimap.c:879:17: error: comparison between signed and unsigned integer
> expressions [-Werror=sign-compare]
>
> Yousong Zhou [2019-11-04 11:29:05]:
>
>> The -Wclobber check can produce false-positives right?
>
> I didn't looked deeper, but GCC 6,7,8,9 on x86/64 reports following:
>
> list.c:626:21: error: variable ‘prev’ might be clobbered by ‘longjmp’ or
> ‘vfork’ [-Werror=clobbered]
> 626 | struct uci_option *prev = NULL;
> | ^~~~
>
> list.c:627:14: error: variable ‘value2’ might be clobbered by ‘longjmp’ or
> ‘vfork’ [-Werror=clobbered]
> 627 | const char *value2 = NULL;
> | ^~
I think I also saw these problems in my manual review, but the
-Wclobbered did not complain about them for me with gcc 8.3 on MIPS.
>
> but clang 9,10 on x86/64 doesn't.
>
> -- ynezz
>
Hauke
signature.asc
Description: OpenPGP digital signature
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH uci 2/2] build: Add -Wclobbered to detect problems with longjmp
Hi,
Hauke Mehrtens [2019-11-01 17:06:34]:
> +ADD_DEFINITIONS(-Os -Wall -Werror -Wclobbered --std=gnu99 -g3 -I.
> -DUCI_PREFIX="${CMAKE_INSTALL_PREFIX}")
is there any reason to not use -Wextra directly?
list.c:140:11: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
file.c:572:51: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
file.c:850:15: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
file.c:865:15: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
delta.c:199:6: error: this statement may fall through
[-Werror=implicit-fallthrough=]
parse.c:80:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
parse.c:81:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
file.c:572:51: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
file.c:850:15: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
file.c:865:15: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
delta.c:199:6: error: this statement may fall through
[-Werror=implicit-fallthrough=]
parse.c:80:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
parse.c:81:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
ucimap.c:146:16: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
ucimap.c:151:17: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
ucimap.c:243:34: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
ucimap.c:247:9: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
ucimap.c:254:39: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
ucimap.c:258:9: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
ucimap.c:285:34: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
ucimap.c:363:17: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
ucimap.c:563:12: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
ucimap.c:753:18: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
ucimap.c:879:17: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
Yousong Zhou [2019-11-04 11:29:05]:
> The -Wclobber check can produce false-positives right?
I didn't looked deeper, but GCC 6,7,8,9 on x86/64 reports following:
list.c:626:21: error: variable ‘prev’ might be clobbered by ‘longjmp’ or
‘vfork’ [-Werror=clobbered]
626 | struct uci_option *prev = NULL;
| ^~~~
list.c:627:14: error: variable ‘value2’ might be clobbered by ‘longjmp’ or
‘vfork’ [-Werror=clobbered]
627 | const char *value2 = NULL;
| ^~
but clang 9,10 on x86/64 doesn't.
-- ynezz
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH uci 2/2] build: Add -Wclobbered to detect problems with longjmp
On 11/4/19 4:29 AM, Yousong Zhou wrote: > Hi Hauke > > On Sat, 2 Nov 2019 at 00:07, Hauke Mehrtens 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 > > 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"? Hi Yousong, Yes, some of the volatiles are not necessary, but I activated the warning -Wclobbered and GCC is not so intelligent and warns for all variables which are written after setjmp() is called. When we activate the warning everyone gets this warning and no one will introduce such a problem again, but we probably have volatile also in some places where it is not needed. I want to handle this part of the setjmp() manpage: 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 GCC documentation says this: -Wclobbered Warn for variables that might be changed by longjmp or vfork. This warning is also enabled by -Wextra. Hauke signature.asc Description: OpenPGP digital signature ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH uci 2/2] build: Add -Wclobbered to detect problems with longjmp
Hi Hauke
On Sat, 2 Nov 2019 at 00:07, Hauke Mehrtens 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
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 = N
[OpenWrt-Devel] [PATCH uci 2/2] 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.
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
---
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;
+
