Improve QUIC defragmentation of TLS packets. #158
Conversation
This avoids excessive persistent memory usage associated with every QUIC connection, by avoiding a large (21k) preallocation of a buffer used for defragmentation of TLS packets. Instead of preallocating space for all TLS data, this only stores TLS fragments that cannot be fully assembled on the connection.
|
This addresses the issue in #157. As far as I can tell, there isn't any real testing of calling |
| size_t length; | ||
| /* char data[]; should be here but C90 VLAs not allowed here */ |
There was a problem hiding this comment.
Paragraph 6.7.3.2.20 of n3220 permits flexible arrays as last element which would be allowed. However I think we need to double check what century our compilers are from. This is probably the clean way to do what you want, as I'm not sure if what you're doing is undefined behavior or not in all places
There was a problem hiding this comment.
I would also be inclined to use real C99 flexible arrays.
On a different project investigating the question of flexible array support, my colleague wrote "Using the godbolt.org side, I couldn't find a gcc or clang level that didn't support flex arrays.". That project ended up using an autoconf test with some probably-undefined-behavior alternative for systems that don't support flexible arrays (e.g., AIX), whose compilers can probably safely be assumed to not be trying to do anything with that undefined behavior.
quictls inherits its configure script from openssl, and does not have real facility for autoconf-style "try a build and see if it works" checks, but our target audience is also probably for modern enough systems that we could just assume support for it. (That same other project I mentioned also concluded that Microsoft's compilers supported it as an extension well before it was standardized.)
There was a problem hiding this comment.
I don't have any strong preference. That part of the change was simply restoring code from 075ede1.
There was a problem hiding this comment.
I'm good with flexible arrays. @nibanks ?
|
When 3.1.6 is ready, this should be rebased... but we can always pull the commit over. |
|
I've added nibanks as a reviewer. Let's figure out how to release this on 3.1.7 after we finish making it (bump a version somehow with a suffix?) or agree it's good for 3.1.8/whatever happens next. Still need input from nibanks on if VLA will work for him: suspect answer is yes. (And I think that means a code change if yes) |
|
I don't have a lot of feedback except to add that MsQuic explicitly looks at the TLS framing layer to ensure it never gives a fragment down to the lower layer, so it can completely handle the complexity itself the same across all TLS libraries. So we don't need any large preallocated buffer. But that's just MsQuic... |
|
What about the use of VLA? I think that would make the patch nicer, as I'm not 100% sure of the current code being right. It would however mean updating our C level to C99, and we want to be sure you're onboard with that. |
|
We are all good with VLA @bwelling could you adjust your patch and rebase on 3.1.7, and then we can release with it? |
|
I opened a new PR for the existing patch against 3.1.7. I'm not sure what VLA changes are wanted or needed, so it would probably be easier for someone who does know to make the changes. |
|
Replaced by #173. |
This avoids excessive persistent memory usage associated with every QUIC connection, by avoiding a large (21k) preallocation of a buffer used for defragmentation of TLS packets.
Instead of preallocating space for all TLS data, this only stores TLS fragments that cannot be fully assembled on the connection.