From 05cbbc7181569b326f6057a295c85a68f23ef578 Mon Sep 17 00:00:00 2001 From: PatR Date: Sun, 9 Jun 2024 14:17:14 -0700 Subject: [PATCH] fix PR #1254 - avoid signed integer overflow Pull request from mkuoppal: avoid integer overflow when user types digits and they're combined into a number by successively multiplying intermediate value by 10 and adding new digit. Needed to avoid triggering undefined behavior if the value overflows the largest signed integer (actually long int). This is a much more general fix than the code in the pull request, which imposed an arbitrary limit for one aspect of tty input. I'm not convinced that integer.h was the right place to add the new AppendLongDigit() macro. I may not have caught all the places where it is needed. files.c accumulates a value from digits but uses unsigned int, so overflow won't trigger undefined behavior (although it presumably ends up with a different value than what was intended). options.c and coloratt.c accumulate smaller integers and have a limit on the number of digits they'll use, so can't overflow. Fixes #1254 --- include/integer.h | 11 ++++++++++- src/cmd.c | 7 +++++-- win/X11/winX.c | 11 +++++++++-- win/tty/topl.c | 7 +++++-- win/tty/wintty.c | 11 +++++++++-- win/win32/mswproc.c | 8 ++++++-- 6 files changed, 44 insertions(+), 11 deletions(-) diff --git a/include/integer.h b/include/integer.h index 6ce6054e3..435da8ad3 100644 --- a/include/integer.h +++ b/include/integer.h @@ -1,4 +1,4 @@ -/* NetHack 3.7 integer.h $NHDT-Date: 1596498539 2020/08/03 23:48:59 $ $NHDT-Branch: NetHack-3.7 $:$NHDT-Revision: 1.8 $ */ +/* NetHack 3.7 integer.h $NHDT-Date: 1717967331 2024/06/09 21:08:51 $ $NHDT-Branch: NetHack-3.7 $:$NHDT-Revision: 1.12 $ */ /* Copyright (c) 2016 by Michael Allison */ /* NetHack may be freely redistributed. See license for details. */ @@ -101,4 +101,13 @@ typedef uint32_t uint32; typedef int64_t int64; typedef uint64_t uint64; +/* for non-negative L, calculate L * 10 + D, avoiding signed overflow; + yields -1 if overflow would have happened; + assumes compiler will optimize the constants */ +#define AppendLongDigit(L,D) \ + (((L) < LONG_MAX / 10L \ + || ((L) == LONG_MAX / 10L && (D) <= LONG_MAX % 10L)) \ + ? (L) * 10L + (D) \ + : -1L) + #endif /* INTEGER_H */ diff --git a/src/cmd.c b/src/cmd.c index c25d05d0f..af69e19e5 100644 --- a/src/cmd.c +++ b/src/cmd.c @@ -1,4 +1,4 @@ -/* NetHack 3.7 cmd.c $NHDT-Date: 1716592962 2024/05/24 23:22:42 $ $NHDT-Branch: NetHack-3.7 $:$NHDT-Revision: 1.728 $ */ +/* NetHack 3.7 cmd.c $NHDT-Date: 1717967336 2024/06/09 21:08:56 $ $NHDT-Branch: NetHack-3.7 $:$NHDT-Revision: 1.729 $ */ /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */ /*-Copyright (c) Robert Patrick Rankin, 2013. */ /* NetHack may be freely redistributed. See license for details. */ @@ -4669,7 +4669,10 @@ get_count( } if (digit(key)) { - cnt = 10L * cnt + (long) (key - '0'); + long dgt = (long) (key - '0'); + + /* cnt = (10 * cnt) + (key - '0'); */ + cnt = AppendLongDigit(cnt, dgt); if (cnt < 0L) cnt = 0L; else if (maxcount > 0L && cnt > maxcount) diff --git a/win/X11/winX.c b/win/X11/winX.c index c92f38f75..8daf610ef 100644 --- a/win/X11/winX.c +++ b/win/X11/winX.c @@ -1,4 +1,4 @@ -/* NetHack 3.7 winX.c $NHDT-Date: 1643491577 2022/01/29 21:26:17 $ $NHDT-Branch: NetHack-3.7 $:$NHDT-Revision: 1.110 $ */ +/* NetHack 3.7 winX.c $NHDT-Date: 1717967337 2024/06/09 21:08:57 $ $NHDT-Branch: NetHack-3.7 $:$NHDT-Revision: 1.136 $ */ /* Copyright (c) Dean Luick, 1992 */ /* NetHack may be freely redistributed. See license for details. */ @@ -2219,8 +2219,15 @@ yn_key(Widget w, XEvent *event, String *params, Cardinal *num_params) } else { if (yn_getting_num) { if (digit(ch)) { + long dgt = (long) (ch - '0'); + yn_ndigits++; - yn_val = (yn_val * 10) + (long) (ch - '0'); + /* yn_val = (10 * yn_val) + (ch - '0'); */ + yn_val = AppendLongDigit(yn_val, dgt); + if (yn_val < 0L) { + yn_ndigits = 0; + yn_val = 0; + } return; /* wait for more input */ } if (yn_ndigits && (ch == '\b' || ch == 127 /*DEL*/)) { diff --git a/win/tty/topl.c b/win/tty/topl.c index 82965a6df..e9f70e420 100644 --- a/win/tty/topl.c +++ b/win/tty/topl.c @@ -1,4 +1,4 @@ -/* NetHack 3.7 topl.c $NHDT-Date: 1596498344 2020/08/03 23:45:44 $ $NHDT-Branch: NetHack-3.7 $:$NHDT-Revision: 1.72 $ */ +/* NetHack 3.7 topl.c $NHDT-Date: 1717967339 2024/06/09 21:08:59 $ $NHDT-Branch: NetHack-3.7 $:$NHDT-Revision: 1.89 $ */ /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */ /*-Copyright (c) Michael Allison, 2009. */ /* NetHack may be freely redistributed. See license for details. */ @@ -493,7 +493,10 @@ tty_yn_function( if (!preserve_case) z = lowc(z); if (digit(z)) { - value = (10 * value) + (z - '0'); + long dgt = (long) (z - '0'); + + /* value = (10 * value) + (z - '0'); */ + value = AppendLongDigit(value, dgt); if (value < 0) break; /* overflow: try again */ digit_string[0] = z; diff --git a/win/tty/wintty.c b/win/tty/wintty.c index d7ec5a45e..d6cdf6200 100644 --- a/win/tty/wintty.c +++ b/win/tty/wintty.c @@ -1,4 +1,4 @@ -/* NetHack 3.7 wintty.c $NHDT-Date: 1717482166 2024/06/04 06:22:46 $ $NHDT-Branch: NetHack-3.7 $:$NHDT-Revision: 1.408 $ */ +/* NetHack 3.7 wintty.c $NHDT-Date: 1717967340 2024/06/09 21:09:00 $ $NHDT-Branch: NetHack-3.7 $:$NHDT-Revision: 1.410 $ */ /* Copyright (c) David Cohrs, 1991 */ /* NetHack may be freely redistributed. See license for details. */ @@ -1566,7 +1566,14 @@ process_menu_window(winid window, struct WinDesc *cw) if (!counting && strchr(gacc, morc)) goto group_accel; - count = (count * 10L) + (long) (morc - '0'); + { + long dgt = (long) (morc - '0'); + + /* count = (10 * count) + (morc - '0'); */ + count = AppendLongDigit(count, dgt); + if (count < 0L) /* overflow */ + continue; /* reset_count is True */ + } /* * It is debatable whether we should allow 0 to * start a count. There is no difference if the diff --git a/win/win32/mswproc.c b/win/win32/mswproc.c index 2b0df926c..d64aa0734 100644 --- a/win/win32/mswproc.c +++ b/win/win32/mswproc.c @@ -1,4 +1,4 @@ -/* NetHack 3.7 mswproc.c $NHDT-Date: 1613292828 2021/02/14 08:53:48 $ $NHDT-Branch: NetHack-3.7 $:$NHDT-Revision: 1.165 $ */ +/* NetHack 3.7 mswproc.c $NHDT-Date: 1717967341 2024/06/09 21:09:01 $ $NHDT-Branch: NetHack-3.7 $:$NHDT-Revision: 1.193 $ */ /* Copyright (C) 2001 by Alex Kompel */ /* NetHack may be freely redistributed. See license for details. */ @@ -1637,6 +1637,7 @@ mswin_yn_function(const char *question, const char *choices, char def) char z, digit_string[2]; int n_len = 0; long value = 0; + mswin_putstr_ex(WIN_MESSAGE, ATR_BOLD, ("#"), 1); n_len++; digit_string[1] = '\0'; @@ -1650,7 +1651,10 @@ mswin_yn_function(const char *question, const char *choices, char def) do { /* loop until we get a non-digit */ z = lowc(readchar()); if (digit(z)) { - value = (10 * value) + (z - '0'); + long dgt = (long) (z - '0'); + + /* value = (10 * value) + (z - '0'); */ + value = AppendLongDigit(value, dgt); if (value < 0) break; /* overflow: try again */ digit_string[0] = z;