Project

General

Profile

Bug #4809

NANOSEC should be 'long long' to avoid integer overflow bugs

Added by Josef Sipek almost 5 years ago. Updated 11 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
kernel
Start date:
2014-04-25
Due date:
% Done:

100%

Estimated time:
Difficulty:
Bite-size
Tags:

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

Related to illumos gate - Bug #4810: spa_async_tasks_pending suffers from an integer overflow bugClosed2014-04-25

Actions
Related to illumos gate - Bug #4811: in.mpathd: tv2ns suffers from an integer overflow bugClosed2014-04-25

Actions

History

#1

Updated by Gordon Ross almost 5 years ago

If the expected result is most often an hrtime_t, how about using the ULL suffix?

#2

Updated by Josef Sipek almost 5 years ago

#3

Updated by Electric Monk almost 5 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>

#4

Updated by Robert Mustacchi almost 5 years ago

  • Tags deleted (needs-triage)

Also available in: Atom PDF