parse_proxy(): fix memory leak in case of invalid proxy server name #1761

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@rouault
Contributor

rouault commented Aug 11, 2017

Fixes the below leak:

$ valgrind --leak-check=full ~/install-curl-git/bin/curl --proxy "http://a:b@/x" http://127.0.0.1
==5048== Memcheck, a memory error detector
==5048== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==5048== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==5048== Command: /home/even/install-curl-git/bin/curl --proxy http://a:b@/x http://127.0.0.1
==5048==
curl: (5) Couldn't resolve proxy name
==5048==
==5048== HEAP SUMMARY:
==5048== in use at exit: 532 bytes in 12 blocks
==5048== total heap usage: 5,288 allocs, 5,276 frees, 445,271 bytes allocated
==5048==
==5048== 2 bytes in 1 blocks are definitely lost in loss record 1 of 12
==5048== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5048== by 0x4E6CB79: parse_login_details (url.c:5614)
==5048== by 0x4E6BA82: parse_proxy (url.c:5091)
==5048== by 0x4E6C46D: create_conn_helper_init_proxy (url.c:5346)
==5048== by 0x4E6EA18: create_conn (url.c:6498)
==5048== by 0x4E6F9B4: Curl_connect (url.c:6967)
==5048== by 0x4E86D05: multi_runsingle (multi.c:1436)
==5048== by 0x4E88432: curl_multi_perform (multi.c:2160)
==5048== by 0x4E7C515: easy_transfer (easy.c:708)
==5048== by 0x4E7C74A: easy_perform (easy.c:794)
==5048== by 0x4E7C7B1: curl_easy_perform (easy.c:813)
==5048== by 0x414025: operate_do (tool_operate.c:1563)
==5048==
==5048== 2 bytes in 1 blocks are definitely lost in loss record 2 of 12
==5048== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5048== by 0x4E6CBB6: parse_login_details (url.c:5621)
==5048== by 0x4E6BA82: parse_proxy (url.c:5091)
==5048== by 0x4E6C46D: create_conn_helper_init_proxy (url.c:5346)
==5048== by 0x4E6EA18: create_conn (url.c:6498)
==5048== by 0x4E6F9B4: Curl_connect (url.c:6967)
==5048== by 0x4E86D05: multi_runsingle (multi.c:1436)
==5048== by 0x4E88432: curl_multi_perform (multi.c:2160)
==5048== by 0x4E7C515: easy_transfer (easy.c:708)
==5048== by 0x4E7C74A: easy_perform (easy.c:794)
==5048== by 0x4E7C7B1: curl_easy_perform (easy.c:813)
==5048== by 0x414025: operate_do (tool_operate.c:1563)

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=2984
Credit to OSS Fuzz for discovery

parse_proxy(): fix memory leak in case of invalid proxy server name
Fixes the below leak:

$ valgrind --leak-check=full ~/install-curl-git/bin/curl --proxy "http://a:b@/x" http://127.0.0.1
==5048== Memcheck, a memory error detector
==5048== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==5048== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==5048== Command: /home/even/install-curl-git/bin/curl --proxy http://a:b@/x http://127.0.0.1
==5048==
curl: (5) Couldn't resolve proxy name
==5048==
==5048== HEAP SUMMARY:
==5048==     in use at exit: 532 bytes in 12 blocks
==5048==   total heap usage: 5,288 allocs, 5,276 frees, 445,271 bytes allocated
==5048==
==5048== 2 bytes in 1 blocks are definitely lost in loss record 1 of 12
==5048==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5048==    by 0x4E6CB79: parse_login_details (url.c:5614)
==5048==    by 0x4E6BA82: parse_proxy (url.c:5091)
==5048==    by 0x4E6C46D: create_conn_helper_init_proxy (url.c:5346)
==5048==    by 0x4E6EA18: create_conn (url.c:6498)
==5048==    by 0x4E6F9B4: Curl_connect (url.c:6967)
==5048==    by 0x4E86D05: multi_runsingle (multi.c:1436)
==5048==    by 0x4E88432: curl_multi_perform (multi.c:2160)
==5048==    by 0x4E7C515: easy_transfer (easy.c:708)
==5048==    by 0x4E7C74A: easy_perform (easy.c:794)
==5048==    by 0x4E7C7B1: curl_easy_perform (easy.c:813)
==5048==    by 0x414025: operate_do (tool_operate.c:1563)
==5048==
==5048== 2 bytes in 1 blocks are definitely lost in loss record 2 of 12
==5048==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5048==    by 0x4E6CBB6: parse_login_details (url.c:5621)
==5048==    by 0x4E6BA82: parse_proxy (url.c:5091)
==5048==    by 0x4E6C46D: create_conn_helper_init_proxy (url.c:5346)
==5048==    by 0x4E6EA18: create_conn (url.c:6498)
==5048==    by 0x4E6F9B4: Curl_connect (url.c:6967)
==5048==    by 0x4E86D05: multi_runsingle (multi.c:1436)
==5048==    by 0x4E88432: curl_multi_perform (multi.c:2160)
==5048==    by 0x4E7C515: easy_transfer (easy.c:708)
==5048==    by 0x4E7C74A: easy_perform (easy.c:794)
==5048==    by 0x4E7C7B1: curl_easy_perform (easy.c:813)
==5048==    by 0x414025: operate_do (tool_operate.c:1563)

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=2984
Credit to OSS Fuzz for discovery
@bagder

bagder approved these changes Aug 11, 2017

I approve! Even though I can't view the oss-fuzz bug...

@bagder

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Aug 11, 2017

Member

I also wrote up a new test case (1447) that verifies your fix and reproduces the leak without it.

Member

bagder commented Aug 11, 2017

I also wrote up a new test case (1447) that verifies your fix and reproduces the leak without it.

@rouault

This comment has been minimized.

Show comment Hide comment
@rouault

rouault Aug 11, 2017

Contributor

Even though I can't view the oss-fuzz bug...

Yes, it is embargoed for now for people not in the GDAL team. Anyway the report wasn't that great (the reproducer file didn't match the report). I just got the leak stracktrace, and looking at curl code, reverse engineered a likely reproducer...

FYI, some GDAL files (virtual raster files written as XML) can contain reference to extended filenames, such as /vsicurl/proxy=http://a:b@/x,url=http://foo that contains the curl URL and a subset of possible curl options. So oss-fuzz can end up crafting such extended filenames when messing with the XML file, discovering those curl issues.

Contributor

rouault commented Aug 11, 2017

Even though I can't view the oss-fuzz bug...

Yes, it is embargoed for now for people not in the GDAL team. Anyway the report wasn't that great (the reproducer file didn't match the report). I just got the leak stracktrace, and looking at curl code, reverse engineered a likely reproducer...

FYI, some GDAL files (virtual raster files written as XML) can contain reference to extended filenames, such as /vsicurl/proxy=http://a:b@/x,url=http://foo that contains the curl URL and a subset of possible curl options. So oss-fuzz can end up crafting such extended filenames when messing with the XML file, discovering those curl issues.

@bagder

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Aug 11, 2017

Member

I'm just glad we get the chance to iron out more bugs! =)

Member

bagder commented Aug 11, 2017

I'm just glad we get the chance to iron out more bugs! =)

@bagder bagder closed this in 6e0e152 Aug 11, 2017

@bagder

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Aug 11, 2017

Member

Thanks!

Member

bagder commented Aug 11, 2017

Thanks!

@coveralls

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Aug 11, 2017

Coverage Status

Coverage increased (+0.02%) to 75.084% when pulling 984add7 on rouault:fix_memleak_parse_proxy into 783d434 on curl:master.

Coverage Status

Coverage increased (+0.02%) to 75.084% when pulling 984add7 on rouault:fix_memleak_parse_proxy into 783d434 on curl:master.

@lock lock bot locked as resolved and limited conversation to collaborators May 21, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.