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

Incorrect implementation of cpr::Session move constructor and move assignment operator #990

Closed
Polarhigh opened this issue Dec 10, 2023 · 4 comments · Fixed by #999
Closed
Assignees
Labels

Comments

@Polarhigh
Copy link

Description

When moving a cpr::Session object, the curl options CURLOPT_XFERINFODATA, CURLOPT_DEBUGDATA, CURLOPT_WRITEDATA, and others are not updated.
As a result, when cpr tries to call, for example, the progress callback function, the progresscb_ of the old cpr::Session object that was moved is accessed.

Example/How to Reproduce

The sample code below is guaranteed to crash in debug mode. (In release mode, it's a matter of luck)

#include <iostream>
#include <cpr/cpr.h>

cpr::Session CreateCprSession(const std::string& url)
{
    cpr::Session session;

    session.SetUrl(url);
    session.SetProgressCallback(cpr::ProgressCallback(
        [](cpr::cpr_off_t downloadTotal, cpr::cpr_off_t downloadNow, cpr::cpr_off_t uploadTotal, cpr::cpr_off_t uploadNow, intptr_t userdata) {
            std::cout << "Downloaded: " << downloadTotal << " / " << downloadNow << "\n";
            return true;
        }));

    return session;
}


int main()
{
    cpr::Session session = CreateCprSession("https://httpbin.org/get");
    cpr::Response response = session.Get();

    std::cout << "Response: " << response.text << "\n";
    return 0;
}

Possible Fix

I think it can be fixed by adding support to the constructor and assignment operator so that the necessary curl options are updated.

Where did you get it from?

Other (specify in "Additional Context/Your Environment")

Additional Context/Your Environment

  • OS: Windows 11 Pro 23H2
  • cpr version: 1.10.5
    The cpr was obtained from github and added to the project via add_subdirectory.
Polarhigh added a commit to Next21Team/AmxxEasyHttp that referenced this issue Dec 10, 2023
@COM8
Copy link
Member

COM8 commented Dec 22, 2023

@Polarhigh thanks for reporting! I will try to take a look at this over the next few weeks.

@COM8
Copy link
Member

COM8 commented Dec 26, 2023

I decided to make cpr::Session neither movable nor copyable since it's a too complex of an object. Copying a cpr::Session does not make sense. What should happen in case you copy it in mids of request?

As a solution I suggest using some kind of a container like std::shared_ptr.

Example:

#include "cpr/session.h"
#include <iostream>
#include <memory>
#include <cpr/cpr.h>

std::shared_ptr<cpr::Session> CreateCprSession(const std::string& url) {
    std::shared_ptr<cpr::Session> session = std::make_shared<cpr::Session>();

    session->SetUrl(url);
    session->SetProgressCallback(cpr::ProgressCallback(
        [](cpr::cpr_off_t downloadTotal, cpr::cpr_off_t downloadNow, cpr::cpr_off_t uploadTotal, cpr::cpr_off_t uploadNow, intptr_t userdata) {
            std::cout << "Downloaded: " << downloadTotal << " / " << downloadNow << "\n";
            return true;
        }));

    return session;
}

int main() {
    std::shared_ptr<cpr::Session> session = CreateCprSession("https://httpbin.org/get");
    cpr::Response response = session->Get();

    std::cout << "Response: " << response.text << "\n";
    return 0;
}

@COM8
Copy link
Member

COM8 commented Dec 26, 2023

I'm keeping #999 open for the next week or so, to ensure there are no other important arguments against this approach.

@Polarhigh
Copy link
Author

What should happen in case you copy it in mids of request?

Indeed, nothing good will come of it)

As a solution I suggest using some kind of a container like std::shared_ptr.

Yes, thanks, in my case I decided to use unique_ptr

@COM8 COM8 closed this as completed in #999 Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants