Bug #4809
closedNANOSEC should be 'long long' to avoid integer overflow bugs
100%
Description
NANOSEC is defined as: 1000000000. This makes it an int - a 32-bit signed integer (on both ILP32 and LP64).
If NANOSEC is used to convert seconds to nanoseconds (by multiplying), the naive approach will fail with quantities larger than 2 seconds. For example:
hrtime_t convert(int secs) { return (secs * NANOSEC); }
Since both secs and NANOSEC are integers, the compiler will compute the product and then sign extend the result to 64-bits.
There are many places in the gate that work around this by casting or specifying suffixes:
x = (hrtime_t)secs * NANOSEC; y = 15ll * NANOSEC;
Changing the definition of NANOSEC to 1000000000ll will mitigate any bugs caused by the programmers not realizing that NANOSEC is a mere int.
The difficult part of this change is verifying that nothing broke as a result.
wsdiff on nightly proto areas built by gcc found ~30 files that were different after the change. I manually compared the disassembly of each of these to make sure that the differences were harmless.
Most of the differences were caused by gcc swapping a pair of instructions (read: harmless).
Below is the list of the remaining files that were different because of different instruction choice due to arithmetic being performed using 64-bit values. These are harmless as well.
kernel/drv/profile kernel/drv/amd64/profile kernel/dtrace/profile kernel/dtrace/amd64/profile kernel/drv/audio kernel/drv/amd64/audio kernel/genunix kernel/amd64/genunix usr/lib/fm/libfmd_log.so.1 usr/lib/fm/fmd/fmd usr/bin/timex platform/i86pc/kernel/mach/pcplusmp platform/i86pc/kernel/mach/apix platform/i86pc/kernel/mach/amd64/apix platform/i86pc/kernel/mach/amd64/pcplusmp platform/i86pc/kernel/unix platform/i86xpv/kernel/unix platform/i86pc/kernel/amd64/unix platform/i86xpv/kernel/amd64/unix usr/lib/zones/zonestatd
Bellow are files that are different because redefining NANOSEC fixed a bug. (See #4810 for ZFS and #4811 for in.mpathd)
kernel/fs/zfs kernel/fs/amd64/zfs usr/lib/libzpool.so.1 usr/lib/amd64/libzpool.so.1 lib/inet/in.mpathd
Related issues
Updated by Gordon Ross over 8 years ago
If the expected result is most often an hrtime_t, how about using the ULL suffix?
Updated by Josef Sipek over 8 years ago
Because hrtime_t is defined as a signed 64-bit int:
http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/sys/time.h#249
Updated by Electric Monk over 8 years ago
- Status changed from New to Closed
- % Done changed from 80 to 100
git commit b59e2127f21675e88c58a4dd924bc55eeb83c7a6
commit b59e2127f21675e88c58a4dd924bc55eeb83c7a6 Author: Josef 'Jeff' Sipek <josef.sipek@nexenta.com> Date: 2014-04-29T15:42:11.000Z 4809 NANOSEC should be 'long long' to avoid integer overflow bugs 4810 spa_async_tasks_pending suffers from an integer overflow bug 4811 in.mpathd: tv2ns suffers from an integer overflow bug Reviewed by: Marcel Telka <marcel.telka@nexenta.com> Reviewed by: Dan McDonald <danmcd@omniti.com> Approved by: Robert Mustacchi <rm@joyent.com>