[Home]

Summary:ASTERISK-24891: [USAN] Int overflow in strings.h
Reporter:Badalian Vyacheslav (slavon)Labels:
Date Opened:2015-03-17 12:54:51Date Closed:2016-02-09 19:57:06.000-0600
Priority:MinorRegression?
Status:Closed/CompleteComponents:Core/General
Versions:11.16.0 Frequency of
Occurrence
Related
Issues:
Environment:Attachments:
Description: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}
Comments:By: Matt Jordan (mjordan) 2015-03-17 12:59:40.283-0500

Generally, the fact that the two hash functions may roll over is not harmful. The actual hash table implementations already handle this:

{code}
i = abs(c->hash_fn(user_data, OBJ_POINTER));
{code}

Yes, this is undefined behaviour, but in this case, practically it causes little or no harm.

Unless you're willing to write a patch to convert this to an {{unsigned int}} across the board, I doubt this will receive any attention.
By: Diederik de Groot (dkdegroot) 2015-11-14 07:50:30.811-0600

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.
By: Badalian Vyacheslav (slavon) 2016-02-09 19:57:06.540-0600

Fresh USAN ASTERISK-25761