From 634574ed9f7066b1fa060a1119ef879668c715c9 Mon Sep 17 00:00:00 2001 From: Jan Date: Fri, 28 Jul 2017 12:08:37 +0200 Subject: [PATCH] don't mask errors with fake OOM messages (#2872) --- arangod/RocksDBEngine/RocksDBCollection.cpp | 6 +- arangod/RocksDBEngine/RocksDBIndexFactory.cpp | 180 ++++++++---------- arangod/RocksDBEngine/RocksDBVPackIndex.cpp | 16 +- 3 files changed, 90 insertions(+), 112 deletions(-) diff --git a/arangod/RocksDBEngine/RocksDBCollection.cpp b/arangod/RocksDBEngine/RocksDBCollection.cpp index b05aba5c13..28f1f625a1 100644 --- a/arangod/RocksDBEngine/RocksDBCollection.cpp +++ b/arangod/RocksDBEngine/RocksDBCollection.cpp @@ -381,7 +381,7 @@ static std::shared_ptr findIndex( if (!value.isString()) { // Compatibility with old v8-vocindex. - THROW_ARANGO_EXCEPTION(TRI_ERROR_OUT_OF_MEMORY); + THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL, "invalid index type definition"); } std::string tmp = value.copyString(); @@ -1369,7 +1369,7 @@ RocksDBOperationResult RocksDBCollection::insertDocument( for (std::shared_ptr const& idx : _indexes) { innerRes.reset(idx->insert(trx, revisionId, doc, false)); - // in case of no-memory, return immediately + // in case of OOM return immediately if (innerRes.is(TRI_ERROR_OUT_OF_MEMORY)) { return innerRes; } @@ -1433,7 +1433,7 @@ RocksDBOperationResult RocksDBCollection::removeDocument( Result tmpres = idx->remove(trx, revisionId, doc, false); resInner.reset(tmpres); - // in case of no-memory, return immediately + // in case of OOM return immediately if (resInner.is(TRI_ERROR_OUT_OF_MEMORY)) { return resInner; } diff --git a/arangod/RocksDBEngine/RocksDBIndexFactory.cpp b/arangod/RocksDBEngine/RocksDBIndexFactory.cpp index f13c602336..0984ce80b6 100644 --- a/arangod/RocksDBEngine/RocksDBIndexFactory.cpp +++ b/arangod/RocksDBEngine/RocksDBIndexFactory.cpp @@ -57,42 +57,38 @@ static int ProcessIndexFields(VPackSlice const definition, TRI_ASSERT(builder.isOpenObject()); std::unordered_set fields; - try { - VPackSlice fieldsSlice = definition.get("fields"); - builder.add(VPackValue("fields")); - builder.openArray(); - if (fieldsSlice.isArray()) { - // "fields" is a list of fields - for (auto const& it : VPackArrayIterator(fieldsSlice)) { - if (!it.isString()) { - return TRI_ERROR_BAD_PARAMETER; - } - - StringRef f(it); - - if (f.empty() || (create && f == StaticStrings::IdString)) { - // accessing internal attributes is disallowed - return TRI_ERROR_BAD_PARAMETER; - } - - if (fields.find(f) != fields.end()) { - // duplicate attribute name - return TRI_ERROR_BAD_PARAMETER; - } - - fields.insert(f); - builder.add(it); + VPackSlice fieldsSlice = definition.get("fields"); + builder.add(VPackValue("fields")); + builder.openArray(); + if (fieldsSlice.isArray()) { + // "fields" is a list of fields + for (auto const& it : VPackArrayIterator(fieldsSlice)) { + if (!it.isString()) { + return TRI_ERROR_BAD_PARAMETER; } - } - if (fields.empty() || (numFields > 0 && (int)fields.size() != numFields)) { - return TRI_ERROR_BAD_PARAMETER; - } + StringRef f(it); - builder.close(); - } catch (...) { - return TRI_ERROR_OUT_OF_MEMORY; + if (f.empty() || (create && f == StaticStrings::IdString)) { + // accessing internal attributes is disallowed + return TRI_ERROR_BAD_PARAMETER; + } + + if (fields.find(f) != fields.end()) { + // duplicate attribute name + return TRI_ERROR_BAD_PARAMETER; + } + + fields.insert(f); + builder.add(it); + } } + + if (fields.empty() || (numFields > 0 && (int)fields.size() != numFields)) { + return TRI_ERROR_BAD_PARAMETER; + } + + builder.close(); return TRI_ERROR_NO_ERROR; } @@ -293,77 +289,65 @@ int RocksDBIndexFactory::enhanceIndexDefinition(VPackSlice const definition, } TRI_ASSERT(enhanced.isEmpty()); + + VPackObjectBuilder b(&enhanced); + current = definition.get("id"); + uint64_t id = 0; + if (current.isNumber()) { + id = current.getNumericValue(); + } else if (current.isString()) { + id = basics::StringUtils::uint64(current.copyString()); + } + if (id > 0) { + enhanced.add("id", VPackValue(std::to_string(id))); + } + + if (create && !isCoordinator) { + if (!definition.hasKey("objectId")) { + enhanced.add("objectId", + VPackValue(std::to_string(TRI_NewTickServer()))); + } + } + + enhanced.add("type", VPackValue(Index::oldtypeName(type))); + int res = TRI_ERROR_INTERNAL; - try { - VPackObjectBuilder b(&enhanced); - current = definition.get("id"); - uint64_t id = 0; - if (current.isNumber()) { - id = current.getNumericValue(); - } else if (current.isString()) { - id = basics::StringUtils::uint64(current.copyString()); - } - if (id > 0) { - enhanced.add("id", VPackValue(std::to_string(id))); + switch (type) { + case Index::TRI_IDX_TYPE_PRIMARY_INDEX: + case Index::TRI_IDX_TYPE_EDGE_INDEX: { + break; } - if (create && !isCoordinator) { - if (!definition.hasKey("objectId")) { - enhanced.add("objectId", - VPackValue(std::to_string(TRI_NewTickServer()))); - } + case Index::TRI_IDX_TYPE_GEO1_INDEX: + res = EnhanceJsonIndexGeo1(definition, enhanced, create); + break; + + case Index::TRI_IDX_TYPE_GEO2_INDEX: + res = EnhanceJsonIndexGeo2(definition, enhanced, create); + break; + + case Index::TRI_IDX_TYPE_HASH_INDEX: + res = EnhanceJsonIndexHash(definition, enhanced, create); + break; + + case Index::TRI_IDX_TYPE_SKIPLIST_INDEX: + res = EnhanceJsonIndexSkiplist(definition, enhanced, create); + break; + + case Index::TRI_IDX_TYPE_PERSISTENT_INDEX: + res = EnhanceJsonIndexPersistent(definition, enhanced, create); + break; + + case Index::TRI_IDX_TYPE_FULLTEXT_INDEX: + res = EnhanceJsonIndexFulltext(definition, enhanced, create); + break; + + case Index::TRI_IDX_TYPE_UNKNOWN: + default: { + res = TRI_ERROR_BAD_PARAMETER; + break; } - // breaks lookupIndex() - /*else { - if (!definition.hasKey("objectId")) { - // objectId missing, but must be present - return TRI_ERROR_INTERNAL; - } - }*/ - - enhanced.add("type", VPackValue(Index::oldtypeName(type))); - - switch (type) { - case Index::TRI_IDX_TYPE_PRIMARY_INDEX: - case Index::TRI_IDX_TYPE_EDGE_INDEX: { - break; - } - - case Index::TRI_IDX_TYPE_GEO1_INDEX: - res = EnhanceJsonIndexGeo1(definition, enhanced, create); - break; - - case Index::TRI_IDX_TYPE_GEO2_INDEX: - res = EnhanceJsonIndexGeo2(definition, enhanced, create); - break; - - case Index::TRI_IDX_TYPE_HASH_INDEX: - res = EnhanceJsonIndexHash(definition, enhanced, create); - break; - - case Index::TRI_IDX_TYPE_SKIPLIST_INDEX: - res = EnhanceJsonIndexSkiplist(definition, enhanced, create); - break; - - case Index::TRI_IDX_TYPE_PERSISTENT_INDEX: - res = EnhanceJsonIndexPersistent(definition, enhanced, create); - break; - - case Index::TRI_IDX_TYPE_FULLTEXT_INDEX: - res = EnhanceJsonIndexFulltext(definition, enhanced, create); - break; - - case Index::TRI_IDX_TYPE_UNKNOWN: - default: { - res = TRI_ERROR_BAD_PARAMETER; - break; - } - } - - } catch (...) { - // TODO Check for different type of Errors - return TRI_ERROR_OUT_OF_MEMORY; } return res; diff --git a/arangod/RocksDBEngine/RocksDBVPackIndex.cpp b/arangod/RocksDBEngine/RocksDBVPackIndex.cpp index c9197a9c92..078b4fecd8 100644 --- a/arangod/RocksDBEngine/RocksDBVPackIndex.cpp +++ b/arangod/RocksDBEngine/RocksDBVPackIndex.cpp @@ -532,13 +532,10 @@ Result RocksDBVPackIndex::insertInternal(transaction::Methods* trx, std::vector elements; std::vector hashes; int res; - try { + { + // rethrow all types of exceptions from here... transaction::BuilderLeaser leased(trx); res = fillElement(*(leased.get()), revisionId, doc, elements, hashes); - } catch (basics::Exception const& ex) { - res = ex.code(); - } catch (...) { - res = TRI_ERROR_OUT_OF_MEMORY; } if (res != TRI_ERROR_NO_ERROR) { @@ -604,14 +601,11 @@ Result RocksDBVPackIndex::removeInternal(transaction::Methods* trx, std::vector hashes; int res; - try { + { + // rethrow all types of exceptions from here... transaction::BuilderLeaser leased(trx); res = fillElement(*(leased.get()), revisionId, doc, elements, hashes); - } catch (basics::Exception const& ex) { - res = ex.code(); - } catch (...) { - res = TRI_ERROR_OUT_OF_MEMORY; - } + } if (res != TRI_ERROR_NO_ERROR) { return IndexResult(res, this);