Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix many errors that occur on 32-bit systems under UBSan #804

Merged
merged 2 commits into from
Jan 1, 2025

Conversation

JohnoKing
Copy link

This commit fixes fixes a large number of runtime errors that occur when ksh is compiled with -fsanitize=undefined on Linux 32-bit using GCC.

Noted changes:

  • Switched out long and double for Sflong_t and Sfdouble_t in many places to fix runtime errors under UBSan. (In certain places (u)int64_t was used to specify exactly 64-bits rather than intmax_t.) The following errors should no longer occur:
src/cmd/ksh93/sh/macro.c:2489:9: runtime error: left shift of 1 by 31 places cannot be represented in type 'long int'
src/cmd/ksh93/sh/args.c:252:4: runtime error: left shift of 1 by 31 places cannot be represented in type 'long int'
src/cmd/ksh93/sh/args.c:272:4: runtime error: left shift of 1 by 31 places cannot be represented in type 'long int'
src/cmd/builtin/pty.c:355:12: runtime error: left shift of 12 by 28 places cannot be represented in type 'long int'
src/cmd/ksh93/sh/args.c:175:60: runtime error: left shift of 1 by 31 places cannot be represented in type 'long int'
src/cmd/ksh93/sh/xec.c:2359:4: runtime error: signed integer overflow: 1734423994 - -1213168416 cannot be represented in type 'long int'
src/cmd/builtin/pty.c:355:12: runtime error: left shift of 12 by 28 places cannot be represented in type 'long int'
src/cmd/ksh93/sh/args.c:607:10: runtime error: left shift of 1 by 31 places cannot be represented in type 'long int'
src/cmd/ksh93/sh/args.c:622:38: runtime error: left shift of 1 by 31 places cannot be represented in type 'long int'
  • For mamake, use unsigned long long to avoid requiring <stdint.h>.

  • Remove !timeofday code. All modern systems have gettimeofday, which makes this code unused and an unnecessary maintenance burden.

  • Define _TIME_BITS=64 to fix integer overflows in the time keyword and likely elsewhere (ksh assumes time_t is 64-bits).

  • Moved the AST_LC_* bitflags into the 32-bit range.

  • Don't assume long long and intmax_t are the same. While intmax_t is in practice the same as long long, this is not guaranteed by the C standard (it's permitted for these types to differ). For correctness, only use %jd and %ju with Sflong_t (aka intmax_t), and enforce this distinction in SFIO's handling of %ll and %j. (The extra if statements are optimized away on platforms where the types are identical.)

  • Use suseconds_t for tv->usec and enforce 64-bit time_t in the libast feature tests.

  • Casted more PIDs to Sflong_t when using PIDs with format specifiers.

  • Use the %zu format specifier with size_t vars rather than unnecessary casts.

This commit fixes fixes a large number of runtime errors that occur when
ksh is compiled with '-fsanitize=undefined' on Linux 32-bit using GCC.

Noted changes:
- Switched out long and double for Sflong_t and Sfdouble_t in many
  places to fix runtime errors under UBSan. (In certain places
  (u)int64_t was used to specify exactly 64-bits rather than
  intmax_t.) The following errors should no longer occur:
  src/cmd/ksh93/sh/macro.c:2489:9: runtime error: left shift of 1 by 31 places cannot be represented in type 'long int'
  src/cmd/ksh93/sh/args.c:252:4: runtime error: left shift of 1 by 31 places cannot be represented in type 'long int'
  src/cmd/ksh93/sh/args.c:272:4: runtime error: left shift of 1 by 31 places cannot be represented in type 'long int'
  src/cmd/builtin/pty.c:355:12: runtime error: left shift of 12 by 28 places cannot be represented in type 'long int'
  src/cmd/ksh93/sh/args.c:175:60: runtime error: left shift of 1 by 31 places cannot be represented in type 'long int'
  src/cmd/ksh93/sh/xec.c:2359:4: runtime error: signed integer overflow: 1734423994 - -1213168416 cannot be represented in type 'long int'
  src/cmd/builtin/pty.c:355:12: runtime error: left shift of 12 by 28 places cannot be represented in type 'long int'
  src/cmd/ksh93/sh/args.c:607:10: runtime error: left shift of 1 by 31 places cannot be represented in type 'long int'
  src/cmd/ksh93/sh/args.c:622:38: runtime error: left shift of 1 by 31 places cannot be represented in type 'long int'

- For mamake, use 'unsigned long long' to avoid requiring stdint.h.

- Remove !timeofday code. All modern systems have gettimeofday, which
  makes this code unused and an unnecessary maintenance burden.

- Define _TIME_BITS=64 to fix integer overflows in the time keyword and
  likely elsewhere (ksh assumes time_t is 64-bits).

- Moved the AST_LC_* bitflags into the 32-bit range.

- Don't assume 'long long' and 'intmax_t' are the same. While intmax_t is
  in practice the same as 'long long', this is not guaranteed by the C
  standard (it's permitted for these types to differ). For correctness,
  only use %jd and %ju with Sflong_t (aka intmax_t), and enforce this
  distinction in SFIO's handling of %ll and %j. (The extra if statements
  are optimized away on platforms where the types are identical.)

- Use suseconds_t for tv->usec and enforce 64-bit time_t in the libast
  feature tests.

- Casted more PIDs to Sflong_t when using PIDs with format specifiers.

- Use the %zu format specifier with size_t vars rather than unnecessary
  casts.
@McDutchie
Copy link

Thanks for all this work.

The use of long long in this PR means we now officially abandon C90 and require C99. I am okay with this; see #777.

@McDutchie
Copy link

  • Remove !timeofday code. All modern systems have gettimeofday, which makes this code unused and an unnecessary maintenance burden.

Looks like gettimeofday was removed in POSIX-1.2024, so we probably should not rely on having it. POSIX-1.2017 marked it as obsolescent and recommends using clock_gettime instead. In our code it would probably be best to use tvgettime from libast.

Since this is unrelated to fixing UBSan errors, I think this would be best dealt with in a separate issue/commit.

src/lib/libast/features/time Outdated Show resolved Hide resolved
src/lib/libast/features/time Outdated Show resolved Hide resolved
@McDutchie McDutchie merged commit 0a2302e into ksh93:dev Jan 1, 2025
@JohnoKing JohnoKing deleted the fix-32bit-asan-bitoverflow2 branch January 1, 2025 08:21
McDutchie pushed a commit that referenced this pull request Jan 2, 2025
This commit fixes fixes a large number of runtime errors that occur
when ksh is compiled with '-fsanitize=undefined' on Linux 32-bit
using GCC.

Noted changes:
- Switched out long and double for Sflong_t and Sfdouble_t in many
  places to fix runtime errors under UBSan. (In certain places
  (u)int64_t was used to specify exactly 64-bits rather than
  intmax_t.) The following runtime errors should no longer occur
  (line numbers/positions as of commit e67af95):

  * left shift of 1 by 31 places cannot be represented in type
    'long int': macro.c:2489:9, args.c:252:4, args.c:272:4,
    args.c:175:60, args.c:607:10, args.c:622:38
  * left shift of 12 by 28 places cannot be represented in type
    'long int': pty.c:355:12
  * signed integer overflow: 1734423994 - -1213168416 cannot be
    represented in type 'long int': xec.c:2359:4

- For mamake, use 'unsigned long long' instead of 'uint64_t' to
  avoid requiring stdint.h.

- Remove !timeofday code. All modern systems have gettimeofday,
  which makes this code unused and an unnecessary maintenance
  burden.

- Define _TIME_BITS=64 to fix integer overflows in the time keyword
  and likely elsewhere (ksh assumes time_t is 64-bits).

- Moved the AST_LC_* bitflags into the 32-bit range.

- Don't assume 'long long' and 'intmax_t' are the same. While
  intmax_t is in practice the same as 'long long', this is not
  guaranteed by the C standard (it's permitted for these types to
  differ). For correctness, only use %jd and %ju with Sflong_t (aka
  intmax_t), and enforce this distinction in SFIO's handling of %ll
  and %j. (The extra if statements are optimized away on platforms
  where the types are identical.)

- Use suseconds_t for tv->usec and enforce 64-bit time_t in the
  libast feature tests.

- Casted more PIDs to Sflong_t when using PIDs with format
  specifiers.

- Use the %zu format specifier with size_t vars rather than
  unnecessary casts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants