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

Add basic canary check to BSSL stack thunk #6156

Merged
merged 1 commit into from
May 28, 2019

Conversation

earlephilhower
Copy link
Collaborator

On return from a BSSL call, check that the last element of the stack is
still untouched. If it is modified, print an error and abort().

Will catch problems like #6143 many times with an informative error
message instead of corrupting the heap and having a random crash
sometime later.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
On return from a BSSL call, check that the last element of the stack is
still untouched.  If it is modified, print an error and abort().

Will catch problems like esp8266#6143 many times with an informative error
message instead of corrupting the heap and having a random crash
sometime later.
@earlephilhower earlephilhower requested a review from devyte May 27, 2019 22:52
@dok-net
Copy link
Contributor

dok-net commented Jul 9, 2019

Dear @earlephilhower!
I am building a cooperative scheduler [https://github.com/dok-net/CoopTask/], and stack space is an issue there, too. I've found this StackThunk implementation, and I'd like to refactor and use it. I've got two questions:

  • On the ESP8266, does the stack grow downward as usual? I think StackThunk has stack_top and stack_bottom backward, stack_bottom should be the base, have the highest address, and stack_top should be the lower address, in terms of malloc the address that's returned by that. I guess the naming is consistent right now, but it would be entirely confusing if I'm right, would you agree?

cont_stack_start and cont_stack_end on the other hand denominate the reserved memory area, for that, the naming is OK.

  • Could the StackThunk implementation be refactored into a class, such that it becomes re-usable for other libraries that need their own additional stack space? I imagine creating a singleton instance of it in the BearSSL *.cpp would be easy to do, and fixing core_esp8266_postmortem.cpp to deal with that can't be an unsurmountable task either.

If you have no objections and would be willing to merge such a change "immediately", I might give it a try.

@earlephilhower
Copy link
Collaborator Author

For a scheduler, I'd not use the current thunk code. It's really special purpose and has assumptions about parameter passing (i.e. nothing overflows the parameter regs so no stack copying either way, no preservation of processor state, etc.).

@dok-net
Copy link
Contributor

dok-net commented Jul 11, 2019

I've found a pure C++ way of giving the tasks each their own stack on the heap, which works on ESP8266 and ESP32. I will test this on an 8bit Arduino, too.
I am surprised that nobody has built cooperative multitasking on the ESP8266, using the cont.h/.S etc. functionality, which seems to go almost all the way toward that goal already. I am in a discussion with the ESP32 maintainer regarding cooperative multitasking on that SOC, where he is convinced that the FreeRTOS tasks should be used instead. Yes, I am willing to believe that RTOS task scheduling is faster that that of, say, Windows or Linux, but given the complexity of concurrent programming, and all that work Microsoft have done for their TPM async/await programming is not without reason, I am confident that our small devices stand to take good advantage of cooperative multitasking. Take delays and infinite loops, they look really much simpler than if you try to express that in the single-loop() model, while there is none of the atomicity and synchronization problems that befall preemptive MT.

Possibly, even some of the current driver work, for which the Schedule class was revamped to supported recurrent and timed functions, could use such a scheduler for much more expressive code?
https://github.com/dok-net/CoopTask/releases/tag/0.7.0

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.

None yet

3 participants