conncache: don't assume curl_off_t increment wrap-around

[bagder]

In the totally unlikely event that next_easy_id would ever wrap, avoid undefined behavior. It is a signed type.

Also: update the related comment in urldata.h that mentioned LONG_MAX but is nowadays CURL_OFF_T_MAXa

[Copilot]

Pull request overview

This PR aims to prevent undefined behavior in the connection cache transfer ID assignment by avoiding signed curl_off_t overflow if the per-pool next_easy_id counter ever reaches its maximum value, and updates documentation to match the current curl_off_t limits.

Changes:

• Updates the easy handle id comment to reference CURL_OFF_T_MAX wrap-around behavior.
• Adjusts Curl_cpool_xfer_init() logic intended to reset next_easy_id when the maximum curl_off_t value is reached.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
lib/urldata.h	Updates the Curl_easy::id wrap-around comment to use CURL_OFF_T_MAX.
lib/conncache.c	Changes the next_easy_id wrap handling in Curl_cpool_xfer_init().

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

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

Suppressed comments (1)

lib/conncache.c:278

• cpool->next_easy_id++ still evaluates a post-increment on a signed curl_off_t before any guard. If next_easy_id is ever CURL_OFF_T_MAX (e.g. due to initialization/state corruption), this increment is signed overflow (UB). Also, with the current logic data->id can never become CURL_OFF_T_MAX, which contradicts the updated comment in urldata.h about wrapping after CURL_OFF_T_MAX. Consider rewriting to avoid incrementing at the maximum value and to wrap only after returning CURL_OFF_T_MAX.

    /* the identifier inside the connection cache */
    data->id = cpool->next_easy_id++;
    if(cpool->next_easy_id == CURL_OFF_T_MAX)
      cpool->next_easy_id = 0;

[testclutch]

Analysis of PR #22569 at e518d476:

Test 2502 failed, but it has been 11.5% flaky lately, so it's probably NOT a fault of the PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (3, to be specific) so it may be that this failure is rather a systemic issue with this job and not with this specific PR.

Test 1700 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them).

Test 2501 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them).

Test 2100 failed, but it has been 3.6% flaky lately, so it's probably NOT a fault of the PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them).

Test 3017 failed, but it has been 54.5% flaky lately, so it's probably NOT a fault of the PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (2, to be specific) so it may be that this failure is rather a systemic issue with this job and not with this specific PR.

Generated by Testclutch