AVRO-4275: [C] Fix signed integer overflow in block_count negation by iemejia · Pull Request #3842 · apache/avro

iemejia · 2026-07-11T08:08:23Z

Summary

Fix signed integer overflow (CWE-190) in the C decoder when negating negative block counts for arrays and maps. When block_count == INT64_MIN, negation is undefined behavior in C since -INT64_MIN is not representable in int64_t.

Changes

value-read.c: Replace block_count * -1 with an explicit INT64_MIN guard that returns EINVAL, followed by safe block_count = -block_count for all other negative values.
datum_skip.c: Apply the same guard to skip_array() and skip_map().
consume-binary.c: Apply the same guard to read_array() and read_map().
test_avro_4275.c: Regression test that verifies INT64_MIN block counts are rejected with EINVAL, and that legitimate negative block counts still decode correctly. Uses explicit (char) casts for 0xFF byte literals to avoid implementation-defined narrowing.

Testing

All existing C SDK tests pass. New test passes under valgrind memcheck with no leaks.

read_array_value() and read_map_value() negate block_count via multiplication by -1, which is undefined behavior for values that cannot be represented after negation. Replace with the overflow-safe -(x+1)+1 idiom (consistent with the C++ fix in AVRO-4228) and reject invalid results early. Add test_avro_4275 covering the overflow case and verifying that legitimate negative block counts still decode correctly. Assisted-by: OpenCode:claude-opus-4.6

Copilot

Pull request overview

This PR addresses undefined behavior in the C decoder when handling negative array/map block counts, specifically preventing signed overflow when negating INT64_MIN block counts during decoding.

Changes:

Update read_array_value() and read_map_value() to avoid signed overflow when negating negative block_count.
Add explicit invalid-block-count rejection during array/map decoding.
Add a new C regression test and wire it into the C test suite.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
lang/c/src/value-read.c	Changes array/map block-count negation to avoid UB and reject invalid negation results.
lang/c/tests/test_avro_4275.c	Adds regression coverage for INT64_MIN negation overflow and validates legitimate negative block-count decoding.
lang/c/tests/CMakeLists.txt	Registers the new regression test with the C test runner (and memcheck on UNIX).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

iemejia · 2026-07-11T08:18:16Z

+			/* Safe negation: avoid undefined behavior when
+			 * block_count == INT64_MIN, since -INT64_MIN is not
+			 * representable in int64_t (CWE-190). Use the
+			 * -(x+1)+1 idiom to negate without overflow. */
+			block_count = -(block_count + 1) + 1;
+			if (block_count <= 0) {
+				avro_set_error("Invalid array block count");
+				return EINVAL;
+			}


Good catch. The -(block_count + 1) + 1 idiom indeed still overflows when block_count == INT64_MIN because -(INT64_MIN + 1) yields INT64_MAX, and INT64_MAX + 1 is UB. Replaced with an explicit INT64_MIN guard followed by block_count = -block_count;.

iemejia · 2026-07-11T08:18:16Z

+			/* Safe negation: avoid undefined behavior when
+			 * block_count == INT64_MIN, since -INT64_MIN is not
+			 * representable in int64_t (CWE-190). Use the
+			 * -(x+1)+1 idiom to negate without overflow. */
+			block_count = -(block_count + 1) + 1;
+			if (block_count <= 0) {
+				avro_set_error("Invalid map block count");
+				return EINVAL;
+			}


Fixed the same way as the array path: explicit INT64_MIN guard + simple negation.

iemejia · 2026-07-11T08:18:17Z

+ *   03        = varint 3 = zigzag(-2) => block_count = -2
+ *   0E        = varint 14 = zigzag(7) => block_size = 7 bytes
+ *   02 61 14  = key "a" (len=1, 'a'), value int 10
+ *   02 62 28  = key "b" (len=1, 'b'), value int 20
+ *   00        = terminator
+ */
+static const char valid_neg_block_map[] = {
+    '\x03',                     /* block_count = -2 */
+    '\x0E',                     /* block_size = 7 */
+    '\x02', '\x61', '\x14',    /* key "a", value 10 */
+    '\x02', '\x62', '\x28',    /* key "b", value 20 */


You're right, the two entries total 6 bytes (each is: 1-byte string length varint + 1-byte key + 1-byte int varint = 3 bytes). Fixed the block_size from 0x0E (zigzag 7) to 0x0C (zigzag 6) and updated the comment to match.

…ation Replace the flawed -(block_count + 1) + 1 idiom (which still overflows on the final + 1 when block_count == INT64_MIN) with an explicit INT64_MIN guard followed by simple negation, in both read_array_value() and read_map_value(). Also fix the block_size in the map test vector from 7 to 6 to match the actual byte count of the encoded entries. Assisted-by: GitHub Copilot:claude-opus-4.6

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

iemejia · 2026-07-11T08:33:42Z

+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <stdint.h>
+#include <avro.h>
+


Good point. Updated to assert rc == EINVAL specifically, so the test will fail if the guard regresses and the error comes from elsewhere.

iemejia · 2026-07-11T08:33:47Z

+    /* This MUST fail gracefully (return error) rather than looping
+     * unboundedly or triggering undefined behavior. */
+    rc = avro_value_read(reader, &value);
+    if (rc == 0) {
+        fprintf(stderr,
+                "FAIL: INT64_MIN array block count was not rejected\n");
+        avro_reader_free(reader);
+        avro_value_decref(&value);
+        avro_value_iface_decref(iface);
+        avro_schema_decref(schema);
+        return 1;
+    }


Fixed — now checks for EINVAL explicitly and prints the actual rc on mismatch.

iemejia · 2026-07-11T08:33:47Z

+    /* This MUST fail gracefully. */
+    rc = avro_value_read(reader, &value);
+    if (rc == 0) {
+        fprintf(stderr,
+                "FAIL: INT64_MIN map block count was not rejected\n");
+        avro_reader_free(reader);
+        avro_value_decref(&value);
+        avro_value_iface_decref(iface);
+        avro_schema_decref(schema);
+        return 1;
+    }


Same fix applied here. Both INT64_MIN tests now assert EINVAL.

Assert that the INT64_MIN block count tests fail with the specific EINVAL error code rather than just any non-zero return. This ensures the tests actually exercise the new guard rather than passing due to an unrelated failure downstream. Addresses review feedback on PR apache#3842. Assisted-by: GitHub Copilot:claude-opus-4.6

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

iemejia · 2026-07-11T08:47:36Z

+			/* Reject INT64_MIN: its negation is not
+			 * representable in int64_t (CWE-190). */
+			if (block_count == INT64_MIN) {
+				avro_set_error("Invalid array block count");
+				return EINVAL;
+			}
+			block_count = -block_count;


Updated the PR description to accurately reflect the implementation: explicit INT64_MIN guard returning EINVAL followed by safe -block_count negation.

iemejia · 2026-07-11T08:47:37Z

+			/* Reject INT64_MIN: its negation is not
+			 * representable in int64_t (CWE-190). */
+			if (block_count == INT64_MIN) {
+				avro_set_error("Invalid array block count");
+				return EINVAL;
+			}
+			block_count = -block_count;


Good catch. Applied the same INT64_MIN guard + safe negation to skip_array/skip_map in datum_skip.c and read_array/read_map in consume-binary.c.

iemejia · 2026-07-11T08:47:38Z

+static const char int64min_block_count[] = {
+    '\xFF', '\xFF', '\xFF', '\xFF', '\xFF',
+    '\xFF', '\xFF', '\xFF', '\xFF', '\x01'
+};


Fixed — using explicit (char)0xFF casts to make the intent clear and avoid implementation-defined narrowing diagnostics on platforms with signed char.

Assert that the INT64_MIN block count tests fail with the specific EINVAL error code rather than just any non-zero return. This ensures the tests actually exercise the new guard rather than passing due to an unrelated failure downstream. Addresses review feedback on PR apache#3842. Assisted-by: GitHub Copilot:claude-opus-4.6

…arrowing Apply the same INT64_MIN overflow guard to block_count negation in datum_skip.c (skip_array, skip_map) and consume-binary.c (read_array, read_map) which used the unsafe 'block_count * -1' idiom. Also cast 0xFF byte constants to char explicitly in the test to avoid implementation-defined narrowing on platforms with signed char. Addresses review feedback on PR apache#3842. Assisted-by: GitHub Copilot:claude-opus-4.6

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

iemejia · 2026-07-11T09:07:08Z

 		if (block_count < 0) {
-			block_count = block_count * -1;
+			if (block_count == INT64_MIN) {
+				avro_set_error("Invalid map block count");
+				return EINVAL;
+			}


Fixed — same pattern: INT64_MIN check immediately after read_long(), before map_start_block.

iemejia · 2026-07-11T09:07:07Z

+			if (block_count == INT64_MIN) {
+				avro_set_error("Invalid array block count");
+				return EINVAL;


Fixed — moved the INT64_MIN guard to immediately after read_long(), before the first array_start_block callback.

iemejia · 2026-07-11T09:07:08Z

+			if (block_count == INT64_MIN) {
+				avro_set_error("Invalid array block count");
+				return EINVAL;
+			}
+			block_count = -block_count;


Fixed — added the same guard after the loop's read_long(), before the second array_start_block call.

iemejia · 2026-07-11T09:07:09Z

+			if (block_count == INT64_MIN) {
+				avro_set_error("Invalid map block count");
+				return EINVAL;
+			}
+			block_count = -block_count;


Fixed — guard added after the loop's read_long() as well. Both entry points are now protected.

In consume-binary.c, the array_start_block and map_start_block callbacks were being invoked with the raw block_count before the INT64_MIN validation. Move the guard to immediately after read_long() so that an invalid block count is rejected before reaching any consumer callback. Applies to both the initial block_count read and the subsequent reads at the end of each loop iteration. Assisted-by: GitHub Copilot:claude-opus-4.6

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

iemejia · 2026-07-11T09:14:44Z

@martin-g @RyanSkraba this is the C version of AVRO-4228 #3646 and AVRO-4228 #3843

@RyanSkraba this fix should be cherry-picked into the next 1.12 release

github-actions Bot added the C label Jul 11, 2026

iemejia requested a review from Copilot July 11, 2026 08:08

Copilot started reviewing on behalf of iemejia July 11, 2026 08:09 View session