8000 backport some changes from https://github.com/arangodb/arangodb/pull/… · arangodb/arangodb@d4f4219 · GitHub
[go: up one dir, main page]
Skip to content
8000

Commit d4f4219

Browse files
jsteemannKVS85
jsteemann
and
KVS85
authored
backport some changes from #13507 (#13520)
* backport some changes from #13507 This is a preparation PR for backporting some further replication tests and bugfixes from devel to 3.7. It isolates some of the changes done in #13507 into a small PR to facilitate reviewing. A further PR with the added replication tests and a bugfix for 3.7 will follow up. This PR extends the length of some internal byte buffers from 11 to 21, so that they cannot overflow, regardless of what input is coming in. The overflow was theoretically possible before, but only with manipulated timestamp input. The PR also fixes an issue in a RocksDB heap helper class that does a potentially unsafe self-move assignment. This issue was raised when running tests with a debug STL build (`_GLIBCXX_DEBUG`). * Update CHANGELOG Co-authored-by: Vadim <vadim@arangodb.com>
1 parent 0d0a0d7 commit d4f4219

16 files changed

+74
-20
lines changed

‎3rdParty/rocksdb/6.8/util/heap.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,9 @@ class BinaryHeap {
6666

6767
void replace_top(T&& value) {
6868
assert(!empty());
69-
data_.front() = std::move(value);
69+
if (data_.size() > 1) {
70+
data_.front() = std::move(data_.back());
71+
}
7072
downheap(get_root());
7173
}
7274

‎CHANGELOG

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
v3.7.8 (XXXX-XX-XX)
22
-------------------
33

4+
* Fixed a RocksDB bug which could lead to an assertion failure when compiling
5+
with STL debug mode -D_GLIBCXX_DEBUG.
6+
7+
* Fixed a rare internal buffer overflow around ridBuffers.
8+
49
* Issue #13141: The `move-filters-into-enumerate` optimization, when applied to
510
an EnumerateCollectionNode (i.e. full collection scan), did not do regular
611
checks for the query being killed during the filtering of documents, resulting

‎arangod/IResearch/IResearchFeature.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -323,7 +323,7 @@ bool upgradeArangoSearchLinkCollectionName(TRI_vocbase_t& vocbase,
323323
break;
324324
}
325325
std::this_thread::sleep_for(std::chrono::milliseconds(500));
326-
} while(--tryCount);
326+
} while (--tryCount);
327327
} else {
328328
clusterCollectionName = collection->name();
329329
}
@@ -351,7 +351,7 @@ bool upgradeArangoSearchLinkCollectionName(TRI_vocbase_t& vocbase,
351351
auto builder =
352352
collection->toVelocyPackIgnore({"path", "statusString"},
353353
arangodb::LogicalDataSource::Serialization::PersistenceWithInProgress);
354-
auto res =
354+
[[maybe_unused]] arangodb::Result res =
355355
engine.writeCreateCollectionMarker(vocbase.id(), collection->id(),
356356
builder.slice(),
357357
arangodb::RocksDBLogValue::Empty());

‎arangod/Indexes/Index.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -979,7 +979,7 @@ void Index::warmup(arangodb::transaction::Methods*, std::shared_ptr<basics::Loca
979979

980980
/// @brief generate error message
981981
/// @param key the conflicting key
982-
Result& Index::addErrorMsg(Result& r, std::string const& key) {
982+
Result& Index::addErrorMsg(Result& r, std::string const& key) const {
983983
// now provide more context based on index
984984
r.appendErrorMessage(" - in index ");
985985
r.appendErrorMessage(name());

‎arangod/Indexes/Index.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -442,7 +442,7 @@ class Index {
442442
/// @param code the error key
443443
/// @param key the conflicting key
444444
arangodb::Result& addErrorMsg(Result& r, int code,
445-
std::string const& key = "") {
445+
std::string const& key = "") const {
446446
if (code != TRI_ERROR_NO_ERROR) {
447447
r.reset(code);
448448
return addErrorMsg(r, key);
@@ -452,7 +452,7 @@ class Index {
452452

453453
/// @brief generate error result
454454
/// @param key the conflicting key
455-
arangodb::Result& addErrorMsg(Result& r, std::string const& key = "");
455+
arangodb::Result& addErrorMsg(Result& r, std::string const& key = "") const;
456456

457457
/// @brief extracts a timestamp value from a document
458458
/// returns a negative value if the document does not contain the specified

‎arangod/Replication/DatabaseInitialSyncer.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ arangodb::Result fetchRevisions(arangodb::transaction::Methods& trx,
189189
std::size_t current = 0;
190190
auto guard = arangodb::scopeGuard(
191191
[&current, &stats]() -> void { stats.numDocsRequested += current; });
192-
char ridBuffer[11];
192+
char ridBuffer[arangodb::basics::maxUInt64StringSize];
193193
std::unique_ptr<arangodb::httpclient::SimpleHttpResult> response;
194194
while (current < toFetch.size()) {
195195
arangodb::transaction::BuilderLeaser requestBuilder(&trx);
@@ -1415,7 +1415,7 @@ Result DatabaseInitialSyncer::fetchCollectionSyncByRevisions(arangodb::LogicalCo
14151415
{
14161416
VPackBuilder requestBuilder;
14171417
{
1418-
char ridBuffer[11];
1418+
char ridBuffer[arangodb::basics::maxUInt64StringSize];
14191419
VPackArrayBuilder list(&requestBuilder);
14201420
for (auto& pair : ranges) {
14211421
VPackArrayBuilder range(&requestBuilder);

‎arangod/RestHandler/RestReplicationHandler.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1685,7 +1685,7 @@ Result RestReplicationHandler::processRestoreDataBatch(transaction::Methods& trx
16851685
if (!physical) {
16861686
return TRI_ERROR_ARANGO_DATA_SOURCE_NOT_FOUND;
16871687
}
1688-
char ridBuffer[11];
1688+
char ridBuffer[arangodb::basics::maxUInt64StringSize];
16891689

16901690
if (collection->hasClusterWideUniqueRevs()) {
16911691
generateNewRevisionIds = true;
@@ -3196,7 +3196,7 @@ void RestReplicationHandler::handleCommandRevisionRanges() {
31963196
};
31973197
setRange(current);
31983198

3199-
char ridBuffer[11];
3199+
char ridBuffer[arangodb::basics::maxUInt64StringSize];
32003200
{
32013201
TRI_ASSERT(response.isOpenObject());
32023202
VPackArrayBuilder rangesGuard(&response, StaticStrings::RevisionTreeRanges);

‎arangod/RocksDBEngine/RocksDBIncrementalSync.cpp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -435,6 +435,10 @@ Result syncChunkRocksDB(DatabaseInitialSyncer& syncer, SingleCollectionTransacti
435435
": response is no array");
436436
}
437437

438+
syncer.setProgress(std::string("applying documents chunk ") +
439+
std::to_string(chunkId) + " (" + std::to_string(toFetch.size()) +
440+
" keys) for collection '" + collectionName + "'");
441+
438442
size_t foundLength = slice.length();
439443

440444
double t = TRI_microtime();
@@ -773,7 +777,7 @@ Result handleSyncKeysRocksDB(DatabaseInitialSyncer& syncer,
773777

774778
tempBuilder.clear();
775779
// use a temporary char buffer for building to rid string
776-
char ridBuffer[21];
780+
char ridBuffer[arangodb::basics::maxUInt64StringSize];
777781
tempBuilder.add(TRI_RidToValuePair(docRev, &ridBuffer[0]));
778782
localHash ^= tempBuilder.slice().hashString(); // revision as string
779783

‎arangod/RocksDBEngine/RocksDBReplicationContext.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include "Basics/MutexLocker.h"
2929
#include "Basics/StaticStrings.h"
3030
#include "Basics/StringBuffer.h"
31+
#include "Basics/StringUtils.h"
3132
#include "Basics/VPackStringBufferAdapter.h"
3233
#include "Basics/system-functions.h"
3334
#include "Logger/Logger.h"
@@ -480,7 +481,7 @@ arangodb::Result RocksDBReplicationContext::dumpKeyChunks(TRI_vocbase_t& vocbase
480481

481482
// reserve some space in the result builder to avoid frequent reallocations
482483
b.reserve(8192);
483-
char ridBuffer[21]; // temporary buffer for stringifying revision ids
484+
char ridBuffer[arangodb::basics::maxUInt64StringSize]; // temporary buffer for stringifying revision ids
484485
RocksDBKey docKey;
485486
VPackBuilder tmpHashBuilder;
486487
rocksdb::TransactionDB* db = globalRocksDB();
@@ -642,7 +643,7 @@ arangodb::Result RocksDBReplicationContext::dumpKeys(TRI_vocbase_t& vocbase,
642643

643644
// reserve some space in the result builder to avoid frequent reallocations
644645
b.reserve(8192);
645-
char ridBuffer[21]; // temporary buffer for stringifying revision ids
646+
char ridBuffer[arangodb::basics::maxUInt64StringSize]; // temporary buffer for stringifying revision ids
646647
rocksdb::TransactionDB* db = globalRocksDB();
647648
auto* rcoll = static_cast<RocksDBMetaCollection*>(cIter->logical->getPhysical());
648649
const uint64_t cObjectId = rcoll->objectId();

‎arangod/RocksDBEngine/RocksDBTransactionState.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,8 +229,11 @@ class RocksDBKeyLeaser {
229229
~RocksDBKeyLeaser();
230230
inline RocksDBKey* builder() { return &_key; }
231231
inline RocksDBKey* operator->() { return &_key; }
232+
inline RocksDBKey const* operator->() const { return &_key; }
232233
inline RocksDBKey* get() { return &_key; }
234+
inline RocksDBKey const* get() const { return &_key; }
233235
inline RocksDBKey& ref() { return _key; }
236+
inline RocksDBKey const& ref() const { return _key; }
234237

235238
private:
236239
transaction::Context* _ctx;

‎arangod/StorageEngine/PhysicalCollection.cpp

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
#include "Basics/Exceptions.h"
2828
#include "Basics/ReadLocker.h"
2929
#include "Basics/StaticStrings.h"
30+
#include "Basics/StringUtils.h"
3031
#include "Basics/VelocyPackHelper.h"
3132
#include "Basics/WriteLocker.h"
3233
#include "Basics/encoding.h"
@@ -287,7 +288,7 @@ Result PhysicalCollection::mergeObjectsForUpdate(
287288
}
288289
if (!handled) {
289290
// temporary buffer for stringifying revision ids
290-
char ridBuffer[21];
291+
char ridBuffer[arangodb::basics::maxUInt64StringSize];
291292
revisionId = newRevisionId();
292293
b.add(StaticStrings::RevString, TRI_RidToValuePair(revisionId, &ridBuffer[0]));
293294
}
@@ -441,7 +442,7 @@ Result PhysicalCollection::newObjectForInsert(transaction::Methods*,
441442
}
442443
if (!handled) {
443444
// temporary buffer for stringifying revision ids
444-
char ridBuffer[21];
445+
char ridBuffer[arangodb::basics::maxUInt64StringSize];
445446
revisionId = newRevisionId();
446447 F438
builder.add(StaticStrings::RevString, TRI_RidToValuePair(revisionId, &ridBuffer[0]));
447448
}
@@ -469,7 +470,7 @@ void PhysicalCollection::newObjectForRemove(transaction::Methods*,
469470
}
470471

471472
// temporary buffer for stringifying revision ids
472-
char ridBuffer[21];
473+
char ridBuffer[arangodb::basics::maxUInt64StringSize];
473474
revisionId = newRevisionId();
474475
builder.add(StaticStrings::RevString, TRI_RidToValuePair(revisionId, &ridBuffer[0]));
475476
builder.close();
@@ -530,7 +531,7 @@ Result PhysicalCollection::newObjectForReplace(transaction::Methods*,
530531
}
531532
if (!handled) {
532533
// temporary buffer for stringifying revision ids
533-
char ridBuffer[21];
534+
char ridBuffer[arangodb::basics::maxUInt64StringSize];
534535
revisionId = newRevisionId();
535536
builder.add(StaticStrings::RevString, TRI_RidToValuePair(revisionId, &ridBuffer[0]));
536537
}

‎arangod/StorageEngine/TransactionState.cpp

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,13 @@ TransactionState::TransactionState(TRI_vocbase_t& vocbase,
6262
_serverRole(ServerState::instance()->getRole()),
6363
_options(options),
6464
_id(tid),
65-
_registeredTransaction(false) {}
65+
_registeredTransaction(false) {
66+
67+
// patch intermediateCommitCount for testing
68+
#ifdef ARANGODB_ENABLE_FAILURE_TESTS
69+
transaction::Options::adjustIntermediateCommitCount(_options);
70+
#endif
71+
}
6672

6773
/// @brief free a transaction container
6874
TransactionState::~TransactionState() {

‎arangod/Transaction/Methods.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -437,7 +437,7 @@ void transaction::Methods::buildDocumentIdentity(
437437
builder.add(StaticStrings::KeyString,
438438
VPackValuePair(key.data(), key.length(), VPackValueType::String));
439439

440-
char ridBuffer[21];
440+
char ridBuffer[arangodb::basics::maxUInt64StringSize];
441441
builder.add(StaticStrings::RevString, TRI_RidToValuePair(rid, &ridBuffer[0]));
442442

443443
if (oldRid != 0) {

‎arangod/Transaction/Options.cpp

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,12 @@ Options::Options()
4646
skipInaccessibleCollections(false),
4747
#endif
4848
waitForSync(false),
49-
isFollowerTransaction(false) {}
49+
isFollowerTransaction(false) {
50+
#ifdef ARANGODB_ENABLE_FAILURE_TESTS
51+
// patch intermediateCommitCount for testing
52+
adjustIntermediateCommitCount(*this);
53+
#endif
54+
}
5055

5156
Options Options::replicationDefaults() {
5257
Options options;
@@ -102,8 +107,13 @@ void Options::fromVelocyPack(arangodb::velocypack::Slice const& slice) {
102107
if (value.isBool()) {
103108
isFollowerTransaction = value.getBool();
104109
}
110+
105111
// we are intentionally *not* reading allowImplicitCollectionForWrite here.
106112
// this is an internal option only used in replication
113+
#ifdef ARANGODB_ENABLE_FAILURE_TESTS
114+
// patch intermediateCommitCount for testing
115+
adjustIntermediateCommitCount(*this);
116+
#endif
107117
}
108118

109119
/// @brief add the options to an opened vpack builder
@@ -123,3 +133,18 @@ void Options::toVelocyPack(arangodb::velocypack::Builder& builder) const {
123133
// this is an internal option only used in replication
124134
builder.add("isFollowerTransaction", VPackValue(isFollowerTransaction));
125135
}
136+
137+
#ifdef ARANGODB_ENABLE_FAILURE_TESTS
138+
/// @brief patch intermediateCommitCount for testing
139+
/*static*/ void Options::adjustIntermediateCommitCount(Options& options) {
140+
TRI_IF_FAILURE("TransactionState::intermediateCommitCount100") {
141+
options.intermediateCommitCount = 100;
142+
}
143+
TRI_IF_FAILURE("TransactionState::intermediateCommitCount1000") {
144+
options.intermediateCommitCount = 1000;
145+
}
146+
TRI_IF_FAILURE("TransactionState::intermediateCommitCount10000") {
147+
options.intermediateCommitCount = 10000;
148+
}
149+
}
150+
#endif

‎arangod/Transaction/Options.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,11 @@ struct Options {
5252
/// @brief add the options to an opened vpack builder
5353
void toVelocyPack(arangodb::velocypack::Builder&) const;
5454

55+
#ifdef ARANGODB_ENABLE_FAILURE_TESTS
56+
/// @brief patch intermediateCommitCount for testing
57+
static void adjustIntermediateCommitCount(Options& options);
58+
#endif
59+
5560
static constexpr double defaultLockTimeout = 900.0;
5661
static uint64_t defaultMaxTransactionSize;
5762
static uint64_t defaultIntermediateCommitSize;

‎lib/Basics/StringUtils.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,8 @@
4545
namespace arangodb {
4646
namespace basics {
4747

48+
static constexpr size_t maxUInt64StringSize = 21;
49+
4850
/// @brief collection of string utility functions
4951
///
5052
/// This namespace holds function used for string manipulation.

0 commit comments

Comments
 (0)
0