Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Safe Curl opt in url.cpp #2734

Merged
merged 4 commits into from
Aug 9, 2023
Merged

Safe Curl opt in url.cpp #2734

merged 4 commits into from
Aug 9, 2023

Conversation

AntoinePrv
Copy link
Member

@AntoinePrv AntoinePrv commented Aug 4, 2023

Using RAII to prevent leaking memory

@AntoinePrv AntoinePrv self-assigned this Aug 4, 2023
@AntoinePrv AntoinePrv requested a review from Hind-M August 7, 2023 08:40
* Never null, throw exception at construction if creating the handle fails.
*/
class CurlEasyHandle
{
Copy link
Member

Choose a reason for hiding this comment

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

There is already such a handle in curl.hpp that could be used instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are some differences:

  • ~CurlHandle calls curl_slist_free_all(p_headers) which was not the case here.
  • There is no CURL* raw(); which was the point here.

Overall curl.hpp seems to be intended for actually using CURL to connect to the Internet, whereas here it is only a implementation detail for parsing urls (or at least what I'm trying to move it forward)

Copy link
Member

Choose a reason for hiding this comment

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

If we keep that new RAII type, please add a comment over the class to explain why this exist and how it is different from the CurlHandle, like you did here.

Copy link
Member

Choose a reason for hiding this comment

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

  • freeing the p_headers is not an issue, the pointer is null constructed so it is a noop if you don't explicitely add any header.
  • curl should wrapped instead of beeing exposed through a raw accessor, thus the usage of the wrapper in curl.

Anyway, if you prefer to keep.this tony wrapper (which is not a problem asnlong as it is not exposed in any header) you need to implement proper valu semantics; the current implementation has the same issue as CurlStr regarding the destructor.

Copy link
Member Author

@AntoinePrv AntoinePrv Aug 9, 2023

Choose a reason for hiding this comment

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

@JohanMabille I think keeping these wrappers is a pragmatic choice. As discussed with @Klaim, we don't really want to rely on Curl for parsing urls in the first place, so I am happy to keep this hidden and independent from the rest of the lib.

libmamba/src/core/url.cpp Outdated Show resolved Hide resolved
libmamba/src/core/url.cpp Outdated Show resolved Hide resolved
libmamba/src/core/url.cpp Outdated Show resolved Hide resolved
@JohanMabille JohanMabille merged commit be105ee into mamba-org:main Aug 9, 2023
20 checks passed
@AntoinePrv AntoinePrv deleted the safe-curl branch August 10, 2023 08:05
cvanelteren pushed a commit to cvanelteren/mamba that referenced this pull request Aug 29, 2023
* Safe Curl opt in url.cpp

* Delete Curl wrappers copy/move ctor/assign

* Simplify CurlStr free

* Improve CURLEasyHandle doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants