Fix 64-bit offset/timestamp truncation on Windows in Admin/Uuid marshalling - #2325
Draft
Piotr WOLSKI (piochelepiotr) wants to merge 1 commit into
Draft
Conversation
…alling cfl_PyDict_SetLong() takes a C 'long', which librdkafka's int64_t offsets, timestamps, and rd_kafka_Uuid_t halves were being passed through. 'long' is 64 bits on Linux/macOS (LP64) but only 32 bits on Windows (LLP64), so any value above 2^31 silently wrapped to a negative number on Windows only. Adds cfl_PyDict_SetLongLong()/cfl_PyLong_FromLongLong(), backed by PyLong_FromLongLong(), and switches every call site that carries a 64-bit value: AdminClient.list_offsets() offset/timestamp, delete_records() low_watermark, and Uuid.most_significant_bits()/least_significant_bits(). Fixes confluentinc#1696
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
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.
What does this PR do?
Fixes a Windows-only bug where
AdminClient.list_offsets()/AdminClient.delete_records()results, andUuid.most_significant_bits()/Uuid.least_significant_bits(), silently truncate any value above2^31 - 1and can return a negative number.Root cause:
cfl_PyDict_SetLong()marshals a value into a Python dict via a parameter typedlong. On Linux/macOS (LP64),longis 64 bits, so this is harmless there. On Windows (LLP64),longis always 32 bits, even in a 64-bit build. Every current call site passes a genuinely 64-bitint64_tvalue straight from librdkafka (rd_kafka_topic_partition_t.offset,ListOffsetstimestamps,rd_kafka_Uuid_thalves) through this 32-bit-only helper, so on Windows the value wraps modulo2**32and gets reinterpreted as signed.Fix
Adds
cfl_PyDict_SetLongLong()(backed byPyLong_FromLongLong, correctly 64-bit on every platform) and switches all fivecfl_PyDict_SetLong()call sites that carry a 64-bit value, plus the two directcfl_PyLong_FromLong()calls in theUuidgetters, to the new helper.cfl_PyDict_SetLong()itself is left as-is (doc comment added) in case a genuinely 32-bit-safe caller is ever needed; nothing currently calls it for a value that needs to be 64-bit.Motivation
A production incident: a Windows host on kafka_consumer (a Datadog Agent integration) reported
broker_offset = -1533701557where the true high-watermark offset was2761265739— exactlytrue - 2**32.TopicPartition.offset(a separate, already 64-bit-safe code path usingPyLong_FromLongLong, seeconfluent_kafka.c:513) reported the correct value in the same run, which is what localized the defect to this one marshalling helper.This also affects
Uuid.most_significant_bits()/least_significant_bits(),AdminClient.list_offsets()timestamps, anddelete_records()'slow_watermark— any consumer of these on Windows can silently receive corrupted data above ~2.1 billion.Relates to #1696, which reports the same symptom (a negative
ListOffsetstimestamp on Windows) but was never triaged to a root cause.Reproduction
The bug only manifests where
sizeof(long) == 4(Windows LLP64, or any ILP32 platform). It doesn't reproduce on the Linux x64 / macOS CI this project runs functional tests on, which is presumably why it shipped unnoticed — the Windows CI job only builds wheels, it doesn't execute the test suite (.semaphore/semaphore.yml, "Wheels: Windows" job).To reproduce without a Windows machine, the two files below extract the exact
cfl_PyDict_SetLong()implementation fromconfluent_kafka.c(before) and the newcfl_PyDict_SetLongLong()(after) verbatim, feed in the real incident offset (2761265739), and run them in a 32-bit-longenvironment (i386 Debian container — same ABI class as Windows x64 for this purpose):-1533701557is an exact match for the incident report. Running the same offset through the patchedcfl_PyDict_SetLongLong()(backed byPyLong_FromLongLong) in the identical i386 container:I'm attaching a zip with both repro files (
repro.c,repro_fixed.c) and their Dockerfiles so this can be re-run directly. Happy to also open a separate suggestion issue about adding an actual test-execution step to the Windows CI job, since that's the gap that let this ship — didn't want to bundle that into this fix.Checklist
sizeof(long) == 4(see above)sizeof(long) == 8(native macOS run, unaffected)sizeof(long) == 4, which none of this project's CI targets exercise a test run on (see Reproduction section); a Python-level unit test on the existing Linux/macOS CI would pass identically before and after this fix and wouldn't guard against a regression