rightTrimInPlace stack-buffer-underflow

[krisgry]

asan and claude helped me find a bug in rightTrimInPlace(char*): it reads (and may also write) one or more bytes before the start of its input buffer when the input is either:

1. the empty string, or
2. a string consisting entirely of whitespace.

Buggy code

dune/src/DUNE/Utils/String.cpp

Lines 83 to 89 in f505275

	String::rightTrimInPlace(char* str)
	{
	char* r = str + std::strlen(str) - 1; // Rightmost character
	for (; isspace(*r); --r)
	*r = 0;
	}

• Empty input: strlen(str) == 0, so r = str - 1. The very first isspace(*r) reads one byte before the buffer.
• All-whitespace input: the loop walks r past the start of the buffer with no lower bound check.
• Minor: isspace on a plain char is undefined for negative values (signed char on most ARM/x86 targets); should be cast to unsigned char.

More details from Claude, if you would like

## How it triggers in practice

Config::parseFile calls rightTrimInPlace(arg) after getOptionAndValue:

// src/DUNE/Parsers/Config.cpp
static bool
getOptionAndValue(const char* line, char* option, char* value)
{
  char equal[2] = {0};
  int rv = std::sscanf(line, " %[^=] %1[=] %[^;|#] ", option, equal, value);
  if (rv < 2 || rv > 3)
    return false;

  if (rv == 2)
    value[0] = '\0';   // <-- value (== arg at the call site) is now empty

  return true;
}
...
else if (getOptionAndValue(line, option, arg))
{
  ...
  String::rightTrimInPlace(option);
  String::rightTrimInPlace(arg);   // <-- reads arg[-1] when rv == 2
  ...
}

When a config line has a key followed by = but no value (or whatever else makes sscanf stop at rv == 2), arg is explicitly set to the empty string and then handed straight to rightTrimInPlace, which underflows.

Audit of all call sites

All four call sites are in src/DUNE/Parsers/Config.cpp:

Line	Call	Empty input reachable?	All-whitespace reachable?
113	rightTrimInPlace(section) after sscanf("[%[^]]] ", ...)	No (%[^]] requires ≥1 char)	Yes — e.g. [ ]
141	rightTrimInPlace(option) after getOptionAndValue	No (%[^=] requires ≥1 char)	Yes in principle (key consisting only of whitespace before =)
142	rightTrimInPlace(arg) after getOptionAndValue	Yes — rv == 2 path explicitly sets value[0] = '\0'	Yes
186	rightTrimInPlace(arg) after sscanf(" %[^;|#] ", ...)	No	Practically no (leading ws stripped by outer scanf)

There is no implicit precondition enforced at the call sites that would make the bug unreachable, so the fix belongs inside rightTrimInPlace itself.

Fix

void
String::rightTrimInPlace(char* str)
{
  size_t len = std::strlen(str);
  if (len == 0)
    return;

  char* r = str + len - 1; // Rightmost character
  for (; r >= str && isspace(static_cast<unsigned char>(*r)); --r)
    *r = 0;
}

Three things change:

1. Early return on empty input.
2. r >= str lower bound in the loop, so an all-whitespace input stops at the start of the buffer instead of running off it.
3. static_cast<unsigned char> before isspace to avoid UB on negative char values.

Seemingly trivial, but let me know if you want a PR.

ASan report

AddressSanitizer stack-buffer-overflow report (click to expand)

=================================================================
==3564==ERROR: AddressSanitizer: stack-buffer-overflow on address 0xbeb4fc5f at pc 0x00a59309 bp 0xbeb4dae8 sp 0xbeb4daec
READ of size 1 at 0xbeb4fc5f thread T0
    #0 0xa59306 in DUNE::Utils::String::rightTrimInPlace(char*) (/root/kg/dune_harvest_influx2/dune-2022.04.0-armv7-32bit-linux-glibc-gcc122/bin/dune+0x619306)
    #1 0x9ce36a in DUNE::Parsers::Config::parseFile(char const*) (/root/kg/dune_harvest_influx2/dune-2022.04.0-armv7-32bit-linux-glibc-gcc122/bin/dune+0x58e36a)
    #2 0x9d4bb0 in DUNE::Parsers::Config::parseFile(char const*) (/root/kg/dune_harvest_influx2/dune-2022.04.0-armv7-32bit-linux-glibc-gcc122/bin/dune+0x594bb0)
    #3 0x9d4bb0 in DUNE::Parsers::Config::parseFile(char const*) (/root/kg/dune_harvest_influx2/dune-2022.04.0-armv7-32bit-linux-glibc-gcc122/bin/dune+0x594bb0)
    #4 0x6b2cae in main (/root/kg/dune_harvest_influx2/dune-2022.04.0-armv7-32bit-linux-glibc-gcc122/bin/dune+0x272cae)
    #5 0xb676b316  (/lib/arm-linux-gnueabihf/libc.so.6+0x1e316)
    #6 0xb676b3c6 in __libc_start_main (/lib/arm-linux-gnueabihf/libc.so.6+0x1e3c6)
Address 0xbeb4fc5f is located in stack of thread T0 at offset 8319 in frame
    #0 0x9cdd34 in DUNE::Parsers::Config::parseFile(char const*) (/root/kg/dune_harvest_influx2/dune-2022.04.0-armv7-32bit-linux-glibc-gcc122/bin/dune+0x58dd34)
  This frame has 154 object(s):
    [32, 33) '<unknown>'
    [48, 49) '<unknown>'
    [64, 65) '<unknown>'
    [80, 81) '<unknown>'
    [96, 97) '<unknown>'
    [112, 113) '<unknown>'
    [128, 129) '<unknown>'
    [144, 145) '<unknown>'
    [160, 161) '<unknown>'
    [176, 177) '<unknown>'
    [192, 193) '<unknown>'
    [208, 209) '<unknown>'
    [224, 225) '<unknown>'
    [240, 241) '<unknown>'
    [256, 257) '<unknown>'
    [272, 273) '<unknown>'
    [288, 289) '<unknown>'
    [304, 305) '<unknown>'
    [320, 321) '<unknown>'
    [336, 337) '<unknown>'
    [352, 353) '<unknown>'
    [368, 369) '<unknown>'
    [384, 385) '<unknown>'
    [400, 401) '<unknown>'
    [416, 417) '<unknown>'
    [432, 433) '<unknown>'
    [448, 449) '<unknown>'
    [464, 465) '<unknown>'
    [480, 481) '<unknown>'
    [496, 497) '<unknown>'
    [512, 513) '<unknown>'
    [528, 529) '<unknown>'
    [544, 545) '<unknown>'
    [560, 561) '<unknown>'
    [576, 577) '<unknown>'
    [592, 593) '<unknown>'
    [608, 610) 'equal' (line 67)
    [624, 628) '<unknown>'
    [640, 644) '<unknown>'
    [656, 660) '<unknown>'
    [672, 676) 'sitr' (line 167)
    [688, 692) '<unknown>'
    [704, 708) 'oitr' (line 171)
    [720, 724) '<unknown>'
    [736, 740) '<unknown>'
    [752, 756) '<unknown>'
    [768, 772) '<unknown>'
    [784, 788) '__dnew'
    [800, 804) '__guard'
    [816, 820) '__dnew'
    [832, 836) '__guard'
    [848, 852) '__dnew'
    [864, 868) '__guard'
    [880, 884) '__dnew'
    [896, 900) '__guard'
    [912, 916) '<unknown>'
    [928, 932) '<unknown>'
    [944, 948) '<unknown>'
    [960, 964) '<unknown>'
    [976, 980) '<unknown>'
    [992, 996) '<unknown>'
    [1008, 1012) '<unknown>'
    [1024, 1028) '<unknown>'
    [1040, 1044) '__guard'
    [1056, 1060) '__dnew'
    [1072, 1076) '__guard'
    [1088, 1092) '__dnew'
    [1104, 1108) '__guard'
    [1120, 1124) '__dnew'
    [1136, 1140) '__guard'
    [1152, 1156) '__dnew'
    [1168, 1172) '__guard'
    [1184, 1188) '__dnew'
    [1200, 1204) '__guard'
    [1216, 1220) '__dnew'
    [1232, 1236) '__guard'
    [1248, 1252) '__dnew'
    [1264, 1268) '__guard'
    [1280, 1284) '__dnew'
    [1296, 1300) '__guard'
    [1312, 1316) '__dnew'
    [1328, 1332) '__guard'
    [1344, 1348) '<unknown>'
    [1360, 1364) '<unknown>'
    [1376, 1380) '<unknown>'
    [1392, 1396) '<unknown>'
    [1408, 1412) '<unknown>'
    [1424, 1428) '<unknown>'
    [1440, 1444) '<unknown>'
    [1456, 1460) '<unknown>'
    [1472, 1476) '__guard'
    [1488, 1492) '__dnew'
    [1504, 1508) '__guard'
    [1520, 1524) '__dnew'
    [1536, 1540) '__guard'
    [1552, 1556) '__dnew'
    [1568, 1572) '__guard'
    [1584, 1588) '__dnew'
    [1600, 1604) '__guard'
    [1616, 1620) '<unknown>'
    [1632, 1636) '<unknown>'
    [1648, 1652) '<unknown>'
    [1664, 1668) '<unknown>'
    [1680, 1684) '<unknown>'
    [1696, 1720) 'path' (line 117)
    [1760, 1784) '<unknown>'
    [1824, 1848) '<unknown>'
    [1888, 1912) '<unknown>'
    [1952, 1976) '<unknown>'
    [2016, 2040) '<unknown>'
    [2080, 2104) '<unknown>'
    [2144, 2168) 'path' (line 129)
    [2208, 2232) '<unknown>'
    [2272, 2296) '<unknown>'
    [2336, 2360) '<unknown>'
    [2400, 2424) '<unknown>'
    [2464, 2488) '<unknown>'
    [2528, 2552) '<unknown>'
    [2592, 2616) '<unknown>'
    [2656, 2680) '<unknown>'
    [2720, 2744) '<unknown>'
    [2784, 2808) '<unknown>'
    [2848, 2872) '<unknown>'
    [2912, 2936) '<unknown>'
    [2976, 3000) '<unknown>'
    [3040, 3064) '<unknown>'
    [3104, 3128) '<unknown>'
    [3168, 3192) '<unknown>'
    [3232, 3256) '<unknown>'
    [3296, 3320) '<unknown>'
    [3360, 3384) '<unknown>'
    [3424, 3448) '<unknown>'
    [3488, 3512) '<unknown>'
    [3552, 3576) '<unknown>'
    [3616, 3640) '<unknown>'
    [3680, 3704) '<unknown>'
    [3744, 3768) '<unknown>'
    [3808, 3832) '<unknown>'
    [3872, 3896) '<unknown>'
    [3936, 3960) '<unknown>'
    [4000, 4024) '<unknown>'
    [4064, 4088) '<unknown>'
    [4128, 4152) '<unknown>'
    [4192, 4216) '<unknown>'
    [4256, 4280) '<unknown>'
    [4320, 4528) 'ss'
    [4592, 4800) 'ss'
    [4864, 5888) 'line' (line 88)
    [6016, 7040) 'section' (line 89)
    [7168, 8192) 'option' (line 90)
    [8320, 9344) 'arg' (line 91) <== Memory access at offset 8319 underflows this variable
    [9472, 10496) 'tmp' (line 92)
    [10624, 11648) 'isec' (line 93)
    [11776, 12800) 'iopt' (line 94)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow (/root/kg/dune_harvest_influx2/dune-2022.04.0-armv7-32bit-linux-glibc-gcc122/bin/dune+0x619306) in DUNE::Utils::String::rightTrimInPlace(char*)
Shadow bytes around the buggy address:
  0x37d69f30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x37d69f40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x37d69f50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x37d69f60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x37d69f70: 00 00 00 00 00 00 00 00 00 00 00 00 f2 f2 f2 f2
=>0x37d69f80: f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2[f2]00 00 00 00
  0x37d69f90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x37d69fa0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x37d69fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x37d69fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x37d69fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
==3564==ABORTING

The "this may be a false positive" hint at the bottom is generic ASan boilerplate referring to programs that use swapcontext/vfork/custom unwinders. It does not apply here — parseFile is just a normal (recursive) function.

[Bernardo-Gabriel]

Hi @krisgry, thanks for this.
Could you please open a PR implementing the suggested change?
Much appreciated.