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.

Sign up for GitHub

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

Jump to bottom

Exception in X509List destructor due to invalid default copy assignment operator #8522

Open
6 tasks done
timr49 opened this issue Mar 28, 2022 · 1 comment · May be fixed by #8524
Open
6 tasks done

Exception in X509List destructor due to invalid default copy assignment operator #8522

timr49 opened this issue Mar 28, 2022 · 1 comment · May be fixed by #8524
Assignees
@earlephilhower

Comments

@timr49
Copy link

timr49 commented Mar 28, 2022

Basic Infos

  • This issue complies with the issue POLICY doc.
  • I have read the documentation at readthedocs and the issue is not addressed there.
  • I have tested that the issue is present in current master branch (aka latest git).
  • I have searched the issue tracker for a similar issue.
  • If there is a stack dump, I have decoded it.
  • I have filled out all fields below.

Platform

  • Hardware: ESP8266EX
  • Core Version: 3.0.2
  • Development Env: Arduino IDE
  • Operating System: MacOS

Settings in IDE

  • Module: Wemos D1 mini r2
  • Flash Mode: [qio|dio|other]?
  • Flash Size: 4MB
  • lwip Variant: v2 Lower Memory
  • Reset Method: [ck|nodemcu]?
  • Flash Frequency: [40Mhz]?
  • CPU Frequency: 80Mhz
  • Upload Using: SERIAL
  • Upload Speed: 921600

Problem Description

In the ESP8266WiFi library, class X509List explicitly deletes its copy constructor, presumably because the default copy constructor would be invalid and a working copy constructor would require deep copying of the certificate contents etc. However, it does not delete the default copy assignment operator nor does it define a custom copy assignment operator. It does not define a move constructor or a move assignment operator.

Consequently, a sketch that assigns an instance of X509List can result in an exception due to the destructor (~X509List()) freeing the space allocated for the certificate contents twice - once when the instance on the right hand side of the assignment is deleted after it has been copied, and once when the left hand side instance is eventually deleted.

In the example sketch below, the function test1() announces that it is about to return, but the calling function setup() does not announces that it has returned, instead an exception occurs. This is because the instance x509List is implicitly deleted at the end of test1() but the contents have already been deleted after the assignment. This has been demonstrated with the 3.0.2 release and also with the 3.0.2 release updated with the master version of the ESP8266/WiFi library (retrieved 25 March 2022).

Solutions

  1. A minimal solution is to explicitly delete the default copy assignment operator. This stops the example sketch with a compile-time error. This has been informally coded and tested.
  2. A slightly more advanced solution is to define a move constructor and a move assignment operator. This allows the example sketch to compile and run as expected, because its assignment satisfies the requirements for a "move" so copying of the contents is not actually required. Sketches with assignments that do require a copy should still fail at compile time (per 1 above).

MCVE Sketch

/*
    Test X509List copy assignment
    Derived from ...

    HTTP over TLS (HTTPS) example sketch

    This example demonstrates how to use
    WiFiClientSecure class to access HTTPS API.
    We fetch and display the status of
    esp8266/Arduino project continuous integration
    build.

    Created by Ivan Grokhotkov, 2015.
    This example is in public domain.
*/

#include <ESP8266WiFi.h>
#include <WiFiClientSecure.h>

extern const char cert_DigiCert_High_Assurance_EV_Root_CA [];

void test1() {
  X509List x509list;
  
  Serial.println("X509List assignment ..."); Serial.flush();
  
  x509list = X509List(cert_DigiCert_High_Assurance_EV_Root_CA);
  
  Serial.println("... X509List assignment done"); Serial.flush();  
  Serial.println("test1() returning"); Serial.flush();  
}

void setup() {
  Serial.begin(115200);
  while (!Serial)
    delay(0);
  delay(200);
  Serial.println("\nsetup"); Serial.flush();

  Serial.println("calling test1() ..."); Serial.flush();
  test1();
  Serial.println("... test1() returned"); Serial.flush();  
}

void loop() {
  Serial.println("loop");
  delay(10000);
}

// Derived from certs.h ...

// http://cacerts.digicert.com/DigiCertHighAssuranceEVRootCA.crt
// CN: DigiCert High Assurance EV Root CA => name: DigiCert_High_Assurance_EV_Root_CA
// not valid before: 2006-11-10 00:00:00
// not valid after:  2031-11-10 00:00:00
const char cert_DigiCert_High_Assurance_EV_Root_CA [] = R"CERT(
-----BEGIN CERTIFICATE-----
MIIDxTCCAq2gAwIBAgIQAqxcJmoLQJuPC3nyrkYldzANBgkqhkiG9w0BAQUFADBs
MQswCQYDVQQGEwJVUzEVMBMGA1UEChMMRGlnaUNlcnQgSW5jMRkwFwYDVQQLExB3
d3cuZGlnaWNlcnQuY29tMSswKQYDVQQDEyJEaWdpQ2VydCBIaWdoIEFzc3VyYW5j
ZSBFViBSb290IENBMB4XDTA2MTExMDAwMDAwMFoXDTMxMTExMDAwMDAwMFowbDEL
MAkGA1UEBhMCVVMxFTATBgNVBAoTDERpZ2lDZXJ0IEluYzEZMBcGA1UECxMQd3d3
LmRpZ2ljZXJ0LmNvbTErMCkGA1UEAxMiRGlnaUNlcnQgSGlnaCBBc3N1cmFuY2Ug
RVYgUm9vdCBDQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAMbM5XPm
+9S75S0tMqbf5YE/yc0lSbZxKsPVlDRnogocsF9ppkCxxLeyj9CYpKlBWTrT3JTW
PNt0OKRKzE0lgvdKpVMSOO7zSW1xkX5jtqumX8OkhPhPYlG++MXs2ziS4wblCJEM
xChBVfvLWokVfnHoNb9Ncgk9vjo4UFt3MRuNs8ckRZqnrG0AFFoEt7oT61EKmEFB
Ik5lYYeBQVCmeVyJ3hlKV9Uu5l0cUyx+mM0aBhakaHPQNAQTXKFx01p8VdteZOE3
hzBWBOURtCmAEvF5OYiiAhF8J2a3iLd48soKqDirCmTCv2ZdlYTBoSUeh10aUAsg
EsxBu24LUTi4S8sCAwEAAaNjMGEwDgYDVR0PAQH/BAQDAgGGMA8GA1UdEwEB/wQF
MAMBAf8wHQYDVR0OBBYEFLE+w2kD+L9HAdSYJhoIAu9jZCvDMB8GA1UdIwQYMBaA
FLE+w2kD+L9HAdSYJhoIAu9jZCvDMA0GCSqGSIb3DQEBBQUAA4IBAQAcGgaX3Nec
nzyIZgYIVyHbIUf4KmeqvxgydkAQV8GK83rZEWWONfqe/EW1ntlMMUu4kehDLI6z
eM7b41N5cdblIZQB2lWHmiRk9opmzN6cN82oNLFpmyPInngiK3BD41VHMWEZ71jF
hS9OMPagMRYjyOfiZRYzy78aG6A9+MpeizGLYAiJLQwGXFK3xPkKmNEVX58Svnw2
Yzi9RKR/5CYrCsSXaQ3pjOLAEFe4yHYSkVXySGnYvCoCWw9E1CAx2/S6cCZdkGCe
vEsXCS+0yx5DaMkHJ8HSXPfqIbloEpw8nL+e/IBcm2PN7EeqJSdnoDfzAIJ9VNep
+OkuE6N36B9K
-----END CERTIFICATE-----
)CERT";

Debug Messages

13:16:13.571 -> setup
13:16:13.571 -> calling test1() ...
13:16:13.571 -> X509List assignment ...
13:16:13.647 -> ... X509List assignment done
13:16:13.647 -> test1() returning
13:16:13.647 -> 
13:16:13.647 -> --------------- CUT HERE FOR EXCEPTION DECODER ---------------
13:16:13.647 -> 
13:16:13.647 -> Exception (3):
13:16:13.647 -> epc1=0x40100491 epc2=0x00000000 epc3=0x00000000 excvaddr=0x4003bff9 depc=0x00000000
13:16:13.647 -> 
13:16:13.647 -> >>>stack>>>
13:16:13.647 -> 
13:16:13.647 -> ctx: cont
13:16:13.647 -> sp: 3ffffd70 end: 3fffffc0 offset: 0190
13:16:13.647 -> 3fffff00:  3ffe8d9f 00000000 3fffff70 40201b1a  
13:16:13.647 -> 3fffff10:  000000ad 000000ad 3ffe85dc 401006bf  
13:16:13.681 -> 3fffff20:  3fffdad0 0000000a 00000000 3ffeeb1c  
13:16:13.681 -> 3fffff30:  00000001 00000020 3ffef8d4 4010089e  
13:16:13.681 -> 3fffff40:  40100260 3ffef8d4 00000001 402011d4  
13:16:13.681 -> 3fffff50:  3fffdad0 3fffff7c 00000000 402011fb  
13:16:13.681 -> 3fffff60:  40201c98 00000000 3ffeeaa8 402010a8  
13:16:13.681 -> 3fffff70:  00000001 3ffef8d4 3ffef8e4 00000001  
13:16:13.681 -> 3fffff80:  3ffef8d4 3ffef8e4 3ffeeaa8 40201bec  
13:16:13.681 -> 3fffff90:  3fffdad0 00000000 3ffeeaa8 4020110c  
13:16:13.717 -> 3fffffa0:  feefeffe feefeffe 3ffeeb08 40202400  
13:16:13.717 -> 3fffffb0:  feefeffe feefeffe 3ffe85d8 40100b51  
13:16:13.717 -> <<<stack<<<
13:16:13.717 -> 
13:16:13.717 -> --------------- CUT HERE FOR EXCEPTION DECODER ---------------

Exception 3: LoadStoreError: Processor internal physical address or data error during load or store
PC: 0x40100491: umm_assimilate_up(umm_heap_context_t*, uint16_t) at /Users/tim/Library/Arduino15/packages/esp8266/hardware/esp8266/3.0.2/cores/esp8266/umm_malloc/umm_malloc.cpp line 336
EXCVADDR: 0x4003bff9

Decoding stack results
0x40201b1a: BearSSL::X509List::append(unsigned char const*, unsigned int) at /Users/tim/Library/Arduino15/packages/esp8266/hardware/esp8266/3.0.2/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp line 872
0x401006bf: umm_free_core(umm_heap_context_t*, void*) at /Users/tim/Library/Arduino15/packages/esp8266/hardware/esp8266/3.0.2/cores/esp8266/umm_malloc/umm_malloc.cpp line 549
0x4010089e: free(void*) at /Users/tim/Library/Arduino15/packages/esp8266/hardware/esp8266/3.0.2/cores/esp8266/umm_malloc/umm_malloc.cpp line 595
0x40100260: delayMicroseconds(unsigned int) at /Users/tim/Library/Arduino15/packages/esp8266/hardware/esp8266/3.0.2/cores/esp8266/core_esp8266_wiring.cpp line 209
0x402011d4: brssl::free_certificates(br_x509_certificate*, unsigned int) at /Users/tim/Library/Arduino15/packages/esp8266/hardware/esp8266/3.0.2/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp line 403
0x402011fb: BearSSL::X509List::~X509List() at /Users/tim/Library/Arduino15/packages/esp8266/hardware/esp8266/3.0.2/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp line 834
0x40201c98: HardwareSerial::write(unsigned char const*, unsigned int) at /Users/tim/Library/Arduino15/packages/esp8266/hardware/esp8266/3.0.2/cores/esp8266/HardwareSerial.h line 193
0x402010a8: test1() at /Users/tim/Repositories/sam/src/Arduino/ESP8266-X509List-IssueA/ESP8266-X509List-IssueA.ino line 30
0x40201bec: HardwareSerial::flush() at /Users/tim/Library/Arduino15/packages/esp8266/hardware/esp8266/3.0.2/cores/esp8266/HardwareSerial.cpp line 125
0x4020110c: setup() at /Users/tim/Repositories/sam/src/Arduino/ESP8266-X509List-IssueA/ESP8266-X509List-IssueA.ino line 42
0x40202400: loop_wrapper() at /Users/tim/Library/Arduino15/packages/esp8266/hardware/esp8266/3.0.2/cores/esp8266/core_esp8266_main.cpp line 198
@earlephilhower earlephilhower self-assigned this Mar 29, 2022
earlephilhower added a commit to earlephilhower/Arduino that referenced this issue Mar 29, 2022
@earlephilhower
Add move constructor/assignment to BearSSL helpers
a9fd6cb 
Fixes esp8266#8522
@earlephilhower earlephilhower linked a pull request Mar 29, 2022 that will close this issue
Open
@timr49
Copy link
Author

timr49 commented Apr 2, 2022

Thanks for that. Do we also need to delete copy assignment operators wherever we delete copy constructors so that the default copy assignment operator is not used for an assignment that does not qualify for move assignment, e.g. my original example above?

@paulocsanz paulocsanz mentioned this issue Apr 9, 2022
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@earlephilhower earlephilhower

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add move constructor/assignment to BearSSL helpers earlephilhower/Arduino
2 participants
@earlephilhower @timr49