Skip to content

urlapi: fix memleaks on error in parse_hostname_login()#21879

Closed
vszakats wants to merge 4 commits into
curl:masterfrom
vszakats:possible-leak
Closed

urlapi: fix memleaks on error in parse_hostname_login()#21879
vszakats wants to merge 4 commits into
curl:masterfrom
vszakats:possible-leak

Conversation

@vszakats

@vszakats vszakats commented Jun 5, 2026

Copy link
Copy Markdown
Member

Detected by GitHub Code Quality

Follow-up to acd82c8 #11006
Follow-up to 4183b8f #8049


  • try adding a unit test.

@vszakats vszakats marked this pull request as draft June 5, 2026 14:40
@github-actions github-actions Bot added the URL label Jun 5, 2026
@bagder

bagder commented Jun 5, 2026

Copy link
Copy Markdown
Member

It would be ideal to also get a test case that reproduces this leak...

@vszakats vszakats changed the title urlapi: avoid possible leaks on error in parse_hostname_login() urlapi: avoid memleaks on error in parse_hostname_login() Jun 5, 2026
@vszakats vszakats requested a review from Copilot June 6, 2026 00:27
@vszakats vszakats marked this pull request as ready for review June 6, 2026 00:27

Copilot AI left a comment

Copy link
Copy Markdown

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 addresses a potential memory leak in parse_hostname_login() within curl’s URL parsing code by ensuring already-set credential fields are freed on error paths, and adds a unit test to cover the behavior.

Changes:

  • Update parse_hostname_login() error handling to free u->user, u->password, and u->options (preventing leaks when those fields were previously allocated).
  • Expose parse_hostname_login() under the existing UNITTEST mechanism and annotate it for unit testing.
  • Add unit coverage in unit1675 for login parsing outcomes (including scheme-dependent ;options parsing and CURLU_DISALLOW_USER).

Reviewed changes

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

File Description
lib/urlapi.c Frees existing credential fields on parse_hostname_login() error paths; makes function available for unit tests.
tests/unit/unit1675.c Adds unit tests exercising parse_hostname_login() credential parsing and error behavior.

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

Comment thread tests/unit/unit1675.c
@vszakats vszakats changed the title urlapi: avoid memleaks on error in parse_hostname_login() urlapi: fix memleaks on error in parse_hostname_login() Jun 6, 2026
@vszakats vszakats closed this in 7c34365 Jun 7, 2026
@vszakats vszakats deleted the possible-leak branch June 7, 2026 22:44
dkarpov1970 pushed a commit to dkarpov1970/curl that referenced this pull request Jun 9, 2026
Detected by GitHub Code Quality

Follow-up to acd82c8 curl#11006
Follow-up to 4183b8f curl#8049

Closes curl#21879
dkarpov1970 pushed a commit to dkarpov1970/curl that referenced this pull request Jun 10, 2026
Detected by GitHub Code Quality

Follow-up to acd82c8 curl#11006
Follow-up to 4183b8f curl#8049

Closes curl#21879
vszakats added a commit that referenced this pull request Jun 10, 2026
Adjust code to avoid `-Wformat-signedness` warnings, while making sure
that enums are always cast to a known type when passing them to `printf`
functions, to support compilers and compiler settings where enums are
not default-size signed ints.

- cast integers printed as hex to `unsigned`. (63 times, 20 of them in
  `mbedtls.c`)
- cast misc enums to `int` for printing. (31 times)
- cast `CURL_LOCK_DATA_*` enums to `int`. (4 times)
- cast `CURL_FORMADD_*` enums to `int`. (13 times)
- cast `CURLSHE_*` enums to `int`. (3 times)
- cast `CURLUE_*` enums to `int`. (33 times)
- cast `CURLMSG_*` enums to `int`. (6 times)
- cast `CURLE_*` enums to `int`. (~380 times)
- unit1675: fix mask.
  Follow-up to 7c34365 #21879

Ref: #18343 (initial attempt)

Closes #20848
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants