espressif / esp-idf

Espressif IoT Development Framework. Official development framework for Espressif SoCs.
Apache License 2.0
13.91k stars 7.33k forks source link

"esp_flash_api.c" and "transport_ssl.c“ make some changes (IDFGH-2486) #4598

Closed xiruilin closed 4 years ago

xiruilin commented 4 years ago

Environment

Problem Description

1. In esp_flash_api.c lines 501, function esp_flash_read(), if read length too big, then length_to_allocate = MAX(MAX_READ_CHUNK, length) will cause memory allocation to fail. In lines 519, length_to_read = MIN(MAX_READ_CHUNK, length), so it can be modified as follows:

diff --git a/components/spi_flash/esp_flash_api.c b/components/spi_flash/esp_flash_api.c
index ff9c49e4d..a91a05775 100644
--- a/components/spi_flash/esp_flash_api.c
+++ b/components/spi_flash/esp_flash_api.c
@@ -498,7 +498,7 @@ esp_err_t IRAM_ATTR esp_flash_read(esp_flash_t *chip, void *buffer, uint32_t add
     uint8_t* temp_buffer = NULL;

     if (!direct_read) {
-        uint32_t length_to_allocate = MAX(MAX_READ_CHUNK, length);
+        uint32_t length_to_allocate = MIN(MAX_READ_CHUNK, length);
         length_to_allocate = (length_to_allocate+3)&(~3);
         temp_buffer = heap_caps_malloc(length_to_allocate, MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT);
         ESP_LOGV(TAG, "allocate temp buffer: %p", temp_buffer);

2. In transport_ssl.c lines 168, function ssl_close(), if https request got 301 or 302 status code, need to redirect, the ssl state must be reset to origin, changed below:

diff --git a/components/tcp_transport/transport_ssl.c b/components/tcp_transport/transport_ssl.c
index acc963666..5501b2383 100644
--- a/components/tcp_transport/transport_ssl.c
+++ b/components/tcp_transport/transport_ssl.c
@@ -172,6 +172,7 @@ static int ssl_close(esp_transport_handle_t t)
     if (ssl->ssl_initialized) {
         esp_tls_conn_delete(ssl->tls);
         ssl->ssl_initialized = false;
+        ssl->conn_state = TRANS_SSL_INIT;
     }
     return ret;
 }

Thanks!

Alvin1Zhang commented 4 years ago

@xiruilin Thanks for reporting, we would look into. Thanks.

ginkgm commented 4 years ago

Hi @xiruilin ,

The first question seems to be solved in bfc37ab43f1509df6331781e935af09b6e1ae65c. Could you have a try?

mahavirj commented 4 years ago

@xiruilin For the transport_ssl change suggested above, do you have test case or code snippet which fails to work without this?

xiruilin commented 4 years ago

@ginkgm The first problem has been solved, thanks!

Alvin1Zhang commented 4 years ago

@xiruilin Glad to hear the first issue has been resolved, how about the second issue? For the transport_ssl change suggested above, do you have test case or code snippet which fails to work without this? Thanks.

xiruilin commented 4 years ago

@Alvin1Zhang @mahavirj Sorry for my late answer. The second issue occurred on an asynchronous connection, if not reset ssl->conn_state to TRANS_SSL_INIT in ssl_close(), esp_http_client_set_redirection() would got the issue. In file "/esp-idf/components/tcp_transport/transport_ssl.c" lines 48, function ssl_connect_async() check the ssl->conn_state.