lpereira / lwan

Experimental, scalable, high performance HTTP server
https://lwan.ws
GNU General Public License v2.0
5.92k stars 549 forks source link

Possible NULL pointer dereference on function unsigned int parse_time_period(const char *str, unsigned int default_value) #295

Closed ycaibb closed 3 years ago

ycaibb commented 3 years ago

Dear developers: Our static analysis tool reports a NULL pointer dereference on here. The reason is that the function strchr may return null and should be checked before dereferencing at while site. I notice that function strchr is invoked in many different contexts where it is checked for returning non-null for further usage. It may be a false positive. Thank you for your help.

unsigned int parse_time_period(const char *str, unsigned int default_value)
{
    ...;
    while (*str && sscanf(str, "%u%c", &period, &multiplier) == 2) { // *str may trigger null pointer dereference
        switch (multiplier) {
        case 's': total += period; break;
        case 'm': total += period * ONE_MINUTE; break;
        case 'h': total += period * ONE_HOUR; break;
        case 'd': total += period * ONE_DAY; break;
        case 'w': total += period * ONE_WEEK; break;
        case 'M': total += period * ONE_MONTH; break;
        case 'y': total += period * ONE_YEAR; break;
        default:
            lwan_status_warning("Ignoring unknown multiplier: %c",
                        multiplier);
        }

        str = strchr(str, multiplier) + 1; //str may be null
    }
...;
}
lpereira commented 3 years ago

Thanks for the report. However, this is impossible to happen in this situation: if sscanf() returns 2, multiplier (set by sscanf()) is guaranteed to be in the string pointed to by str, so strchr() won't ever return NULL here. Unless I'm missing something, though.

ycaibb commented 3 years ago

Thank you very much for your explanation. It seems our static tool doesn't model well for the strchr and sscanf method.