Skip to content

Latest WiFiClientSecure client (github 16Jan23) is broken by “client.println("");” #7731

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

Closed
1 task done
Rob58329 opened this issue Jan 19, 2023 · 9 comments
Closed
1 task done
Labels
Type: Question Only question

Comments

@Rob58329
Copy link
Contributor

Board

ESP32 DevKitv1

Device Description

Nothing attached

Hardware Configuration

Defauult config

Version

other

IDE Name

1.8.19

Operating System

Windows10

Flash frequency

80MHz

PSRAM enabled

no

Upload speed

921600

Description

For the current (https://github.com/espressif/arduino-esp32 as at 16Jan23) WiFiClientSecure client, if you do a client.println("");” this breaks the WiFiClientSecure client: although the line eventually returns, the “WiFiClientSecure” buffer gets corrupted and the secure connection doesn’t work.

Note that the old github May2021 had a different “WiFiClientSecure” which was not broken by “client.println("");”

NB. if you consider this issue not worth fixing, please feel free to close this issue!

Sketch

The easiest way to test this is using the ESP32-Example-WiFiClientSecure/ WiFiClientSecure” sketch, and modifying one line and adding 1 line (as shown below):


    // Make a HTTP request:
    client.print("GET https://www.howsmyssl.com/a/check HTTP/1.0");
    client.println(""); // breaks the "WiFiClientSecure client" on current github ESP32 versions
// NB. client.println() and client.print("\r\n") both WORK,
// and  client.println(" ") half-works here, but client.println("") breaks the "WiFiClientSecure client"


### Debug Message

```plain
None

Other Steps to Reproduce

n/a

I have checked existing issues, online documentation and the Troubleshooting Guide

  • I confirm I have checked existing issues, online documentation and Troubleshooting guide.
@Rob58329 Rob58329 added the Status: Awaiting triage Issue is waiting for triage label Jan 19, 2023
@mrengineer7777
Copy link
Collaborator

mrengineer7777 commented Jan 19, 2023

@Rob58329 That's interesting and worth investigating. Can you narrow down which commit caused the failure? https://github.com/espressif/arduino-esp32/commits/master/libraries/WiFiClientSecure/src. Try different versions of arduino-esp32.

The fact that client.println() and client.print("\r\n") work but client.println(""); does not tells me it's not handling 0 length strings correctly.

println() comes from Print.cpp.
WiFiClientSecure.h : class WiFiClient : class ESPLwIPClient : class Client : class Stream : class Print

size_t Print::println(void)
{
    return print("\r\n");
}

size_t Print::println(const String &s)
{
    size_t n = print(s);
    n += println();
    return n;
}

size_t Print::print(const String &s)
{
    return write(s.c_str(), s.length());
}

size_t Print::write(const uint8_t *buffer, size_t size)
{
    size_t n = 0;
    while(size--) {
        n += write(*buffer++);
    }
    return n;
}

The only recent change I see for Print.cpp is on 2022.12.21. https://github.com/espressif/arduino-esp32/commits/master/cores/esp32/Print.cpp

@Rob58329
Copy link
Contributor Author

@mrengineer7777: I forgot to mention that this is only an issue for WiFiClientSecure, this does NOT happen if using "WiFiClient client". IE the non-encrypted client.println(""); works fine!

This is why I suspected it was due to a change in WiFiClientSecure,

@Rob58329
Copy link
Contributor Author

arduino-esp32-4b3f5c8ed463b18afe28d132d607bbf164d8eec9_v18Feb2021 "Remove temp buffer in WiFiClientSecure::lastError" (#4822) works.

arduino-esp32-a0ddd8a16e744b04ee58a7015742b582bc11eea7_IDF_v22Feb21_11:06gmt "IDF release/v3.3" fails.

arduino-esp32-560c0f45f58b907f0d699f65408b87fe54650854_v22Feb202_17:34gmt “Fix dropped SSL connection when buffer gets full. (https://github.com/espressif/arduino-esp32/pull/4820)” fails.

ie. it appears that it is the “IDF release/v3.3 7a85334d8 (https://github.com/espressif/arduino-esp32/pull/4813)” performed on 22Feb21 at 11:06gmt which causes the WiFiClientSecure.println(“”) to FAIL.

@mrengineer7777
Copy link
Collaborator

@Rob58329 Nice research. To summarize for WiFiClientSecure:
2021.02.18 Works with PR 4822
2021.02.21 Fails with PR 4820 Added back looping on mbedtls_ssl_write().
2021.02.22 Fails with PR 4813. This merged in IDF 3.3. Minimal changes in config. Unknown changes in compiled binary files (.a/.bin)

Possibilities:
Could be a change in component MBEDTLS.

Could be a change in IDF.

Could be that

int send_ssl_data(sslclient_context *ssl_client, const uint8_t *data, size_t len)
locks up if sending a zero length string.

Note that send_ssl_data() now has a timeout to break out of the loop. Earlier versions of the code looped indefinitely.

To proceed further: turn on verbose log messages (CORE_DEBUG_LEVEL=5). ssl_client.cpp prints verbose messages that should help pinpoint where the communications are failing.

@mrengineer7777
Copy link
Collaborator

@Rob58329 What did you see when you turned on verbose logging?

@mrengineer7777 mrengineer7777 added Resolution: More info needed More info must be provided in this issue Resolution: Awaiting response Waiting for response of author and removed Status: Awaiting triage Issue is waiting for triage labels Apr 5, 2023
@Rob58329
Copy link
Contributor Author

Rob58329 commented Apr 6, 2023

Using: (https://github.com/espressif/arduino-esp32 as at 16Jan23)
Using: "Core Debug level: "Verbose"
Original Example Sketch gives:

Connected to server!
[  4144][V][ssl_client.cpp:374] send_ssl_data(): Writing HTTP request with 46 bytes...
[  4145][V][ssl_client.cpp:374] send_ssl_data(): Writing HTTP request with 2 bytes...
[  4151][V][ssl_client.cpp:374] send_ssl_data(): Writing HTTP request with 23 bytes...
[  4159][V][ssl_client.cpp:374] send_ssl_data(): Writing HTTP request with 2 bytes...
[  4167][V][ssl_client.cpp:374] send_ssl_data(): Writing HTTP request with 17 bytes...
[  4174][V][ssl_client.cpp:374] send_ssl_data(): Writing HTTP request with 2 bytes...
[  4182][V][ssl_client.cpp:374] send_ssl_data(): Writing HTTP request with 2 bytes...
headers received
{"given_cipher_suites":["TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384"...
...stop_ssl_socket(): Cleaning SSL connection.
Test Ended

After client.println(""); is added gives:

Connected to server!
[  4138][V][ssl_client.cpp:374] send_ssl_data(): Writing HTTP request with 46 bytes...
[  4139][V][ssl_client.cpp:374] send_ssl_data(): Writing HTTP request with 2 bytes...
[  4156][V][ssl_client.cpp:374] send_ssl_data(): Writing HTTP request with 0 bytes...
[  6985][V][ssl_client.cpp:386] send_ssl_data(): Handling error -80
[  6985][E][ssl_client.cpp:37] _handle_error(): [send_ssl_data():387]: (-80) UNKNOWN ERROR CODE (0050)
[  6989][V][ssl_client.cpp:326] stop_ssl_socket(): Cleaning SSL connection.
[  6996][V][ssl_client.cpp:326] stop_ssl_socket(): Cleaning SSL connection.
Test Ended

This addition causes the line: Writing HTTP request with 0 bytes... (and subsequent error)

@me-no-dev
Copy link
Member

If you look at the source of print, you will see that when giving an argument to println it will first print the argument and then println without argument, so println("") means print("") and then println(). I am not sure if this is a bug, though it's easy to fix by adding a check in println(arg) to see if the argument has any length and print only if so.

@sanderkob
Copy link

sanderkob commented Jul 29, 2023

I confirm that client.println("") gives the error -80, even in the latest version Arduino 2.0.9. It can be solved by using

client.print("" + "\r\n")

I need this to indicate the end of headers by an empy line and cannot use client.println()
This haunted me since Arduino 1.0.5, I am glad that I have a workaround now.
This issue was noted earlier in platformio/platform-espressif32#649

@VojtechBartoska VojtechBartoska added Type: Question Only question and removed Resolution: Awaiting response Waiting for response of author Resolution: More info needed More info must be provided in this issue labels Aug 18, 2023
@VojtechBartoska
Copy link
Contributor

Hello, this seems to me like a question which was already answered so I'm closing it, if needed, feel free to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Question Only question
Projects
None yet
Development

No branches or pull requests

5 participants