[asterisk-bugs] [JIRA] (ASTERISK-24891) [USAN] Int overflow in strings.h

[Diederik de Groot (JIRA)]

    [ https://issues.asterisk.org/jira/browse/ASTERISK-24891?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=228303#comment-228303 ] 

Diederik de Groot edited comment on ASTERISK-24891 at 11/14/15 8:04 AM:
------------------------------------------------------------------------

Hi Matt/Badalian,

I do agree that this undefined behavior is not really worry some, it would be nice if we could pass this test/sanitize check without incident. 

The solution is simple and easy to implement (using unsigned int as you suggested). It might even be better to using unsigned long (as the original creator Dan Bernstein did), we have already used the cpu cycles calculating the hash, why throw away [the precision/its result] early instead of at the end.
{quote}
static force_inline int attribute_pure ast_str_hash(const char *str)
\{
    unsigned long hash = 5381;
    while (*str) hash = hash * 33 ^ *str++;
    return (int)hash;
\}
{quote}

There is no need to call abs() at the end, a cast will do (or return hash & INT_MAX). This looks cheaper to me, and it should create a better hash value in the end. 

Note: You could even unroll the loop and do four character at a time, if you like (indicated by the multiple by 33 which is a shift << 5), but that might make things more complex then they need to be.

The function is still returning an int at the end, so it should not require any other changes elsewhere in the code base.

Issues was raised in ASTERISK-24197, as well.


was (Author: dkdegroot):
Hi Matt/Badalian,

I do agree that this undefined behavior is not really worry some, it would be nice if we could pass this test/sanitize check without incident. 

The solution is simple and easy to implement (using unsigned int as you suggested). It might even be better to using unsigned long (as the original creator Dan Bernstein did), we have already used the cpu cycles calculating the hash, why throw away its result early instead of at the end.
{quote}
static force_inline int attribute_pure ast_str_hash(const char *str)
\{
    unsigned long hash = 5381;
    while (*str) hash = hash * 33 ^ *str++;
    return (int)hash;
\}
{quote}

There is no need to call abs() at the end, a cast will do. This looks cheaper to me, and it should create a better hash value in the end. 

Note: You could even unroll the loop and do four character at a time, if you like (indicated by the multiple by 33 which is a shift << 5), but that might make things more complex then they need to be.

The function is still returning an int at the end, so it should not require any other changes elsewhere in the code base.

Issues was raised in ASTERISK-24197, as well.

> [USAN] Int overflow in strings.h
> --------------------------------
>
>                 Key: ASTERISK-24891
>                 URL: https://issues.asterisk.org/jira/browse/ASTERISK-24891
>             Project: Asterisk
>          Issue Type: Bug
>      Security Level: None
>          Components: Core/General
>    Affects Versions: 11.16.0
>            Reporter: Badalian Vyacheslav
>            Severity: Minor
>
> Found by gcc {{Undefined santize}}
> To reproduce. 
> # Add ASTERISK-24718
> # configure with undefind santize
> # compile and install
> # Run 
> {code}
> /home/obs/asterisk-11.16.0-un/include/asterisk/strings.h:1009:15: runtime error: signed integer overflow: 193416315 * 33 cannot be represented in type 'int'
> /home/obs/asterisk-11.16.0-un/include/asterisk/strings.h:1009:15: runtime error: signed integer overflow: 193410403 * 33 cannot be represented in type 'int'
> /home/obs/asterisk-11.16.0-un/include/asterisk/strings.h:969:15: runtime error: signed integer overflow: 193449280 * 33 cannot be represented in type 'int'
> /home/obs/asterisk-11.16.0-un/include/asterisk/strings.h:1009:15: runtime error: signed integer overflow: 193352224 * 33 cannot be represented in type 'int'
> /home/obs/asterisk-11.16.0-un/include/asterisk/strings.h:1009:15: runtime error: signed integer overflow: 193426018 * 33 cannot be represented in type 'int'
> /home/obs/asterisk-11.16.0-un/include/asterisk/strings.h:1009:15: runtime error: signed integer overflow: 193426150 * 33 cannot be represented in type 'int'
> /home/obs/asterisk-11.16.0-un/include/asterisk/strings.h:1009:15: runtime error: signed integer overflow: 193404514 * 33 cannot be represented in type 'int'
> /home/obs/asterisk-11.16.0-un/include/asterisk/strings.h:1009:15: runtime error: signed integer overflow: 193405547 * 33 cannot be represented in type 'int'
> /home/obs/asterisk-11.16.0-un/include/asterisk/strings.h:1009:15: runtime error: signed integer overflow: 193434464 * 33 cannot be represented in type 'int'
> /home/obs/asterisk-11.16.0-un/include/asterisk/strings.h:1009:15: runtime error: signed integer overflow: 193404514 * 33 cannot be represented in type 'int'
> /home/obs/asterisk-11.16.0-un/include/asterisk/strings.h:1009:15: runtime error: signed integer overflow: 193353695 * 33 cannot be represented in type 'int'
> /home/obs/asterisk-11.16.0-un/include/asterisk/strings.h:1009:15: runtime error: signed integer overflow: 193404514 * 33 cannot be represented in type 'int'
> /home/obs/asterisk-11.16.0-un/include/asterisk/strings.h:1009:15: runtime error: signed integer overflow: 193404514 * 33 cannot be represented in type 'int'
> /home/obs/asterisk-11.16.0-un/include/asterisk/strings.h:1009:15: runtime error: signed integer overflow: 193358866 * 33 cannot be represented in type 'int'
> /home/obs/asterisk-11.16.0-un/include/asterisk/strings.h:1009:15: runtime error: signed integer overflow: 193433775 * 33 cannot be represented in type 'int'
> /home/obs/asterisk-11.16.0-un/include/asterisk/strings.h:969:15: runtime error: signed integer overflow: 193467535 * 33 cannot be represented in type 'in
> {code}



--
This message was sent by Atlassian JIRA
(v6.2#6252)