Ineffective overflow checks

[rohanlean]

utf8proc tries to respond to signed integer overflow after the overflow has already occurred. Because signed integer overflow is Undefined Behaviour in C, the compiler can replace the overflow branch by whatever it wants. An optimising compiler will typically elide it.

[stevengj]

Can you indicate which line(s) of code you are referring to?

[rohanlean]

Here is an example. The positive signed integer variable written is increased, then its sign is tested as if signed integer overflow would wrap instead of leading to undefined behaviour:

utf8proc/utf8proc.c

Lines 368 to 371 in 93a88b4

	written += utf8proc_decompose_char(entry_cp, dst+written,
	(bufsize > written) ? (bufsize - written) : 0, options,
	last_boundclass);
	if (written < 0) return UTF8PROC_ERROR_OVERFLOW;

At a glance I see the following additional instances of this problem. There may be more.

utf8proc/utf8proc.c

Lines 539 to 543 in 93a88b4

	rpos += utf8proc_iterate(str + rpos, -1, &uc);
	/* checking of return value is not necessary,
	as 'uc' is < 0 in case of error */
	if (uc < 0) return UTF8PROC_ERROR_INVALIDUTF8;
	if (rpos < 0) return UTF8PROC_ERROR_OVERFLOW;

utf8proc/utf8proc.c

Lines 558 to 563 in 93a88b4

	wpos += decomp_result;
	/* prohibiting integer overflows due to too long strings: */
	if (wpos < 0 ||
	wpos > (utf8proc_ssize_t)(SSIZE_MAX/sizeof(utf8proc_int32_t)/2))
	return UTF8PROC_ERROR_OVERFLOW;
	}

[stevengj]

Fair enough.

The simplest "solution" here is just to remove the checks entirely. (They are pretty pointless on 64-bit machines anywhere where integer overflow cannot really happen here in practice.) The counter-argument is that leaving them in is relatively harmless, and may catch some overflows on 32-bit machines depending on the compiler.

[rohanlean]

If you decide to keep the checks as they are, I would suggest to comment the if-statements. Currently they display a disturbing disparity between apparent intent and actual behaviour. But at that point it seems easier and clearer to just fix the checks properly. You just need to replace

x = a + b;
if (x < 0) {/* ... */}

with

if (FOO_MAX - a < b) {/* ... */}
x = a + b;

which is a correct check if and only if a is positive.