ref: ee5da4468d061b5fa0f321643cec35b42aecbcdd
parent: 503c3b53649cc74218b281c35ff423d1fbdf83bf
author: Rangi <[email protected]>
date: Fri Apr 16 20:52:55 EDT 2021
Fix interpolation/STRFMT overflow issues (#838) Widths and fractional widths greater than 255 would overflow a uint8_t and wrap around to smaller values. Total formatted lengths greater than the avilable buffer size would overflow it and potentially corrupt memory. Fixes #830 Closes #831
--- a/include/asm/format.h
+++ b/include/asm/format.h
@@ -28,9 +28,9 @@
bool prefix;
bool alignLeft;
bool padZero;
- uint8_t width;
+ size_t width;
bool hasFrac;
- uint8_t fracWidth;
+ size_t fracWidth;
int type;
bool valid;
};
--- a/src/asm/format.c
+++ b/src/asm/format.c
@@ -6,6 +6,7 @@
* SPDX-License-Identifier: MIT
*/
+#include <assert.h>
#include <inttypes.h>
#include <math.h>
#include <stdbool.h>
@@ -148,20 +149,25 @@
size_t len = strlen(value);
size_t totalLen = fmt->width > len ? fmt->width : len;
- if (totalLen + 1 > bufLen) /* bufLen includes terminator */
+ if (totalLen > bufLen - 1) { /* bufLen includes terminator */ error("Formatted string value too long\n");+ totalLen = bufLen - 1;
+ if (len > totalLen)
+ len = totalLen;
+ }
+ assert(len < bufLen && totalLen < bufLen && len <= totalLen);
- size_t padLen = fmt->width > len ? fmt->width - len : 0;
+ size_t padLen = totalLen - len;
if (fmt->alignLeft) {- strncpy(buf, value, len < bufLen ? len : bufLen);
- for (size_t i = 0; i < totalLen && len + i < bufLen; i++)
- buf[len + i] = ' ';
+ memcpy(buf, value, len);
+ for (size_t i = len; i < totalLen; i++)
+ buf[i] = ' ';
} else {- for (size_t i = 0; i < padLen && i < bufLen; i++)
+ for (size_t i = 0; i < padLen; i++)
buf[i] = ' ';
- if (bufLen > padLen)
- strncpy(buf + padLen, value, bufLen - padLen - 1);
+ if (totalLen > padLen)
+ memcpy(buf + padLen, value, len);
}
buf[totalLen] = '\0';
@@ -221,12 +227,18 @@
/* Special case for fixed-point */
/* Default fractional width (C's is 6 for "%f"; here 5 is enough) */
- uint8_t fracWidth = fmt->hasFrac ? fmt->fracWidth : 5;
+ size_t fracWidth = fmt->hasFrac ? fmt->fracWidth : 5;
if (fracWidth) {+ if (fracWidth > 255) {+ error("Fractional width %zu too long, limiting to 255\n",+ fracWidth);
+ fracWidth = 255;
+ }
+
char spec[16]; /* Max "%" + 5-char PRIu32 + ".%0255.f" + terminator */
- snprintf(spec, sizeof(spec), "%%" PRIu32 ".%%0%d.f", fracWidth);
+ snprintf(spec, sizeof(spec), "%%" PRIu32 ".%%0%zu.f", fracWidth);
snprintf(valueBuf, sizeof(valueBuf), spec, value >> 16,
(value % 65536) / 65536.0 * pow(10, fracWidth) + 0.5);
} else {@@ -253,46 +265,47 @@
size_t totalLen = fmt->width > numLen ? fmt->width : numLen;
- if (totalLen + 1 > bufLen) /* bufLen includes terminator */
+ if (totalLen > bufLen - 1) { /* bufLen includes terminator */ error("Formatted numeric value too long\n");+ totalLen = bufLen - 1;
+ if (numLen > totalLen) {+ len -= numLen - totalLen;
+ numLen = totalLen;
+ }
+ }
+ assert(numLen < bufLen && totalLen < bufLen && numLen <= totalLen && len <= numLen);
- size_t padLen = fmt->width > numLen ? fmt->width - numLen : 0;
+ size_t padLen = totalLen - numLen;
+ size_t pos = 0;
if (fmt->alignLeft) {- size_t pos = 0;
-
- if (sign && pos < bufLen)
+ if (sign)
buf[pos++] = sign;
- if (prefix && pos < bufLen)
+ if (prefix)
buf[pos++] = prefix;
-
- strcpy(buf + pos, valueBuf);
- pos += len;
-
- for (size_t i = 0; i < totalLen && pos + i < bufLen; i++)
- buf[pos + i] = ' ';
+ memcpy(buf + pos, valueBuf, len);
+ for (size_t i = pos + len; i < totalLen; i++)
+ buf[i] = ' ';
} else {- size_t pos = 0;
-
if (fmt->padZero) {/* sign, then prefix, then zero padding */
- if (sign && pos < bufLen)
+ if (sign)
buf[pos++] = sign;
- if (prefix && pos < bufLen)
+ if (prefix)
buf[pos++] = prefix;
- for (size_t i = 0; i < padLen && pos < bufLen; i++)
+ for (size_t i = 0; i < padLen; i++)
buf[pos++] = '0';
} else {/* space padding, then sign, then prefix */
- for (size_t i = 0; i < padLen && pos < bufLen; i++)
+ for (size_t i = 0; i < padLen; i++)
buf[pos++] = ' ';
- if (sign && pos < bufLen)
+ if (sign)
buf[pos++] = sign;
- if (prefix && pos < bufLen)
+ if (prefix)
buf[pos++] = prefix;
}
- if (bufLen > pos)
- strcpy(buf + pos, valueBuf);
+ if (totalLen > pos)
+ memcpy(buf + pos, valueBuf, len);
}
buf[totalLen] = '\0';
--- a/src/asm/rgbasm.5
+++ b/src/asm/rgbasm.5
@@ -337,7 +337,7 @@
\[en]
.Ql 9 .
If specified, prints this many digits of a fixed-point fraction.
-Defaults to 5 digits.
+Defaults to 5 digits, maximum 255 digits.
.It Ql <type> Ta Specifies the type of value.
.El
.Pp
--- /dev/null
+++ b/test/asm/format-truncation.asm
@@ -1,0 +1,15 @@
+num equ 42
+fix equ 123.0
+str equs "hello"
+
+println "{#0260x:num}"+println "{#-260x:num}"+println "{0280.260f:fix}"+println "{260s:str}"+println "{-260s:str}"+
+println "<{#0260x:num}>"+println "<{#-260x:num}>"+println "<{0280.260f:fix}>"+println "<{260s:str}>"+println "<{-260s:str}>"--- /dev/null
+++ b/test/asm/format-truncation.err
@@ -1,0 +1,35 @@
+ERROR: format-truncation.asm(5):
+ Formatted numeric value too long
+ERROR: format-truncation.asm(6):
+ Formatted numeric value too long
+ERROR: format-truncation.asm(7):
+ Fractional width 260 too long, limiting to 255
+ERROR: format-truncation.asm(7):
+ Formatted numeric value too long
+ERROR: format-truncation.asm(8):
+ Formatted string value too long
+ERROR: format-truncation.asm(9):
+ Formatted string value too long
+ERROR: format-truncation.asm(11):
+ Formatted numeric value too long
+warning: format-truncation.asm(11): [-Wlong-string]
+ String constant too long
+ERROR: format-truncation.asm(12):
+ Formatted numeric value too long
+warning: format-truncation.asm(12): [-Wlong-string]
+ String constant too long
+ERROR: format-truncation.asm(13):
+ Fractional width 260 too long, limiting to 255
+ERROR: format-truncation.asm(13):
+ Formatted numeric value too long
+warning: format-truncation.asm(13): [-Wlong-string]
+ String constant too long
+ERROR: format-truncation.asm(14):
+ Formatted string value too long
+warning: format-truncation.asm(14): [-Wlong-string]
+ String constant too long
+ERROR: format-truncation.asm(15):
+ Formatted string value too long
+warning: format-truncation.asm(15): [-Wlong-string]
+ String constant too long
+error: Assembly aborted (12 errors)!
--- /dev/null
+++ b/test/asm/format-truncation.out
@@ -1,0 +1,10 @@
+$0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002a
+$2a
+123.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
+ hello
+hello
+<$0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002
+<$2a
+<123.0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
+< hell
+<hello
--- /dev/null
+++ b/test/asm/interpolation-overflow.asm
@@ -1,0 +1,4 @@
+; It seems that \1 was the easiest way to notice the memory corruption that
+; resulted from this overflow
+x = 0
+{.99999999f:x}\1--- /dev/null
+++ b/test/asm/interpolation-overflow.err
@@ -1,0 +1,9 @@
+ERROR: interpolation-overflow.asm(4):
+ Fractional width 99999999 too long, limiting to 255
+ERROR: interpolation-overflow.asm(4):
+ Formatted numeric value too long
+warning: interpolation-overflow.asm(4): [-Wlarge-constant]
+ Precision of fixed-point constant is too large
+while expanding symbol "0.0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
+FATAL: interpolation-overflow.asm(4):
+ Macro argument '\1' not defined