Skip to content

Fix tool/library memory management/mixup issues#21099

Closed
bagder wants to merge 4 commits intomasterfrom
bagder/global-mem-testing
Closed

Fix tool/library memory management/mixup issues#21099
bagder wants to merge 4 commits intomasterfrom
bagder/global-mem-testing

Conversation

@bagder
Copy link
Copy Markdown
Member

@bagder bagder commented Mar 26, 2026

memory allocated by libcurl must be freed with curl_free(),

memory allocated by the tool itself must be freed with curlx_free().

@github-actions github-actions bot added cmdline tool CI Continuous Integration labels Mar 26, 2026
@testclutch

This comment was marked as outdated.

@github-actions github-actions bot added the tests label Mar 26, 2026
@bagder bagder requested a review from Copilot March 26, 2026 11:50
@bagder bagder marked this pull request as ready for review March 26, 2026 11:50
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to prevent allocator “mixups” between libcurl-owned memory (must be freed with curl_free()) and tool-owned memory (must be freed with curlx_free()), and introduces an opt-in debug mode to help detect such mistakes.

Changes:

  • Fix several mismatched curl_free()/curlx_free() call sites and normalize ownership transfers (dup into tool memory when needed).
  • Add --enable-init-mem-debug / CURL_DEBUG_GLOBAL_MEM mode to run the curl tool using curl_global_init_mem() with custom allocators and expose this in curl -V.
  • Add a new CI job to exercise the debug mode under valgrind.

Reviewed changes

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

Show a summary per file
File Description
tests/runtests.pl Tightens Debug feature detection to avoid accidental matches from new “global-mem-debug” wording.
src/var.c Frees base64-encoded/decoded buffers with curlx_free() (tool allocator), not curl_free().
src/tool_operhlp.c Uses curl_free() for libcurl-allocated strings and transfers URL ownership into tool memory.
src/tool_operate.c Ensures URL/header/outfile strings end up tool-owned when later freed by tool helpers.
src/tool_help.c Adds “global-mem-debug” to the tool’s feature list when enabled.
src/tool_cfgable.c Adds debug-only curl_global_init_mem() wiring with custom allocators.
src/tool_cb_hdr.c Avoids curl_maprintf for header cloning; uses curlx_memdup0 and tool frees.
lib/curlx/dynbuf.c Frees curl_mvaprintf() output with curl_free() (libcurl allocator).
configure.ac Adds --enable-init-mem-debug and defines CURL_DEBUG_GLOBAL_MEM.
CMakeLists.txt Introduces a CMake option for the feature (currently not wired into compilation).
.github/workflows/linux.yml Adds a valgrind CI job enabling the new init-mem debug mode.

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

@bagder bagder force-pushed the bagder/global-mem-testing branch 2 times, most recently from ef4ee4c to 4fbf8dc Compare March 26, 2026 12:32
bagder added 4 commits March 26, 2026 23:45
Build with "configure --enable-init-mem-debug" to make the tool use
curl_global_init_mem() and a set of private memory funtion callbacks for
libcurl's memory management.

Using this setup, memory mixups in tool code is more likely to cause
crashes and thus get discovered while running tests.

This curl_global_init_mem debug mode can only be done when building
libcurl shared (not static) and without debugging enabled - since it
needs to use the custom memory funtion callbacks.
memory allocated by libcurl must be freed with curl_free() and vice versa,
memory allocated by the tool itself must be freed with curlx_free().

- dynbuf: free libcurl data with curl_free()
- tool_operate: make sure we get URL using the right memory
- tool_operhlp: free libcurl memory with curl_free()
- tool_operate: free curl_maprintf() pointer with curl_free
- var: data from curlx_base64_decode needs curlx_free
- tool_operate: fix memory juggling in etag handling
- tool_cb_hdr: fix memory area mixups
- tool_operate: another mixup in etag management
- tool_cb_hdr: more memory mixup fixes
- tool_cfgable.c: document some details
- tool_help: show global-mem-debug in -V output
For both TrackMemory and Debug
@bagder bagder force-pushed the bagder/global-mem-testing branch from 4fbf8dc to f1d8860 Compare March 26, 2026 22:45
@bagder
Copy link
Copy Markdown
Member Author

bagder commented Mar 26, 2026

augment review

@augmentcode
Copy link
Copy Markdown

augmentcode bot commented Mar 26, 2026

🤖 Augment PR Summary

Summary: This PR tightens separation between tool-owned allocations and libcurl-owned allocations, aiming to prevent (and actively detect) mismatched free calls.

Changes:

  • Adjust several call sites to free libcurl-allocated strings with curl_free() and tool-allocated strings with curlx_free()/tool_safefree().
  • When APIs return libcurl-allocated buffers (e.g., URL API, maprintf helpers), duplicate into tool memory where needed and release the original with curl_free().
  • Add an optional debug build mode that initializes libcurl via curl_global_init_mem() with custom allocators (CURL_DEBUG_GLOBAL_MEM / --enable-init-mem-debug) to flush out mixups.
  • Extend curl -V feature output to advertise the new "global-mem-debug" marker when enabled.
  • Update the test harness feature parsing to avoid falsely detecting Debug builds due to the new "global-mem-debug" feature name.
  • Add a CI job running this mode under valgrind to catch allocator/free mismatches.

Technical Notes: The added mode is intended for diagnostics and depends on using a shared libcurl (per the new in-code notes).

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

@bagder bagder closed this in 59c1107 Mar 27, 2026
bagder added a commit that referenced this pull request Mar 27, 2026
bagder added a commit that referenced this pull request Mar 27, 2026
memory allocated by libcurl must be freed with curl_free() and vice versa,
memory allocated by the tool itself must be freed with curlx_free().

- dynbuf: free libcurl data with curl_free()
- tool_operate: make sure we get URL using the right memory
- tool_operhlp: free libcurl memory with curl_free()
- tool_operate: free curl_maprintf() pointer with curl_free
- var: data from curlx_base64_decode needs curlx_free
- tool_operate: fix memory juggling in etag handling
- tool_cb_hdr: fix memory area mixups
- tool_operate: another mixup in etag management
- tool_cb_hdr: more memory mixup fixes
- tool_cfgable.c: document some details
- tool_help: show global-mem-debug in -V output

Closes #21099
bagder added a commit that referenced this pull request Mar 27, 2026
For both TrackMemory and Debug

Closes #21099
@bagder bagder deleted the bagder/global-mem-testing branch March 27, 2026 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI Continuous Integration cmdline tool tests

Development

Successfully merging this pull request may close these issues.

3 participants