[lts_03_2025] Use safe_add_size_t when sizing DPS transport response buffers (backport of #2738) - #2746
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
The C-canary pipeline on this branch cannot run at all. Every Linux job pulls its container from csdkcontainerregistry.azurecr.io via the csdk-containers endpoint, and runs on the sdk-c--* self-hosted pools. Both the registry and those pools were retired; main moved off them in #2701. On this branch the first job of the Setup stage therefore fails in "Initialize containers" with: Could not fetch access token for ACR. Please configure Managed Service Identity (MSI) for Azure Container Registry with the appropriate permissions. Setup gates everything, so all downstream stages are skipped and any PR targeting lts_03_2025 is red before a line is compiled. Observed on build 162517 (PR #2746), which failed 14s in. Bring the CI configuration to parity with main by taking main's version of the pipeline and the build/test scripts it drives. This corresponds to the eight pipeline commits made on main since this branch diverged: c59df07 Update CODEOWNERS and ubuntu-20.04 runner pool (deprecated) (#2677) 5f11fab Run Cleanup stage even if cancelled (#2682) c5d43cf Replace pipeline with Microsoft-hosted agents (#2701) 891fa30 Self-sufficient hosted-agent optimizations (#2718) 1317b57 Make AZURE_LOCATION a runtime parameter (#2717) 6473693 Replace Node.js check_submodules with PowerShell (#2721) fa6ad5d Disable NTLM in linux-setup-raspberry.sh (#2733) c86b2d6 Make CodeQL opt-in per job (#2743) Taken as an end state rather than eight cherry-picks: the intermediate commits conflict with each other in build_all/linux/run_tests.sh, and the only state that matters is the one main runs green today. Every file changed here is CI or test infrastructure. No product source, no submodule pointers, and no version change. Audited against this branch's tree before committing: - all build_all/, jenkins/, testtools/ and samples/ paths the pipeline invokes exist here - the cmake options it passes resolve the same way they do on main (enable_ipv6 is a c-utility option on both; -DpsX509IndividualEnrollments is a PowerShell parameter, not a cmake one) - the YAML parses: 4 stages, 30 jobs, no remaining azurecr.io reference Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CI on this PR is blocked by broken infrastructure on
|
My previous commit took samples/dockerbuilds/myapp/CMakeLists.txt from main along with the pipeline files. That file is not CI infrastructure, and main's version links c_logging_v2: target_link_libraries(myapp ... aziotsharedutil c_logging_v2 parson ...) main can do that because its root CMakeLists.txt add_subdirectory()s deps/c-logging and installs the v2 target. This branch has no c-logging wiring in its root CMakeLists.txt at all, so libc_logging_v2 is never built or installed here and the cross-compile samples fail at link: ld: cannot find -lc_logging_v2 Observed on build 162518 in both Cross Compile (ARM) and Cross Compile (MIPS32). Restore this branch's line, which links parson and not c_logging_v2. Note both branches pin deps/c-logging at the same commit (504e7d47); the difference is purely that main wires it into the build and this branch does not. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Local build and test verification — now doneI previously said this PR was unvalidated. I have since built a toolchain in my environment (gcc 12.2, cmake 3.25) and built and ran all three affected unit-test suites. Results:
196 tests, 0 failures. This confirms the The three new regression tests each ran and passed: The new tests are meaningful, not vacuousTo prove the tests actually exercise the vulnerable path, I reverted only the three Restoring the fix returns all three suites to green. So the tests genuinely detect the wrap-then-overflow, and the fix genuinely prevents it. Caveat on severity — unchangedThis demonstrates the pattern is dangerous once a length of CICI here will stay red until #2747 merges — this branch still carries the old pipeline that fails in |
There was a problem hiding this comment.
Pull request overview
Backport of mainline hardening to the lts_03_2025 branch to prevent size_t overflow when allocating DPS transport response buffers (HTTP/MQTT/AMQP), plus regression unit tests to ensure oversized lengths are rejected instead of causing heap corruption.
Changes:
- Replace
malloc(len + 1)sizing withsafe_add_size_t(len, 1)checks in DPS HTTP/MQTT/AMQP receive paths. - Add one regression UT per transport that injects a saturating
(size_t)-1length and asserts the error path is taken (no overflow/corruption). - Minor LTS test adaptation: use
IGNORED_PTR_ARGinstead ofIGNORED_ARGfor compatibility with the pinned umock-c.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| provisioning_client/src/prov_transport_http_client.c | Uses safe_add_size_t for reply buffer sizing and avoids dangling payload_data on free. |
| provisioning_client/src/prov_transport_mqtt_common.c | Uses safe_add_size_t for MQTT payload buffer sizing and rejects saturating lengths. |
| provisioning_client/src/prov_transport_amqp_common.c | Uses safe_add_size_t for AMQP message body buffer sizing and rejects saturating lengths. |
| provisioning_client/tests/prov_transport_http_client_ut/prov_transport_http_client_ut.c | Adds regression test for saturating content_len in HTTP receive callback. |
| provisioning_client/tests/prov_transport_mqtt_common_ut/prov_transport_mqtt_common_ut.c | Adds regression test for saturating MQTT payload length and validates error surfacing. |
| provisioning_client/tests/prov_transport_amqp_common_ut/prov_transport_amqp_common_ut.c | Adds regression test for saturating AMQP body length and validates error surfacing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (alloc_size == SIZE_MAX) | ||
| { | ||
| LogError("Failure sending http request"); | ||
| LogError("Failure invalid content length specified"); |
There was a problem hiding this comment.
Agreed on the substance; not changing it in this PR.
You are right that the value is not caller-specified. content_len is the length reported by the receive callback — in azure-uhttp-c it is BUFFER_length(recv_msg.msg_body), i.e. bytes actually received and buffered, not the Content-Length header. That distinction is exactly why this is hardening rather than a remotely reachable overflow, so the wording is worth correcting.
The reason not to do it here: this string is byte-identical to main, and "the three provisioning_client/src files are byte-identical to main at the allocation sites" is the main safety argument for this backport. Changing it would make the branches diverge and leave the same wording wrong on main.
Better fixed on main and allowed to flow down. Happy to open that PR.
Copying main's build_all/docs/Doxyfile carried its PROJECT_NUMBER of 1.15.0 onto this branch, whose iothub_client_version.h is 1.14.0. That contradicts the stated scope of this change (no version bump) and made the generated docs disagree with the SDK version. The Doxyfile is not referenced by the pipeline, so it had no CI value here. Restored to 1.14.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fetch the E2E helper module from master
Both call sites pulled Azure.Iot.Sdk.Test.psm1 from a personal feature
branch on iot-sdks-e2e-fx. That branch never had a pull request, is 43
commits behind master and 3 ahead, so CI depended on a stale dead end that
could vanish at any time. Point both at master.
All three commits unique to that branch are superseded on master:
- "install stable azure-iot CLI extension by default" -> master installs a
pinned extension version (0.30.0b2) via Install-AzureIotCliExtension,
which is strictly more reproducible than selecting whatever --allow-preview
resolves to.
- "use .pem for temp cert files" -> present on master.
- "--login on enrollment commands" -> that workaround existed because
--allow-preview drifted onto an untested extension build; pinning the
version removes the cause.
Verified master still satisfies this pipeline: New-AzureResourceGroupName,
New-AzIotTestEnvironment, New-AzIotCSDKE2ETestConfig and
Test-SubmoduleConsistency all exist, are exported, and accept every
parameter used here.
echo the home directory instead of executing it
A bare $HOME ran the home directory as a command, logging
"Is a directory" and returning 126. Harmless only because the step has no
set -e and $HOME was not the last command.
Make the raspberry toolchain setup idempotent
mkdir $WORK_ROOT failed on any re-run; mkdir -p and quoting fix that.
pushd /${WORK_ROOT}/... had a stray leading slash that produced a
double-slash path and would break outright if WORK_ROOT were relative.
Reject and correct the broken run_tests.sh flag combinations
Three invocations misbehaved silently:
--e2e-only ran nothing and exited 0 (false green)
--e2e --ut-only ran nothing and exited 0 (false green)
--e2e --e2e-only ignored --e2e-only and ran unit tests too
--e2e-only now requires --e2e, and the plain branch honours both flags.
Verified the six invocations this pipeline actually uses produce byte
identical ctest commands before and after.
Make resource group cleanup idempotent
az group delete exits 3 when the group is absent, and the step runs under
set -e. Retrying any job re-runs the Cleanup stage, so a build whose first
cleanup succeeded went red on retry with ResourceGroupNotFound. This was
observed on the previous run of this branch. Check for the group first.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Ubuntu 24.04 ARM64 job intermittently failed in
"Install aarch64 cross toolchain and arm64 dev libs" with apt exit 100:
libssl-dev:arm64 : Depends: libssl3t64:arm64 (= 3.0.13-0ubuntu3.12)
but it is not going to be installed
E: Unable to correct problems, you have held broken packages.
libssl3t64, libcurl4t64 and zlib1g are Multi-Arch: same, so the arm64 copy
must be the exact version of the amd64 copy already on the runner image.
ports.ubuntu.com and archive.ubuntu.com are not always in sync; when they
drift apt rejects the whole transaction.
That the drift, not the tree, is the trigger is visible in the versions:
one failure demanded 3.0.13-0ubuntu3.15 and a later one 3.0.13-0ubuntu3.12,
with no change in between, and a plain retry of the first one passed.
Name both arches for the three Multi-Arch: same runtime libraries so apt can
settle on a version present in both archives, and allow downgrades so it can
move the amd64 copy back when ports is the side that lags.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
build/.horton-e2e.yml on this branch predates two changes that have since happened outside it, and cannot run as written: 1. It names the service connection 'GitHub OAuth - az-iot-builder-01'. That endpoint no longer exists in the azure-iot-sdks project; the GitHub connection is now 'azure-iot-sdk-python-github'. Resource resolution fails before any job starts. 2. It declares `jobs:` with jobs-gate-c.yaml directly, with no setup stage. That template is consumed from iot-sdks-e2e-fx@refs/heads/master, which is a moving ref, and it now begins with steps-load-test-config.yaml. That step does `download: current / artifact: test_config`, published by steps-create-azure-resources.yaml. With no setup stage nothing publishes that artifact, so every job fails on the download. Take main's version, which adds the setup and cleanup stages around the gate and uses the endpoint that exists. The file references only e2e-fx templates and no paths inside this repo, so it is branch agnostic; the SDK itself is cloned by the gate from Horton.Repo at Horton.Commit. Note this file alone does not make the gate run for this branch: the horton-c-gate pipeline's pull request trigger is filtered to +main, so PRs targeting lts_03_2025 are reported as skipped. That filter has to be widened separately for the gate to execute here. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The horton-c-gate pull request trigger was widened to +main,+lts_* so the gate applies to LTS branches. Branch filters are evaluated when a pull request event fires, so the already-reported skip on this PR does not re-evaluate on its own. Empty commit to raise a new event. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Retrying only the failed helgrind E2E job could not succeed: the Cleanup stage runs on build completion with always(), so it had already deleted the resource group before the retry started. A full run re-executes Setup and provisions resources first. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Use safe_add_size_t when sizing DPS transport response buffers The DPS HTTP, MQTT and AMQP transports sized their response buffers with malloc(len + 1). Compute the allocation size with safe_add_size_t (as already done elsewhere in the tree, e.g. uhttp) and fail the request if the addition saturates, instead of allocating a mis-sized buffer. Add a regression unit test per transport covering the saturating-length path. * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Address review feedback - prov_transport_mqtt_common: drop duplicate http_proxy_io.h include - prov_transport_http_client: correct payload-allocation error message - amqp unit test: assert the error is surfaced to the register callback --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> (cherry picked from commit 70f9fa2)
The cherry-picked regression tests use IGNORED_ARG, which is only defined in newer umock-c. lts_03_2025 pins umock-c 504193e6 (Mar 2020), where that macro does not exist, so the three provisioning UT suites would fail to compile. Replace IGNORED_ARG with IGNORED_PTR_ARG, matching the macro already used throughout these test files on this branch. All nine occurrences are pointer arguments, so IGNORED_PTR_ARG is the correct substitution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a61e236 to
4a8a59f
Compare
Rebased onto the migrated pipeline — CI can now run hereThis PR was red for one reason: its branch predated the pipeline migration, so every run died 14s in at Rebased onto The backport itself is unchanged — the rebase was clean and the diff against #2747's branch is exactly the same 6 files: The file count shown against Re-verified locally after the rebase — built and ran all three suites:
196 tests, 0 failures. Merge #2747 first, then this. |
C-canary green after the rebase — 35/35 jobsBuild 162661 succeeded: all 35 jobs, no retries. Previously this PR never got past Remaining red is Not caused by this PR, and not fixable from this repo: the gate's Same failure and same fix on #2747. |
|
/azp run horton-c-gate |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run horton-c-gate |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Gate green. The earlier |
Backports #2738 to
lts_03_2025, the only LTS branch still in maintenance (end date 2026-10-07 perreadme.md).Why
The DPS transports size their response buffers with
malloc(len + 1), so alenofSIZE_MAXwraps tomalloc(0)and the followingmemcpyruns off the allocation. #2738 fixed this onmainon 2026-07-23; the LTS branch still carries the unhardened pattern in all three files.Contents
70f9fa28cherry-picked unchanged. The threeprovisioning_client/srcfiles are byte-identical tomainat the allocation sites.IGNORED_ARG, which does not exist in the umock-c pinned by this branch (504193e6, March 2020). Without this the three provisioning UT suites fail to compile. All nine occurrences are pointer arguments, so they becomeIGNORED_PTR_ARG, matching the macro already used throughout these files here.Verification
Built and ran all three affected suites locally:
prov_transport_http_client_utprov_transport_mqtt_common_utprov_transport_amqp_common_utThe three new regression tests each ran and passed.
To confirm they are not vacuous, I reverted only the three
srcfiles to the unfixedmalloc(len + 1)and reran — all three suites crashed with heap corruption (munmap_chunk(): invalid pointer,malloc(): unaligned tcache chunk detected,malloc(): memory corruption (fast), core dumped). Restoring the fix returns them to green.Reachability
Hardening / defence in depth. I could not find a path by which a wire-provided length reaches a wrapping value:
content_lenisBUFFER_length(recv_msg.msg_body)in azure-uhttp-c, i.e. bytes actually received and buffered, not theContent-Lengthheader. uhttp errors on mismatch.payload->lengthcomes from the umqtt codec, whereprepareheaderDataInfo()rejectstotalLen > MAX_SEND_SIZEand Remaining Length fields longer than 4 bytes.binary_data.lengthoriginates fromamqp_binary.length, auint32_t; on 64-bituint32 + 1cannot wrapsize_t.The crash above is produced by test mocks injecting
SIZE_MAXdirectly.Notes
772a4f8b) already providessafe_add_size_t, macro identical tomain.5a48d3fd8).payload_data = NULLafterfree(), so the malloc-failure branch no longer leaves a dangling pointer.http_proxy_io.hinclude dropped fromprov_transport_mqtt_common.cwas genuinely duplicated here too (lines 17 and 19); one remains.