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

Adds a Kconfig option for mbedtls' MBEDTLS_PLATFORM_MEMORY define. #2237

Closed
wants to merge 4 commits into from

Conversation

vonnieda
Copy link
Contributor

This makes it possible to override the mbedtls allocator with your own.

More information available in this forum thread:
https://esp32.com/viewtopic.php?f=2&t=6550

…is makes it possible to override the mbedtls allocator with your own.
@CLAassistant
Copy link

CLAassistant commented Jul 27, 2018

CLA assistant check
All committers have signed the CLA.

@projectgus
Copy link
Contributor

Hi @vonnieda ,

I've replied to you on the forum again: https://esp32.com/viewtopic.php?f=2&t=6550&p=28208#p28208

As written there, I'm not convinced this is the best solution to the problem you encountered.

The downside with enabling options like this in kconfig is that we have to support the inevitable second-order problems that appear when people misuse them.

@vonnieda
Copy link
Contributor Author

Hi @projectgus,

I responded in the thread.

I think it's important to be able to manage such a large allocation. A single TLS connection is nearly 10% of the total internal RAM, and if you take WiFi and BLE into account it's actually more like 16%. If you have to juggle more than one TLS connection, as I do, it becomes a real issue finding internal RAM for it.

Either way, my specific issue is solved by making a copy of mbedtls in my components directory, but it would be nice not to have to do that.

Thanks,
Jason

Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussing the details on the forum, I'm OK with merging this and agree other users will find it helpful as well. Thanks for sending it to us.

I have some requests about the way the feature is described in Kconfig, other than that it's good to go!

@@ -1,5 +1,27 @@
menu "mbedTLS"

config MBEDTLS_PLATFORM_MEMORY
bool "Enable mbedTLS memory allocation layer."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest "Enable custom mbedTLS memory allocation layer", so it's clear this is more than just having mbedTLS support memory allocation.

This allows different allocators (self-implemented or provided) to be
provided to the platform abstraction layer.

Enabling MBEDTLS_PLATFORM_MEMORY without the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this description doesn't make sense in the ESP-IDF context, where the user isn't editing the mbedTLS config header and can't easily define macros. Suggest something like:

If this option is disabled, mbed TLS uses the default system calloc() and free() functions.

If this option is enabled, the mbed TLS config macro MBEDTLS_PLATFORM_MEMORY will be defined. The function mbedtls_platform_set_calloc_free() must be called at runtime to provide custom calloc() and free() function pointers for use by mbedTLS.

This option allows fine-grained control over how mbedTLS allocates heap memory.

* 'master' of https://github.com/espressif/esp-idf:
  freertos: Remove either one or two assertions from "fast path" of vPortCPUReleaseMutex()
  tools: Allow running unit tests from command line
  asio: initial idf port of asio library without ssl
  lwip fix for udp receivefrom
  heap: Fix heap metadata test to account for background memory allocations
  ci: Simplify github deployment
  component/bt: bugfix on the crash during end of sco link resulted from link supervision timeout
  make: Add feature to cmd 'make flash' - reset ota_data partition
  removed possible uint16 access to 32bit register, noted fifo use not recommended
  coap: pass null-terminated string to gethostbyname
@vonnieda
Copy link
Contributor Author

Thanks @projectgus, updated the help and option name as you suggested.

@projectgus
Copy link
Contributor

Thanks @vonnieda . I've squashed these to a single commit and cherry-picked them into our internal review & merge queue.

@projectgus projectgus added the Status: Pending blocked by some other factor label Aug 1, 2018
@vonnieda
Copy link
Contributor Author

vonnieda commented Aug 1, 2018

Great, thank you @projectgus!

igrr pushed a commit that referenced this pull request Aug 6, 2018
…is makes it possible to override the mbedtls allocator with your own.

Merges #2237
@projectgus
Copy link
Contributor

Cherry-picked as d7a17ac, thanks!

@projectgus projectgus closed this Aug 6, 2018
@vonnieda
Copy link
Contributor Author

vonnieda commented Aug 6, 2018

Thank you!

@igrr igrr removed the Status: Pending blocked by some other factor label Aug 9, 2018
catalinio pushed a commit to catalinio/pycom-esp-idf that referenced this pull request Jun 28, 2019
…is makes it possible to override the mbedtls allocator with your own.

Merges espressif/esp-idf#2237
catalinio pushed a commit to catalinio/pycom-esp-idf that referenced this pull request Jun 28, 2019
turmary pushed a commit to Seeed-Studio/Seeed_Arduino_mbedtls that referenced this pull request Jan 22, 2020
…is makes it possible to override the mbedtls allocator with your own.

Merges espressif/esp-idf#2237
turmary pushed a commit to Seeed-Studio/Seeed_Arduino_mbedtls that referenced this pull request Jan 22, 2020
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

Successfully merging this pull request may close these issues.

4 participants