Repository navigation
Validate integer conversion ranges before narrowing - #2242
Open
idanfreiberg wants to merge 2 commits into
Open
idanfreiberg wants to merge 2 commits into
idanfreiberg wants to merge 2 commits into
Conversation
Contributor
Author
|
I'm not sure how it hurts performance so happy if someone take a second look |
rfuchs
reviewed
Nov 12, 2026
Comment on lines
+416
to
+440
| INLINE type name(const str *s, type def) { \ | ||
| char *ep; \ | ||
| char buf[max_len + 1]; \ | ||
| __typeof__(fn(buf, &ep, 10)) ret; \ | ||
| const __typeof__(ret) min = min_val, max = max_val; \ | ||
| if (s->len == 0 || s->len > max_len) \ | ||
| return def; \ | ||
| memcpy(buf, s->s, s->len); \ | ||
| buf[s->len] = '\0'; \ | ||
| /* A minus sign is only valid before digits, after any whitespace. */ \ | ||
| if (min == 0 && G_UNLIKELY(buf[0] < '0' || buf[0] > '9')) { \ | ||
| const char *start = buf; \ | ||
| while (isspace((unsigned char) *start)) \ | ||
| start++; \ | ||
| if (*start == '-') \ | ||
| return def; \ | ||
| } \ | ||
| errno = 0; \ | ||
| ret = fn(buf, &ep, 10); \ | ||
| if (errno == ERANGE || ep == buf || ep != buf + s->len) \ | ||
| return def; \ | ||
| if (ret < min || ret > max) \ | ||
| return def; \ | ||
| return ret; \ | ||
| } |
Member
There was a problem hiding this comment.
Put this into str_to_xx. It was introduced to remove duplication, and this reintroduces the code duplication
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Regression origin and disposition
The narrowing regression originated in bc08b059 — stricter str_to_X family. It was backported as 41cdf9d1 and first included in
mr26.0.1.25;mr26.0.1.24still has the explicit destination-range checks.The refactor aimed to make parsing stricter and avoid writing into the input buffer. Previously,
str_to_iparsed intolong longand rejected values outsideINT_MIN/INT_MAX. The shared macro instead assignsstrtol()directly into the destinationintbefore checking conversion errors and omits those destination-range checks. On AMD64 and ARM64, the input can fit inlongwhile overflowingint, so libc does not setERANGE.The introducing commit added no boundary tests under
t/. The demonstrated impact remains acceptance of a malformed RTCP port; a crash or RTP audio loss has not been established.Disposition: closing this proposal without merging. We are deferring this P2 issue and removing the downstream integer backport and its dedicated regressions for now. The upstream release behavior remains unchanged. The proposed implementation, validation, and benchmark evidence below are retained as historical context.
The integer conversion helpers can narrow a successful
strtol()result before validating the destination range. On LP64,str_to_i("4295003297", def)returns36001rather than the default. An SDP offer with RTP port35000anda=rtcp:4295003297 IN IP4 127.0.0.1therefore selects RTCP port36001, rather than ignoring the invalid attribute and using35001.Keep the full return type of each conversion function until validation completes. Reject
ERANGE, missing digits, trailing bytes (including embedded NULs), and values outside the destination range before narrowing. Usestrtoul()/strtoull()for unsigned helpers and reject a minus sign, including after leading whitespace. This also preserves valid unsigned values aboveLLONG_MAX. Floating-point conversion behavior is unchanged.Add boundary tests for all six integer helpers to
t/test-str.c. Add six NG offer/query cases to the existing RTCP daemon suite. They check the selected RTCP endpoint for valid ports, invalid port bounds, and positive/negative integers that previously wrapped to valid ports.Validation:
test-strpasses on upstream master with this change on ARM64, including AddressSanitizer and UndefinedBehaviorSanitizer.auto-test-helperharness.mr26.0.1.28, with production daemon and recording-binary builds on AMD64 and ARM64.mr26.0.1.28daemon with actual RTCP port36001.The demonstrated impact is selection of the wrong RTCP destination for malformed SDP. A crash or dropped RTP audio has not been established.
Unsigned fast path and native performance
Skip the full minus-sign scan when unsigned input starts with a digit. Otherwise, skip leading whitespace with
isspace((unsigned char) ...)and reject a leading minus. Internal signs still fail complete-consumption validation. All overflow, range, digit, and slice-consumption checks remain in place.Extend the unsigned regressions with every ASCII whitespace, mixed whitespace, valid plus/whitespace input, and every byte prefix before
-1. Expanded tests pass with ASAN/UBSAN on native AMD64 and ARM64. The upstream RTCP suite passes 313/313 checks.Native benchmark run: https://github.com/idanfreiberg/rtpengine/actions/runs/43164321493
The benchmark compares all three variants together on each native runner, using the pinned
mr26.0.1.28backport, GCC 13.3.0, identical flags/inputs, 15 rotated triplets, and one pinned vCPU. Changes are medians of paired ratios/differences. NG decoding uses the actual bencode decoder on four synthetic control messages. This does not measure full call setup or RTP forwarding. Raw samples, compiler/CPU metadata, and patch hashes are in the run artifacts.