[pacman-dev] [PATCH 0/3] Char signedness issues
I tried compiling pacman on my RPi (Arm), where char is unsigned by default. That leads to some things not working the way they are expected too. The problem is as follows: char a = -1; if (a == -1) { ... } If char is unsigned, we will not enter the if-statement (a is converted to int) before making the comparisson. Gcc will warn about these comparissons. The first two patches fixes the cases I (and Gcc) could find, and the third one is a couple of tests I wrote to convince myself there was a problem in pacsort (and more importantly, that the changes actually fixes something). After doing this, I realize that the simplest solution probably would be to add -fsigned-char as an argument to the compiler... thoughts? Rikard Falkeborn (3): Make alpm_graph state signedness explicit pacsort, introduce define for escape_char error code Add pacsort tests with invalid input lib/libalpm/graph.h | 2 +- src/util/pacsort.c | 11 ++++++----- test/util/pacsorttest.sh | 18 +++++++++++++++++- 3 files changed, 24 insertions(+), 7 deletions(-) -- 2.6.4
The signedness of char is implementation defined. Since the alpm_graph state is clearly meant to be signed, make the signedness explicit. This fixes bugs on systems where char is unsigned, in comparissons of the following type: if(v.state == -1) which, if state is unsigned, will never be true due to integer promotion rules. Fixes failing test/pacman/tests/sync012.py when compiling with -funsigned-char. Fixes two warnings [-Wtype-limits] for comparissons with -1 when compiling with -funsigned-char. Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com> --- lib/libalpm/graph.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/libalpm/graph.h b/lib/libalpm/graph.h index fb24d73..ae0b5d9 100644 --- a/lib/libalpm/graph.h +++ b/lib/libalpm/graph.h @@ -29,7 +29,7 @@ typedef struct __alpm_graph_t { alpm_list_t *children; alpm_list_t *childptr; /* points to a child in children list */ off_t weight; /* weight of the node */ - char state; /* 0: untouched, -1: entered, other: leaving time */ + signed char state; /* 0: untouched, -1: entered, other: leaving time */ } alpm_graph_t; alpm_graph_t *_alpm_graph_new(void); -- 2.6.4
The signedness of char is implementation defined. On systems where char is unsigned, comparing a variable of type char with -1 is never true, due to integer promotion rules. To avoid this, introduce a define for invalid field separators where -1 is cast to char. This will ensure that the return value check works for both unsigned and signed char. Fixes one warning [-Wtype-limits] for comparissons with -1 when compiling with -funsigned-char. Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com> --- src/util/pacsort.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/util/pacsort.c b/src/util/pacsort.c index e7dc63e..dcf1ade 100644 --- a/src/util/pacsort.c +++ b/src/util/pacsort.c @@ -28,6 +28,7 @@ #include "util-common.h" #define DELIM ' ' +#define INVALD_ESCAPE_CHAR ((char)-1) #ifndef MIN #define MIN(a, b) \ @@ -385,13 +386,13 @@ static int vercmp(const void *p1, const void *p2) static char escape_char(const char *string) { if(!string) { - return -1; + return INVALD_ESCAPE_CHAR; } const size_t len = strlen(string); if(len > 2) { - return -1; + return INVALD_ESCAPE_CHAR; } if(len == 1) { @@ -399,7 +400,7 @@ static char escape_char(const char *string) } if(*string != '\\') { - return -1; + return INVALD_ESCAPE_CHAR; } switch(string[1]) { @@ -412,7 +413,7 @@ static char escape_char(const char *string) case '0': return '\0'; default: - return -1; + return INVALD_ESCAPE_CHAR; } } @@ -463,7 +464,7 @@ static int parse_options(int argc, char **argv) break; case 't': opts.delim = escape_char(optarg); - if(opts.delim == -1) { + if(opts.delim == INVALD_ESCAPE_CHAR) { fprintf(stderr, "error: invalid field separator -- `%s'\n", optarg); return 1; } -- 2.6.4
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com> --- test/util/pacsorttest.sh | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/test/util/pacsorttest.sh b/test/util/pacsorttest.sh index 3b285b8..32ee6ba 100755 --- a/test/util/pacsorttest.sh +++ b/test/util/pacsorttest.sh @@ -35,7 +35,16 @@ tap_runtest() { tap_diff <(printf "$1" | $bin $4) <(printf "$2") "$3" } -tap_plan 26 +# args: +# check_return_value input expected_return_value test_description optional_opts +tap_check_return_value() { + # run the test + printf "$1" | $bin $4 2>/dev/null + tap_is_int "$?" "$2" "$3" + +} + +tap_plan 32 in="1\n2\n3\n4\n" tap_runtest $in $in "already ordered" @@ -108,6 +117,13 @@ tap_runtest "$separator_reverse" "$separator" "really long input, sort key, sepa tap_runtest "$separator_reverse" "$separator_reverse" "really long input, sort key, separator, reversed" "-k 3 -t| -r" tap_runtest "$separator" "$separator_reverse" "really long input, sort key, separator, reversed" "-k 3 -t| -r" +tap_check_return_value "" "2" "invalid sort key (no argument)" "-k" +tap_check_return_value "" "2" "invalid sort key (non-numeric)" "-k asd" +tap_check_return_value "" "2" "invalid field separator (no argument)" "-t" +tap_check_return_value "" "2" "invalid field separator (multiple characters)" "-t sda" +tap_check_return_value "" "2" "invalid field separator (two characters must start with a slash)" "-t ag" +tap_check_return_value "" "2" "invalid field separator (\g is invalid)" '-t \g' + tap_finish # vim: set noet: -- 2.6.4
On 31/12/15 23:19, Rikard Falkeborn wrote:
The signedness of char is implementation defined. Since the alpm_graph state is clearly meant to be signed, make the signedness explicit.
This fixes bugs on systems where char is unsigned, in comparissons of the following type:
if(v.state == -1)
which, if state is unsigned, will never be true due to integer promotion rules.
Fixes failing test/pacman/tests/sync012.py when compiling with -funsigned-char.
Fixes two warnings [-Wtype-limits] for comparissons with -1 when compiling with -funsigned-char.
Pulled.
On 31/12/15 23:19, Rikard Falkeborn wrote:
The signedness of char is implementation defined. On systems where char is unsigned, comparing a variable of type char with -1 is never true, due to integer promotion rules. To avoid this, introduce a define for invalid field separators where -1 is cast to char. This will ensure that the return value check works for both unsigned and signed char.
Fixes one warning [-Wtype-limits] for comparissons with -1 when compiling with -funsigned-char.
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
OK.
participants (2)
-
Allan McRae
-
Rikard Falkeborn