10000 fixed some TLS errors when combining protocols SSL/TLS and VST (#6551) · mnemosdev/arangodb@b25ffd9 · GitHub
[go: up one dir, main page]
Skip to content

Commit b25ffd9

Browse files
jsteemannjsteemann
authored
fixed some TLS errors when combining protocols SSL/TLS and VST (arangodb#6551)
1 parent fbd49fe commit b25ffd9

File tree

7 files changed

+124
-68
lines changed

7 files changed

+124
-68
lines changed

‎CHANGELOG

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,13 @@
1+
v3.3.17 (XXXX-XX-XX)
2+
--------------------
3+
4+
* fix some TLS errors that occurred when combining HTTPS/TLS transport with the
5+
VelocyStream protocol (VST)
6+
7+
That combination could have led to spurious errors such as "TLS padding error"
8+
or "Tag mismatch" and connections being closed.
9+
10+
111
v3.3.16 (2018-09-19)
212
--------------------
313

‎arangod/GeneralServer/HttpCommTask.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,13 @@ HttpCommTask::HttpCommTask(EventLoop loop, GeneralServer* server,
7070
ConnectionStatistics::SET_HTTP(_connectionStatistics);
7171
}
7272

73+
// whether or not this task can mix sync and async I/O
74+
bool HttpCommTask::canUseMixedIO() const {
75+
// in case SSL is used, we cannot use a combination of sync and async I/O
76+
// because that will make TLS fall apart
77+
return !_peer->isEncrypted();
78+
}
79+
7380
/// @brief send error response including response body
7481
void HttpCommTask::addSimpleResponse(rest::ResponseCode code, rest::ContentType respType,
7582
uint64_t /*messageId*/, velocypack::Buffer<uint8_t>&& buffer) {

‎arangod/GeneralServer/HttpCommTask.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,10 @@ class HttpCommTask final : public GeneralCommTask {
2323
arangodb::Endpoint::TransportType transportType() override {
2424
return arangodb::Endpoint::TransportType::HTTP;
2525
}
26-
26+
27+
// whether or not this task can mix sync and async I/O
28+
bool canUseMixedIO() const override;
29+
2730
private:
2831
bool processRead(double startTime) override;
2932
void compactify() override;

‎arangod/GeneralServer/VstCommTask.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,13 @@ VstCommTask::VstCommTask(EventLoop loop, GeneralServer* server,
9999
ServerFeature>("Server")
100100
->vstMaxSize();
101101
}
102+
103+
// whether or not this task can mix sync and async I/O
104+
bool VstCommTask::canUseMixedIO() const {
105+
// in case SSL is used, we cannot use a combination of sync and async I/O
106+
// because that will make TLS fall apart
107+
return !_peer->isEncrypted();
108+
}
102109

103110
/// @brief send simple response including response body
104111
void VstCommTask::addSimpleResponse(rest::ResponseCode code, rest::ContentType respType,

‎arangod/GeneralServer/VstCommTask.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,9 @@ class VstCommTask final : public GeneralCommTask {
4646
return arangodb::Endpoint::TransportType::VST;
4747
}
4848

49+
// whether or not this task can mix sync and async I/O
50+
bool canUseMixedIO() const override;
51+
4952
protected:
5053
// read data check if chunk and message are complete
5154
// if message is complete execute a request

‎arangod/Scheduler/SocketTask.cpp

Lines changed: 89 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ bool SocketTask::start() {
120120

121121
if (_closeRequested.load(std::memory_order_acquire)) {
122122
LOG_TOPIC(DEBUG, Logger::COMMUNICATION)
123-
<< "cannot start, close alread in progress";
123+
<< "cannot start, close already in progress";
124124
return false;
125125
}
126126

@@ -192,6 +192,7 @@ void SocketTask::closeStream() {
192192
if (_abandoned.load(std::memory_order_acquire)) {
193193
return;
194194
}
195+
195196
// strand::dispatch may execute this immediately if this
196197
// is called on a thread inside the same strand
197198
auto self = shared_from_this();
@@ -291,6 +292,7 @@ bool SocketTask::trySyncRead() {
291292

292293
asio_ns::error_code err;
293294
TRI_ASSERT(_peer != nullptr);
295+
294296
if (0 == _peer->available(err)) {
295297
return false;
296298
}
@@ -315,17 +317,15 @@ bool SocketTask::trySyncRead() {
315317

316318
_readBuffer.increaseLength(bytesRead);
317319

318-
if (err) {
319-
if (err == asio_ns::error::would_block) {
320-
return false;
321-
} else {
322-
LOG_TOPIC(DEBUG, Logger::COMMUNICATION) << "trySyncRead failed with: "
323-
<< err.message();
324-
return false;
325-
}
320+
if (!err) {
321+
return true;
326322
}
327323

328-
return true;
324+
if (err != asio_ns::error::would_block && err != asio_ns::error::try_again) {
325+
LOG_TOPIC(DEBUG, Logger::COMMUNICATION) << "trySyncRead failed with: " << err.message();
326+
}
327+
328+
return false;
329329
}
330330

331331
// must run in strand
@@ -379,39 +379,44 @@ void SocketTask::asyncReadSome() {
379379
TRI_ASSERT(_peer != nullptr);
380380
TRI_ASSERT(_peer->strand.running_in_this_thread());
381381

382-
try {
383-
size_t const MAX_DIRECT_TRIES = 2;
384-
size_t n = 0;
385-
386-
while (++n <= MAX_DIRECT_TRIES &&
387-
!_abandoned.load(std::memory_order_acquire)) {
388-
if (!trySyncRead()) {
389-
if (n < MAX_DIRECT_TRIES) {
390-
std::this_thread::yield();
382+
if (this->canUseMixedIO()) {
383+
// try some direct reads only for non-SSL mode
384+
// in SSL mode it will fall apart when mixing direct reads and async
385+
// reads later
386+
try {
387+
size_t const MAX_DIRECT_TRIES = 2;
388+
size_t n = 0;
389+
390+
while (++n <= MAX_DIRECT_TRIES &&
391+
!_abandoned.load(std::memory_order_acquire)) {
392+
if (!trySyncRead()) {
393+
if (n < MAX_DIRECT_TRIES) {
394+
std::this_thread::yield();
395+
}
396+
continue;
391397
}
392-
continue;
393-
}
394398

395-
if (_abandoned.load(std::memory_order_acquire)) {
396-
return;
399+
if (_abandoned.load(std::memory_order_acquire)) {
400+
return;
401+
}
402+
403+
// ignore the result of processAll, try to read more bytes down below
404+
processAll();
405+
compactify();
397406
}
407+
} catch (asio_ns::system_error const& err) {
408+
LOG_TOPIC(DEBUG, Logger::COMMUNICATION) << "sync read failed with: "
409+
<< err.what();
410+
closeStreamNoLock();
411+
return;
412+
} catch (...) {
413+
LOG_TOPIC(DEBUG, Logger::COMMUNICATION) << "general error on stream";
398414

399-
// ignore the result of processAll, try to read more bytes down below
400-
processAll();
401-
compactify();
415+
closeStreamNoLock();
416+
return;
402417
}
403-
} catch (asio_ns::system_error const& err) {
404-
LOG_TOPIC(DEBUG, Logger::COMMUNICATION) << "sync read failed with: "
405-
<< err.what();
406-
closeStreamNoLock();
407-
return;
408-
} catch (...) {
409-
LOG_TOPIC(DEBUG, Logger::COMMUNICATION) << "general error on stream";
410-
411-
closeStreamNoLock();
412-
return;
413418
}
414-
419+
415420
// try to read more bytes
416421
if (_abandoned.load(std::memory_order_acquire)) {
417422
return;
@@ -461,54 +466,72 @@ void SocketTask::asyncWriteSome() {
461466
if (_writeBuffer.empty()) {
462467
return;
463468
}
469+
470+
TRI_ASSERT(_writeBuffer._buffer != nullptr);
464471
size_t total = _writeBuffer._buffer->length();
465472
size_t written = 0;
466473

467474
TRI_ASSERT(!_abandoned);
468-
TRI_ASSERT(_peer != nullptr);
469475

470476
asio_ns::error_code err;
471-
err.clear();
472-
while (true) {
473-
RequestStatistics::SET_WRITE_START(_writeBuffer._statistics);
474-
written = _peer->writeSome(_writeBuffer._buffer, err);
477+
478+
if (this->canUseMixedIO()) {
479+
// try some direct writes only for non-SSL mode
480+
// in SSL mode it will fall apart when mixing direct writes and async
481+
// writes later
482+
while (true) {
483+
TRI_ASSERT(_writeBuffer._buffer != nullptr);
484+
485+
// we can directly skip sending empty buffers
486+
if (_writeBuffer._buffer->length() > 0) {
487+
RequestStatistics::SET_WRITE_START(_writeBuffer._statistics);
488+
written = _peer->writeSome(_writeBuffer._buffer, err);
489+
490+
RequestStatistics::ADD_SENT_BYTES(_writeBuffer._statistics, written);
491+
492+
if (err || written != total) {
493+
// unable to write everything at once, might be a lot of data
494+
// above code does not update the buffer positon
495+
break;
496+
}
475497

476-
if (err) {
477-
break;
478-
}
498+
TRI_ASSERT(written > 0);
499+
}
479500

480-
RequestStatistics::ADD_SENT_BYTES(_writeBuffer._statistics, written);
501+
if (!completedWriteBuffer()) {
502+
return;
503+
}
481504

482-
if (written != total) {
483-
// unable to write everything at once, might be a lot of data
484-
// above code does not update the buffer positon
485-
break;
505+
// try to send next buffer
506+
TRI_ASSERT(_writeBuffer._buffer != nullptr);
507+
total = _writeBuffer._buffer->length();
486508
}
487509

488-
if (!completedWriteBuffer()) {
510+
// write could have blocked which is the only acceptable error
511+
if (err && err != asio_ns::error::would_block && err != asio_ns::error::try_again) {
512+
LOG_TOPIC(DEBUG, Logger::COMMUNICATION) << "sync write on stream failed with: "
513+
<< err.message();
514+
closeStreamNoLock();
489515
return;
490516
}
517+
} // !_peer->isEncrypted
491518

492-
// try to send next buffer
493-
total = _writeBuffer._buffer->length();
494-
written = 0;
495-
}
496-
497-
// write could have blocked which is the only acceptable error
498-
if (err && err != ::asio_ns::error::would_block) {
499-
LOG_TOPIC(DEBUG, Logger::COMMUNICATION) << "sync write on failed with: "
500-
<< err.message();
501-
closeStreamNoLock();
502-
return;
503-
}
519+
// we will be getting here in the following cases
520+
// - encrypted mode (SSL)
521+
// - we send only parts of the write buffer, but have more to send
522+
// - we got the error would_block/try_again when sending data
523+
// in this case we dispatch an async write
504524

505525
if (_abandoned.load(std::memory_order_acquire)) {
506526
return;
507527
}
508-
528+
529+
TRI_ASSERT(_writeBuffer._buffer != nullptr);
530+
509531
// so the code could have blocked at this point or not all data
510532
// was written in one go, begin writing at offset (written)
511533
auto self = shared_from_this();
534+
512535
_peer->asyncWrite(
513536
asio_ns::buffer(_writeBuffer._buffer->begin() + written, total - written),
514537
[self, this](const asio_ns::error_code& ec, std::size_t transferred) {
@@ -517,7 +540,8 @@ void SocketTask::asyncWriteSome() {
517540

518541
if (_abandoned.load(std::memory_order_acquire)) {
519542
return;
520-
} else if (ec) {
543+
}
544+
if (ec) {
521545
LOG_TOPIC(DEBUG, Logger::COMMUNICATION) << "write on failed with: "
522546
<< ec.message();
523547
closeStream();

‎arangod/Scheduler/SocketTask.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,9 @@ class SocketTask : virtual public Task {
5555

5656
public:
5757
bool start();
58+
59+
// whether or not this task can mix sync and async I/O
60+
virtual bool canUseMixedIO() const = 0;
5861

5962
protected:
6063
// caller will hold the _lock
@@ -144,11 +147,10 @@ class SocketTask : virtual public Task {
144147
// method returns true. Used for VST upgrade
145148
bool abandon() { return !(_abandoned.exchange(true)); }
146149

147-
/// lease a string buffer from pool
150+
// lease a string buffer from pool
148151
basics::StringBuffer* leaseStringBuffer(size_t length);
149152
void returnStringBuffer(basics::StringBuffer*);
150153

151-
protected:
152154
bool processAll();
153155
void triggerProcessAll();
154156

0 commit comments

Comments
 (0)
0