8000 fix undefined behavior in query cache result handling (#6544) · CoericK/arangodb@a93db9a · GitHub
[go: up one dir, main page]
Skip to content

Commit 8000 a93db9a

Browse files
jsteemannmchacki
jsteemann
authored and
mchacki
committed
fix undefined behavior in query cache result handling (arangodb#6544)
1 parent a6cde70 commit a93db9a

File tree

4 files changed

+65
-154
lines changed

4 files changed

+65
-154
lines changed

‎arangod/Aql/Query.cpp

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -578,8 +578,7 @@ ExecutionState Query::execute(QueryRegistry* registry, QueryResult& queryResult)
578578
auto cacheEntry = arangodb::aql::QueryCache::instance()->lookup(
579579
&_vocbase, queryHash, _queryString
580580
);
581-
arangodb::aql::QueryCacheResultEntryGuard guard(cacheEntry);
582-
581+
583582
if (cacheEntry != nullptr) {
584583
bool hasPermissions = true;
585584

@@ -599,11 +598,6 @@ ExecutionState Query::execute(QueryRegistry* registry, QueryResult& queryResult)
599598
// we don't have yet a transaction when we're here, so let's create
600599
// a mimimal context to build the result
601600
queryResult.context = transaction::StandaloneContext::Create(_vocbase);
602-
queryResult.extra = std::make_shared<VPackBuilder>();
603-
{
604-
VPackObjectBuilder guard(queryResult.extra.get(), true);
605-
addWarningsToVelocyPack(*queryResult.extra);
606-
}
607601
TRI_ASSERT(cacheEntry->_queryResult != nullptr);
608602
queryResult.result = cacheEntry->_queryResult;
609603
queryResult.extra = cacheEntry->_stats;
@@ -784,7 +778,6 @@ ExecutionState Query::executeV8(v8::Isolate* isolate, QueryRegistry* registry, Q
784778
auto cacheEntry = arangodb::aql::QueryCache::instance()->lookup(
785779
&_vocbase, queryHash, _queryString
786780
);
787-
arangodb::aql::QueryCacheResultEntryGuard guard(cacheEntry);
788781

789782
if (cacheEntry != nullptr) {
790783
bool hasPermissions = true;
@@ -974,11 +967,6 @@ ExecutionState Query::finalize(QueryResult& result) {
974967
return state;
975968
}
976969

977-
if (_cacheEntry != nullptr) {
978-
_cacheEntry->_stats = result.extra;
979-
QueryCache::instance()->store(&_vocbase, std::move(_cacheEntry));
980-
}
981-
982970
addWarningsToVelocyPack(*result.extra);
983971
double now = TRI_microtime();
984972
if (_profile != nullptr && _queryOptions.profile >= PROFILE_LEVEL_BASIC) {
@@ -987,6 +975,11 @@ ExecutionState Query::finalize(QueryResult& result) {
987975
}
988976
result.extra->close();
989977

978+
if (_cacheEntry != nullptr) {
979+
_cacheEntry->_stats = result.extra;
980+
QueryCache::instance()->store(&_vocbase, std::move(_cacheEntry));
981+
}
982+
990983
// patch executionTime stats value in place
991984
// we do this because "executionTime" should include the whole span of the execution and we have to set it at the very end
992985
double const rt = runTime(now);

‎arangod/Aql/QueryCache.cpp

Lines changed: 37 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -36,13 +36,13 @@ using namespace arangodb::aql;
3636

3737
namespace {
3838
/// @brief singleton instance of the query cache
39-
static arangodb::aql::QueryCache Instance;
39+
static arangodb::aql::QueryCache instance;
4040

4141
/// @brief maximum number of results in each per-database cache
42-
static size_t MaxResults = 128; // default value. can be changed later
42+
static std::atomic<size_t> maxResults(128); // default value. can be changed later
4343

4444
/// @brief whether or not the cache is enabled
45-
static std::atomic<arangodb::aql::QueryCacheMode> Mode(CACHE_ON_DEMAND);
45+
static std::atomic<arangodb::aql::QueryCacheMode> mode(CACHE_ON_DEMAND);
4646
}
4747

4848
/// @brief create a cache entry
@@ -55,34 +55,7 @@ QueryCacheResultEntry::QueryCacheResultEntry(
5555
_queryResult(queryResult),
5656
_dataSources(dataSources),
5757
_prev(nullptr),
58-
_next(nullptr),
59-
_refCount(0),
60-
_deletionRequested(0) {
61-
}
62-
63-
/// @brief check whether the element can be destroyed, and delete it if yes
64-
void QueryCacheResultEntry::tryDelete() {
65-
_deletionRequested = 1;
66-
67-
if (_refCount == 0) {
68-
delete this;
69-
}
70-
}
71-
72-
/// @brief use the element, so it cannot be deleted meanwhile
73-
void QueryCacheResultEntry::use() { ++_refCount; }
74-
75-
/// @brief unuse the element, so it can be deleted if required
76-
void QueryCacheResultEntry::unuse() {
77-
TRI_ASSERT(_refCount > 0);
78-
79-
if (--_refCount == 0) {
80-
if (_deletionRequested == 1) {
81-
// trigger the deletion
82-
delete this;
83-
}
84-
}
85-
}
58+
_next(nullptr) {}
8659

8760
/// @brief create a database-specific cache
8861
QueryCacheDatabaseEntry::QueryCacheDatabaseEntry()
@@ -96,16 +69,12 @@ QueryCacheDatabaseEntry::QueryCacheDatabaseEntry()
9669

9770
/// @brief destroy a database-specific cache
9871
QueryCacheDatabaseEntry::~QueryCacheDatabaseEntry() {
99-
for (auto& it : _entriesByHash) {
100-
tryDelete(it.second);
101-
}
102-
10372
_entriesByHash.clear();
10473
_entriesByDataSource.clear();
10574
}
10675

10776
/// @brief lookup a query result in the database-specific cache
108-
QueryCacheResultEntry* QueryCacheDatabaseEntry::lookup(
77+
std::shared_ptr<QueryCacheResultEntry> QueryCacheDatabaseEntry::lookup(
10978
uint64_t hash, QueryString const& queryString) {
11079
auto it = _entriesByHash.find(hash);
11180

@@ -124,26 +93,20 @@ QueryCacheResultEntry* QueryCacheDatabaseEntry::lookup(
12493
}
12594

12695
// found an entry
127-
auto entry = (*it).second;
128-
129-
// mark the entry as being used so noone else can delete it while it is in use
130-
entry->use();
131-
132-
return entry;
96+
return (*it).second;
13397
}
13498

13599
/// @brief store a query result in the database-specific cache
136100
void QueryCacheDatabaseEntry::store(uint64_t hash,
137-
QueryCacheResultEntry* entry) {
101+
std::shared_ptr<QueryCacheResultEntry> entry) {
138102
// insert entry into the cache
139103
if (!_entriesByHash.emplace(hash, entry).second) {
140104
// remove previous entry
141105
auto it = _entriesByHash.find(hash);
142106
TRI_ASSERT(it != _entriesByHash.end());
143107
auto previous = (*it).second;
144-
unlink(previous);
108+
unlink(previous.get());
145109
_entriesByHash.erase(it);
146-
tryDelete(previous);
147110

148111
// and insert again
149112
_entriesByHash.emplace(hash, entry);
@@ -178,19 +141,19 @@ void QueryCacheDatabaseEntry::store(uint64_t hash,
178141
TRI_ASSERT(it != _entriesByHash.end());
179142
auto previous = (*it).second;
180143
_entriesByHash.erase(it);
181-
unlink(previous);
182-
tryDelete(previous);
144+
unlink(previous.get());
183145
throw;
184146
}
185147

186-
link(entry);
148+
link(entry.get());
187149

188-
enforceMaxResults(::MaxResults);
150+
size_t mr = ::maxResults.load();
151+
enforceMaxResults(mr);
189152

190-
TRI_ASSERT(_numElements <= ::MaxResults);
153+
TRI_ASSERT(_numElements <= mr);
191154
TRI_ASSERT(_head != nullptr);
192155
TRI_ASSERT(_tail != nullptr);
193-
TRI_ASSERT(_tail == entry);
156+
TRI_ASSERT(_tail == entry.get());
194157
TRI_ASSERT(entry->_next == nullptr);
195158
}
196159

@@ -218,20 +181,18 @@ void QueryCacheDatabaseEntry::invalidate(std::string const& dataSource) {
218181
if (it3 != _entriesByHash.end()) {
219182
// remove entry from the linked list
220183
auto entry = (*it3).second;
221-
unlink(entry);
184+
unlink(entry.get());
222185

223186
// erase it from hash table
224187
_entriesByHash.erase(it3);
225-
226-
// delete the object itself
227-
tryDelete(entry);
228188
}
229189
}
230190

231191
_entriesByDataSource.erase(it);
232192
}
233193

234194
/// @brief enforce maximum number of results
195+
/// must be called under the shard's lock
235196
void QueryCacheDatabaseEntry::enforceMaxResults(size_t value) {
236197
while (_numElements > value) {
237198
// too many elements. now wipe the first element from the list
@@ -242,15 +203,9 @@ void QueryCacheDatabaseEntry::enforceMaxResults(size_t value) {
242203
auto it = _entriesByHash.find(head->_hash);
243204
TRI_ASSERT(it != _entriesByHash.end());
244205
_entriesByHash.erase(it);
245-
tryDelete(head);
246206
}
247207
}
248208

249-
/// @brief check whether the element can be destroyed, and delete it if yes
250-
void QueryCacheDatabaseEntry::tryDelete(QueryCacheResultEntry* e) {
251-
e->tryDelete();
252-
}
253-
254209
/// @brief unlink the result entry from the list
255210
void QueryCacheDatabaseEntry::unlink(QueryCacheResultEntry* e) {
256211
if (e->_prev != nullptr) {
@@ -313,7 +268,7 @@ VPackBuilder QueryCache::properties() {
313268
VPackBuilder builder;
314269
builder.openObject();
315270
builder.add("mode", VPackValue(modeString(mode())));
316-
builder.add("maxResults", VPackValue(::MaxResults));
271+
builder.add("maxResults", VPackValue(::maxResults.load()));
317272
builder.close();
318273

319274
return builder;
@@ -324,7 +279,7 @@ void QueryCache::properties(std::pair<std::string, size_t>& result) {
324279
MUTEX_LOCKER(mutexLocker, _propertiesLock);
325280

326281
result.first = modeString(mode());
327-
result.second = ::MaxResults;
282+
result.second = ::maxResults.load();
328283
}
329284

330285
/// @brief set the cache properties
@@ -343,7 +298,7 @@ bool QueryCache::mayBeActive() const { return (mode() != CACHE_ALWAYS_OFF); }
343298

344299
/// @brief return whether or not the query cache is enabled
345300
QueryCacheMode QueryCache::mode() const {
346-
return ::Mode.load(std::memory_order_relaxed);
301+
return ::mode.load(std::memory_order_relaxed);
347302
}
348303

349304
/// @brief return a string version of the mode
@@ -362,8 +317,8 @@ std::string QueryCache::modeString(QueryCacheMode mode) {
362317
}
363318

364319
/// @brief lookup a query result in the cache
365-
QueryCacheResultEntry* QueryCache::lookup(TRI_vocbase_t* vocbase, uint64_t hash,
366-
QueryString const& queryString) {
320+
std::shared_ptr<QueryCacheResultEntry> QueryCache::lookup(TRI_vocbase_t* vocbase, uint64_t hash,
321+
QueryString const& queryString) {
367322
auto const part = getPart(vocbase);
368323
READ_LOCKER(readLocker, _entriesLock[part]);
369324

@@ -380,21 +335,21 @@ QueryCacheResultEntry* QueryCache::lookup(TRI_vocbase_t* vocbase, uint64_t hash,
380335
/// @brief store a query in the cache
381336
/// if the call is successful, the cache has taken over ownership for the
382337
/// query result!
383-
QueryCacheResultEntry* QueryCache::store(
338+
void QueryCache::store(
384339
TRI_vocbase_t* vocbase, uint64_t hash, QueryString const& queryString,
385340
std::shared_ptr<VPackBuilder> const& result,
386341
std::shared_ptr<VPackBuilder> const& stats,
387342
std::vector<std::string>&& dataSources) {
388343

389344
if (!result->slice().isArray()) {
390-
return nullptr;
345+
return;
391346
}
392347

393348
// get the right part of the cache to store the result in
394349
auto const part = getPart(vocbase);
395350

396351
// create the cache entry outside the lock
397-
auto entry = std::make_unique<QueryCacheResultEntry>(
352+
auto entry = std::make_shared<QueryCacheResultEntry>(
398353
hash, queryString, result, std::move(dataSources));
399354

400355
WRITE_LOCKER(writeLocker, _entriesLock[part]);
@@ -404,17 +359,15 @@ QueryCacheResultEntry* QueryCache::store(
404359
if (it == _entries[part].end()) {
405360
// create entry for the current database
406361
auto db = std::make_unique<QueryCacheDatabaseEntry>();
407-
it = _entries[part].emplace(vocbase, db.get()).first;
408-
db.release();
362+
it = _entries[part].emplace(vocbase, std::move(db)).first;
409363
}
410364

411365
// store cache entry
412-
(*it).second->store(hash, entry.get());
413-
return entry.release();
366+
(*it).second->store(hash, entry);
414367
}
415368

416369
/// @brief store a query in the cache
417-
void QueryCache::store(TRI_vocbase_t* vocbase, std::unique_ptr<QueryCacheResultEntry> entry) {
370+
void QueryCache::store(TRI_vocbase_t* vocbase, std::shared_ptr<QueryCacheResultEntry> entry) {
418371
// get the right part of the cache to store the result in
419372
auto const part = getPart(vocbase);
420373

@@ -425,13 +378,11 @@ void QueryCache::store(TRI_vocbase_t* vocbase, std::unique_ptr<QueryCacheResultE
425378
if (it == _entries[part].end()) {
426379
// create entry for the current database
427380
auto db = std::make_unique<QueryCacheDatabaseEntry>();
428-
it = _entries[part].emplace(vocbase, db.get()).first;
429-
db.release();
381+
it = _entries[part].emplace(vocbase, std::move(db)).first;
430382
}
431383

432384
// store cache entry
433-
(*it).second->store(entry->_hash, entry.get());
434-
entry.release();
385+
(*it).second->store(entry->_hash, entry);
435386
}
436387

437388
/// @brief invalidate all queries for the given data sources
@@ -467,7 +418,7 @@ void QueryCache::invalidate(TRI_vocbase_t* vocbase, std::string const& dataSourc
467418

468419
/// @brief invalidate all queries for a particular database
469420
void QueryCache::invalidate(TRI_vocbase_t* vocbase) {
470-
QueryCacheDatabaseEntry* databaseQueryCache = nullptr;
421+
std::unique_ptr<QueryCacheDatabaseEntry> databaseQueryCache;
471422

472423
{
473424
auto const part = getPart(vocbase);
@@ -479,13 +430,12 @@ void QueryCache::invalidate(TRI_vocbase_t* vocbase) {
479430
return;
480431
}
481432

482-
databaseQueryCache = (*it).second;
433+
databaseQueryCache = std::move((*it).second);
483434
_entries[part].erase(it);
484435
}
485436

486437
// delete without holding the lock
487438
TRI_ASSERT(databaseQueryCache != nullptr);
488-
delete databaseQueryCache;
489439
}
490440

491441
/// @brief invalidate all queries
@@ -504,7 +454,7 @@ void QueryCache::invalidate() {
504454
}
505455

506456
/// @brief get the query cache instance
507-
QueryCache* QueryCache::instance() { return &::Instance; }
457+
QueryCache* QueryCache::instance() { return &::instance; }
508458

509459
/// @brief enforce maximum number of elements in each database-specific cache
510460
void QueryCache::enforceMaxResults(size_t value) {
@@ -527,10 +477,6 @@ unsigned int QueryCache::getPart(TRI_vocbase_t const* vocbase) const {
527477
/// @brief invalidate all entries in the cache part
528478
/// note that the caller of this method must hold the write lock
529479
void QueryCache::invalidate(unsigned int part) {
530-
for (auto& it : _entries[part]) {
531-
delete it.second;
532-
}
533-
534480
_entries[part].clear();
535481
}
536482

@@ -540,11 +486,13 @@ void QueryCache::setMaxResults(size_t value) {
540486
return;
541487
}
542488

543-
if (value > ::MaxResults) {
489+
size_t mr = ::maxResults.load();
490+
491+
if (value > mr) {
544492
enforceMaxResults(value);
545493
}
546494

547-
::MaxResults = value;
495+
mr = value;
548496
}
549497

550498
/// @brief sets the caching mode
@@ -556,7 +504,7 @@ void QueryCache::setMode(QueryCacheMode value) {
556504

557505
invalidate();
558506

559-
::Mode.store(value, std::memory_order_release);
507+
::mode.store(value, std::memory_order_release);
560508
}
561509

562510
/// @brief enable or disable the query cache

0 commit comments

Comments
 (0)
0