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

esp_http_client memory leak in esp_http_client_set_username/password (IDFGH-2306) #4444

Closed
AxelLin opened this issue Dec 5, 2019 · 1 comment

Comments

@AxelLin
Copy link
Contributor

AxelLin commented Dec 5, 2019

I'm checking the code in ESP-IDF-4.0-beta2:

In esp_http_client_set_username, it has below code:

if (username == NULL && client->connection_info.username != NULL) {
    free(client->connection_info.username);
    client->connection_info.username = NULL;
} else if (username != NULL) {
    client->connection_info.username = strdup(username);

Above code can cause memory leak when username != NULL && client->connection_info.username != NULL.
The same issue with esp_http_client_set_password.

e.g.
In esp_http_client/test/test_http_client.c
below test case will have memory leak.

TEST_CASE("Username and password will not reset if new absolute URL doesnot specify auth credentials.", "[ESP HTTP CLIENT]")
{
esp_http_client_config_t config_with_auth = {
.host = HOST,
.path = "/",
.username = USERNAME,
.password = PASSWORD
};
char *value = NULL;
esp_http_client_handle_t client = esp_http_client_init(&config_with_auth);
TEST_ASSERT_NOT_NULL(client);
esp_err_t r = esp_http_client_get_username(client, &value);
TEST_ASSERT_EQUAL(ESP_OK, r);
TEST_ASSERT_NOT_NULL(value);
TEST_ASSERT_EQUAL_STRING(USERNAME, value);
esp_http_client_set_url(client, "http://" HOST "/get");
esp_http_client_set_username(client, value);
esp_http_client_set_password(client, value);

BTW, since there are APIs to set_url/set_usernam/set_password,
why not also support set_authtype as well?

I post my proposed fix in attached file (including add set_authtype function):
esp_http_client_fix_memoryleak_and_allow_set_authtype_diff.txt

@github-actions github-actions bot changed the title esp_http_client memory leak in esp_http_client_set_username/password esp_http_client memory leak in esp_http_client_set_username/password (IDFGH-2306) Dec 5, 2019
@mahavirj
Copy link
Member

mahavirj commented Dec 6, 2019

@AxelLin Is it possible for you to create pull request (based on fix you provided above)?

AxelLin added a commit to AxelLin/esp-idf that referenced this issue Dec 6, 2019
…word

Fix memory in case username/password was set before calling
esp_http_client_set_username/password.

Link: espressif#4444
Fixes: 9fd16c6 ("fixes : set_url discards username and password")
Signed-off-by: Axel Lin <axel.lin@gmail.com>
AxelLin added a commit to AxelLin/esp-idf that referenced this issue Dec 6, 2019
Since currently there are APIs to set url/username/password, it would be
good to also allow setting authtype.

Link: espressif#4444
Signed-off-by: Axel Lin <axel.lin@gmail.com>
AxelLin added a commit to AxelLin/esp-idf that referenced this issue Dec 6, 2019
…word

Fix memory in case username/password was set before calling
esp_http_client_set_username/password.

Link: espressif#4444
Fixes: 9fd16c6 ("fixes : set_url discards username and password")
Signed-off-by: Axel Lin <axel.lin@gmail.com>
AxelLin added a commit to AxelLin/esp-idf that referenced this issue Dec 6, 2019
Since currently there are APIs to set url/username/password, it would be
good to also allow setting authtype.

Link: espressif#4444
Signed-off-by: Axel Lin <axel.lin@gmail.com>
espressif-bot pushed a commit that referenced this issue Dec 16, 2019
Since currently there are APIs to set url/username/password, it would be
good to also allow setting authtype.

Link: #4444
Closes #4454
Signed-off-by: Axel Lin <axel.lin@gmail.com>
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

No branches or pull requests

2 participants