Fix off-by-one when initializing Data with discontiguous, underestimated Sequences #23244
Conversation
|
@swift-ci Please test |
|
Build failed |
|
@swift-ci please clean test linux platform |
| if buffer.count < buffer.capacity { | ||
| buffer.append(byte: element) | ||
| } else { | ||
| buffer.append(byte: element) |
There was a problem hiding this comment.
To confirm my understanding:
bufferhas a fixed capacity- if it's at capacity after appending one element, you append the buffer to
_representationand reset the buffer - the bug was, you were detecting one less-than capacity and not appending under those circumstances, so that element was dropped
- you're sure it's not possible to ever append when already at capacity
Right?
There was a problem hiding this comment.
Correct on all accounts. The capacity is fixed to what we can stack-allocate and once we hit that capacity, we append what we had on the stack. The bug is that we threw away element previously in the case where we appended buffer; we now append it unconditionally with the knowledge that there is always space in the buffer before doing so.
|
Thanks a ton for this fix @itaiferber. I'm almost positive this is the issue causing test failures and crashes in http://github.com/vapor/vapor, https://github.com/vapor/url-encoded-form, and https://github.com/vapor/multipart. I would greatly appreciate if this fix made it into Swift 5.0.0 ❤️ Edit: Oh, and http://github.com/mordil/nio-redis. cc @Mordil |
|
@tanner0101 We're working on that right now. :) #23253 |
|
Build failed |
|
@swift-ci Please smoke test linux |
|
Build failures on Linux were unrelated (also, this version of |
|
We should also get this into SCLF too. |
|
@phausler Agreed — we have more leeway there so slightly less urgent, but still needs to be done ASAP |
|
Merging because smoke testing passed; full test is happening on #23253 |
What's in this pull request?
Resolves a serious off-by-one error which causes us to drop every 15th byte in a discontiguous, underestimated sequence past
max(underestimatedCount, InlineData.bufferSize). (rdar://problem/48775516)Our unit tests didn't previously catch this because the data they tested wasn't long enough.