[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/2] libc: Zentralisieren der strtoxxx-Funktionen
Am 04.03.2021 um 20:48 hat Nico Mayer geschrieben:
> From: Nico Mayer <mayerNico@xxxxxxxxxx>
>
> * Bisher gibt es ein gewisses Durcheinander, was die strtoxxx-Funktionen
> anbelangt. Die Funktionen strtol und strtoll werden in der Datei
> strtol.c implementiert. Die Funktionen strtoul und strtoull werden
> wiederum direkt in string.c implementiert. Dieser Commit zentralisiert
> die Implementierung der verschiedenen strtoxxx-Funktionen. Alle
> strtoxxx-Funktionen jetzt werden durch die Datei strtol.c implementiert.
>
> Signed-off-by: Nico Mayer <mayerNico@xxxxxxxxxx>
> ---
> src/lib/stdlibc/strtol.c | 128 ++++++++++++++++++++++++++++-----------
> src/lib/string.c | 104 -------------------------------
> 2 files changed, 93 insertions(+), 139 deletions(-)
>
> diff --git a/src/lib/stdlibc/strtol.c b/src/lib/stdlibc/strtol.c
> index 0160ebcf..47f03a36 100644
> --- a/src/lib/stdlibc/strtol.c
> +++ b/src/lib/stdlibc/strtol.c
> @@ -31,76 +31,134 @@
> #include "limits.h"
> #include "types.h"
>
> -static long long do_strtoll(const char *str, char **endptr, int base,
> - long long min_val, long long max_val)
> +static unsigned long long do_strtoull(const char *str, char **endptr, int base,
> + unsigned long long max_val,
> + unsigned long long min_val)
Die Einrückung passt nicht mehr. Du kannst entweder bis zur Klammer
einrücken (wie vor deinem Patch) oder pauschal vier Zeichen.
> {
> - long long retval = 0;
> - int overflow = 0;
> - char sign = 0;
> + unsigned long long result = 0;
> + int neg = 0;
> int digit;
> + int overflow = 0;
neg und overflow werden nur als bool benutzt, dann sollten sie auch so
deklariert werden.
> + // Leerzeichen am Anfang muessen ignoriert werden
> while (isspace(*str)) {
> str++;
> }
>
> - // Moegliches Vorzeichen auswerten
> - if (*str == '+' || *str == '-') {
> - sign = *str;
> + // Vorzeichen
> + if (*str == '+') {
> + str++;
> + } else if (*str == '-') {
> + neg = 1;
> str++;
> }
Hast du vom alten strtoull() übernommen und ist tatsächlich schöner.
> - // Moegliches 0, 0x und 0X auswerten
> - if (*str == '0')
> - {
> - if (str[1] == 'x' || str[1] == 'X') {
> - if (base == 0) {
> - base = 16;
> + // Basis feststellen
> + // Bei Basis 16 darf der String mit 0x anfangen
> + // Bei Basis 0 ist automatische Erkennung
> + // Anfang ist 0x: Basis 16
> + // Anfang ist 0: Basis 8
> + // Ansonsten: Basis 10
> + switch (base) {
> + case 0:
> + base = 10;
> + if (*str == '0') {
> + str++;
> + if (*str == 'x' || *str == 'X') {
> + if (isxdigit(str[1])) {
> + base = 16;
> + str++;
> + }
> + } else {
> + base = 8;
> + }
> }
> + break;
>
> - if (base == 16 && isxdigit(str[2])) {
> + case 16:
> + if ((*str == '0') && (str[1] == 'x' || str[1] == 'X')
> + && isxdigit(str[2]))
> + {
> str += 2;
> }
> - } else if (base == 0) {
> - base = 8;
> - }
> + break;
> }
Dieser Teil ist ebenfalls besser als davor.
> - while (*str)
> - {
> - if (isdigit(*str) && *str - '0' < base) {
> - digit = *str - '0';
> - } else if(isalpha(*str) && tolower(*str) - 'a' + 10 < base) {
> - digit = tolower(*str) - 'a' + 10;
> - } else {
> - break;
> + // Ergebnis berechnen
> + result = 0;
> + while (*str) {
> + switch (*str) {
> + case '0' ... '9':
> + if (*str - '0' > base - 1) {
> + goto out;
> + }
> + digit = *str - '0';
> + break;
> + case 'A' ... 'Z':
> + if (*str - 'A' + 10 > base - 1) {
> + goto out;
> + }
> + digit = *str - 'A' + 10;
> + break;
> + case 'a' ... 'z':
> + if (*str - 'a' + 10 > base - 1) {
> + goto out;
> + }
> + digit = *str - 'a' + 10;
> + break;
> +
> + default:
> + goto out;
> }
Bis hierher tut es dasselbe wie der alte Code, nur ausführlicher
geschrieben. Ok.
> - if (retval > (max_val - digit) / base) {
> - overflow = 1;
> + if (neg) {
> + if (result > (min_val - digit) / base) {
> + errno = ERANGE;
> + overflow = 1;
> + }
Hier wird es knifflig. Ich weiß nicht genau, was du hier checkst. Mal
versuchen zu analysieren, was hier passiert:
min_val ist unsigned long long, digit ist int. Für die Subtraktion wird
digit nach unsigned long long konvertiert, d.h. wir rechnen unsigned.
Die unsigned-Varianten lügen hier und geben ihren Maximalwert statt 0
als min_val. In diesem Fall tut der Code also dasselbe wie der
else-Block. Ich glaube, negative und positive Werte gleich zu parsen ist
richtig.
Die signed-Varianten übergeben für min_val ihr Minimum (also eine
negative Zahl), allerdings konvertiert zu einem unsigned long long, der
den Wert nicht repräsentieren kann.
Damit haben wir als Obergrenze für result * base + digit:
* für strtoll(): ULLONG_MAX - LLONG_MIN = (2^64 - 1) - 2^63 = 2^63 - 1
* für strtol(): ULLONG_MAX - LONG_MIN = (2^64 - 1) - 2^32
Ersteres ist falsch, weil -2^63 als Eingabe funktionieren sollte, aber
so ERANGE setzt. Alle anderen Werte sollten in Ordnung sein.
Zweiteres ist vollkommener Blödsinn und lässt viel zu große Eingaben zu.
> + } else {
> + if (result > (max_val - digit) / base) {
> + errno = ERANGE;
> + overflow = 1;
> + }
max_val ist immer positiv, es gibt hier also keine überraschende
Konvertierung. Der Check prüft, dass die nächste Zeile result nicht
größer werden lässt als max_val. Gut.
> }
> - retval = retval * base + digit;
> +
> + result = result * base + digit;
>
> str++;
> }
> +out:
>
> if (endptr != NULL) {
> - *(const char**)endptr = str;
> + *endptr = (char*) str;
> }
>
> if (overflow) {
> - errno = ERANGE;
> - return (sign == '-') ? min_val : max_val;
> + return neg ? min_val : max_val;
> }
>
> - return (sign == '-') ? -retval : retval;
> + return neg ? - result : result;
-result sollte ohne Leerzeichen sein, das ist ein unäres Minus.
> }
>
> +
Überflüssige Leerzeile.
> long long strtoll(const char *str, char **endptr, int base)
> {
> - return do_strtoll(str, endptr, base, LLONG_MIN, LLONG_MAX);
> + return (long long)do_strtoull(str, endptr, base, LLONG_MAX, LLONG_MIN);
> }
>
> long strtol(const char *str, char **endptr, int base)
> {
> - return do_strtoll(str, endptr, base, LONG_MIN, LONG_MAX);
> + return (long)do_strtoull(str, endptr, base, LONG_MAX, LONG_MIN);
> +}
Ich glaube, man kann das Problem relativ leicht fixen, wenn du statt
min_val und max_val den Maximalwert für positiv und negativ ohne
Vorzeichen übergibst, d.h. max_pos und max_neg.
Dann würden diese beiden Funktionen max_neg = -LLONG_MIN bzw. -LONG_MIN
übergeben, was beides positive Zahlen sind, die in einen unsigned long
long reinpassen.
Der overflow-Fall müsste dann natürlich -max_neg statt min_val
zurückgeben (also zusätzliche Negation), aber dein Overflowcheck wäre
richtig, wenn du min_val direkt durch max_neg ersetzt.
> +unsigned long long strtoull(const char *str, char **endptr, int base)
> +{
> + return (unsigned long long)do_strtoull(str, endptr, base,
> + ULLONG_MAX, ULLONG_MAX);
> }
> +
> +unsigned long strtoul(const char *str, char **endptr, int base)
> +{
> + return (unsigned long)do_strtoull(str, endptr, base, ULONG_MAX, ULONG_MAX);
> +}
Falls deine Bugs von den bisherigen Testfällen nicht abgedeckt sind,
wäre es gut, da auch ein paar neue Fälle hinzuzufügen.
Kevin