Skip to content

Conversation

@willmmiles
Copy link
Member

@willmmiles willmmiles commented Mar 16, 2024

Update to ESPAsyncWebServer 2.2.0 and leverage the new features.

  • (ESP8266) Use the ContentType symbols from the AsyncWebServer instance, reducing PROGMEM duplication
  • (ESP8266) Pass PROGMEM types to server.on(), so the String class will use the correct constructors instead of relying on the exception handler
  • Use explicit allocation for AsyncWebSocket handling, eliminating unneeded object allocations and frees; also fixes deprecation warnings
  • serveLiveLeds: avoid unneeded stack->buffer copy, and fix buffer overflow

These were mostly PROGMEM already, but every little bit helps.
Rather than relying on the exception handler, indicate the
__FlashStringHelper type so the correct String constructor is
used.
Eliminate the extra indirection and allocate shared buffers directly.
No need to filter or look up content type, just pitch it over the wall.
Also fixes .gz'd content processing.
There were three problems here:
- AsyncWebServer is going to copy to a heap buffer anyways, so we might
   as well just pass it one it can use
- The buffer size estimate was wrong -- we need 9 bytes per pixel
   ("RRGGBB",), so the buffer could overflow, and it was not
   considering the extra 2D requirements
- On ESP8266, the stack allocation was overflowing the stack, causing
  corruption and crashes.
@blazoncek
Copy link
Contributor

@willmmiles is this ready to be merged? I will blindly believe you. 😄

@willmmiles
Copy link
Member Author

I think so, but the whole point of a PR is to get a second pair of eyes in case I made some easy to spot mistake! The changes are pretty shallow, with the intent to make it easier to review - it's probably easier to go through them one commit at a time.

There are a couple places where the code could maybe be cleaner at the expense of the patches being harder to follow. One example is factoring out the now-duplicate names in wled_server. I had planned to send a second PR with cleanup later after you'd checked that the logic was sound.

The last commit (5f2480c) is probably the most critical, as it both fixes an actual buffer overflow when (used/n)>226 and improves performance by avoiding the stack-to-heap copy. Looking at it now, there's still room for improvement there too - it works, but we could shrink the dynamic allocation to fit the actually needed size instead of using a fixed value.

Use the CONTENT_TYPEs exported by AsyncWebServer directly.
Allocate the serialization buffer size at the required length, rather
than always allocating the maximum size.
@blazoncek
Copy link
Contributor

Code inspection reveals no discrepancies but I have not tested it yet.
In any case everything is logical so if AsyncWebSocketBuffer or DynamicBuffer code have flaws it would need to be addressed there.

@willmmiles
Copy link
Member Author

I went ahead and did those two code quality improvements. Might as well save on the testing effort!

@blazoncek blazoncek merged commit f1987b9 into wled:0_15 Mar 20, 2024
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.

2 participants