I'm sort of surprised an allocation occurs every time the heartbeat is sent. That is a lot of trips to the heap.
I'm not very familiar with how TLS heartbeats are implemented, but I wonder if the buffer could have just been alloc'd once when the connection was created.
I believe there's another one that occurs every time a heartbeat request is received, and was just as surprised that they had to allocate a new buffer, copy the payload over (which is where the bug was), basically construct a new message, when they could've just validated the incoming message, changed type + padding, and sent it back out using the same buffer. Heartbeat extension is only slightly more complex than an echo protocol.
It's probably much harder to remove the one on reception due to how the rest of OpenSSL is written, but at least from a glance at the code, a lot easier to rid the sending one. In terms of design, the simplest implementation would be malloc-on-receive + modify-and-send; a little better is an expanding buffer that's allocated once but reallocated if necessary, and to me, the way it's currently being done is the most complex, inefficient, and error-prone.
Having many unnecessary dynamic allocations tends to be a trend I've noticed most often in C/C++ code written by programmers with a Java background. Not saying that this necessarily applies to heartbleed's culprit, but the general trend of excessive complexity is there.
I wouldn't know, but in any case that would require allocating the whole 64KiB for each connection, when the actual heartbeats can be much smaller. Wouldn't the memory waste be worse than the allocations?
Depends on implementation. Memory allocations can lead to fragmentation and thus to waste and decreased performance. That's what's great about using a compacting GC btw.
Comments
I'm sort of surprised an allocation occurs every time the heartbeat is sent. That is a lot of trips to the heap.
I'm not very familiar with how TLS heartbeats are implemented, but I wonder if the buffer could have just been alloc'd once when the connection was created.
I believe there's another one that occurs every time a heartbeat request is received, and was just as surprised that they had to allocate a new buffer, copy the payload over (which is where the bug was), basically construct a new message, when they could've just validated the incoming message, changed type + padding, and sent it back out using the same buffer. Heartbeat extension is only slightly more complex than an echo protocol.
It's probably much harder to remove the one on reception due to how the rest of OpenSSL is written, but at least from a glance at the code, a lot easier to rid the sending one. In terms of design, the simplest implementation would be malloc-on-receive + modify-and-send; a little better is an expanding buffer that's allocated once but reallocated if necessary, and to me, the way it's currently being done is the most complex, inefficient, and error-prone.
Having many unnecessary dynamic allocations tends to be a trend I've noticed most often in C/C++ code written by programmers with a Java background. Not saying that this necessarily applies to heartbleed's culprit, but the general trend of excessive complexity is there.
I wouldn't know, but in any case that would require allocating the whole 64KiB for each connection, when the actual heartbeats can be much smaller. Wouldn't the memory waste be worse than the allocations?
Depends on implementation. Memory allocations can lead to fragmentation and thus to waste and decreased performance. That's what's great about using a compacting GC btw.