nicolasff / webdis

A Redis HTTP interface with JSON output
https://webd.is
BSD 2-Clause "Simplified" License
2.82k stars 307 forks source link

Fix http_parser check for architectures with unsigned chars #207

Closed jessie-murray closed 2 years ago

jessie-murray commented 2 years ago

http_parser.c has a table named unhex that it uses to convert hex characters to their numeric values, e.g. 'F' -> 15. For non-hex characters, the value is -1 but while the table contains int8_t values, the extraction is done using a char. On ARMv8, char is unsigned, which means it can't be compared to -1 as this is always false. Comparing to (char)-1 instead will work.

Compiling Webdis on ARMv8 produces two warnings:

src/http-parser/http_parser.c: In function ‘http_parser_execute’:
src/http-parser/http_parser.c:1443:15: warning: comparison is always false due to limited range of data type [-Wtype-limits]
         if (c == -1) goto error;
               ^~
src/http-parser/http_parser.c:1460:15: warning: comparison is always false due to limited range of data type [-Wtype-limits]
         if (c == -1) {
               ^~

Here is a program that demonstrates the issue:

#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char *argv[]) {
    int8_t unhex[] = {0, 1, 255, -1};
    for (int i = 0; i < sizeof(unhex) / sizeof(unhex[0]); i++) {
        char c = unhex[i];
        printf("c = %02x", c);
        if (c == -1) {
            printf(" == -1");
        }
        if (c == (int8_t)-1) {
            printf(" == (int8_t)-1");
        }
        if (c == (char)-1) {
            printf(" == (char)-1");
        }
        printf("\n");
    }
    return 0;
}

On x86_64, both 255 and -1 match all cases:

$ cc -o test-int8 test-int8.c && ./test-int8
c = 00
c = 01
c = ffffffff == -1 == (int8_t)-1 == (char)-1
c = ffffffff == -1 == (int8_t)-1 == (char)-1

On ARMv8, only (char)-1 matches:

$ cc -o test-int8 test-int8.c && ./test-int8
c = 00
c = 01
c = ff == (char)-1
c = ff == (char)-1

Changing the two comparisons against -1 to (char)-1 fixes the issue.

nicolasff commented 2 years ago

Thanks!

This looks good but I'm still concerned that there might be some other issues that aren't mentioned at build time. I haven't tested webdis very much on ARMv8; I do have a board with this architecture (a Jetson Nano), but haven't used it for this purpose yet. I'll try to find a way to validate builds on this architecture in the future though.

nicolasff commented 2 years ago

Rebased and merged as 55128ae.