tursom/memgraph
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Don't crash the database when using utils::InputFile

Browse Source 
Reviewers: teon.banek

Reviewed By: teon.banek

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D2460
This commit is contained in:
Matej Ferencevic 2019-09-30 16:35:40 +02:00
parent 8c7c593680
commit 3756534490
5 changed files with 64 additions and 55 deletions

10
src/storage/v2/durability.cpp
View File

@ -132,7 +132,7 @@ void Encoder::Finalize() {
std::optional<uint64_t> Decoder::Initialize(const std::filesystem::path &path,
const std::string &magic) {
file_.Open(path);
if (!file_.Open(path)) return std::nullopt;
std::string file_magic(magic.size(), '\0');
if (!Read(reinterpret_cast<uint8_t *>(file_magic.data()), file_magic.size()))
return std::nullopt;
@ -366,12 +366,12 @@ bool Decoder::SkipPropertyValue() {
}
}
uint64_t Decoder::GetSize() { return file_.GetSize(); }
std::optional<uint64_t> Decoder::GetSize() { return file_.GetSize(); }
uint64_t Decoder::GetPosition() { return file_.GetPosition(); }
std::optional<uint64_t> Decoder::GetPosition() { return file_.GetPosition(); }
void Decoder::SetPosition(uint64_t position) {
file_.SetPosition(utils::InputFile::Position::SET, position);
bool Decoder::SetPosition(uint64_t position) {
return !!file_.SetPosition(utils::InputFile::Position::SET, position);
}
} // namespace storage::durability

6
src/storage/v2/durability.hpp
View File

@ -102,9 +102,9 @@ class Decoder final {
bool SkipString();
bool SkipPropertyValue();
uint64_t GetSize();
uint64_t GetPosition();
void SetPosition(uint64_t position);
std::optional<uint64_t> GetSize();
std::optional<uint64_t> GetPosition();
bool SetPosition(uint64_t position);
private:
utils::InputFile file_;

50
src/utils/file.cpp
View File

@ -63,9 +63,7 @@ bool CopyFile(const std::filesystem::path &src,
static_assert(std::is_same_v<off_t, ssize_t>, "off_t must fit into ssize_t!");
InputFile::~InputFile() {
if (IsOpen()) Close();
}
InputFile::~InputFile() { Close(); }
InputFile::InputFile(InputFile &&other) noexcept
: fd_(other.fd_), path_(std::move(other.path_)) {
@ -73,7 +71,7 @@ InputFile::InputFile(InputFile &&other) noexcept
}
InputFile &InputFile::operator=(InputFile &&other) noexcept {
if (IsOpen()) Close();
Close();
fd_ = other.fd_;
path_ = std::move(other.path_);
@ -83,11 +81,8 @@ InputFile &InputFile::operator=(InputFile &&other) noexcept {
return *this;
}
void InputFile::Open(const std::filesystem::path &path) {
CHECK(!IsOpen())
<< "While trying to open " << path
<< " for writing the database used a handle that already has " << path_
<< " opened in it!";
bool InputFile::Open(const std::filesystem::path &path) {
if (IsOpen()) return false;
path_ = path;
@ -97,15 +92,13 @@ void InputFile::Open(const std::filesystem::path &path) {
// The call was interrupted, try again...
continue;
} else {
// All other possible errors are fatal errors and are handled in the CHECK
// below.
// All other possible errors are fatal errors and are handled with the
// return value.
break;
}
}
CHECK(fd_ != -1) << "While trying to open " << path_
<< " for reading an error occurred: " << strerror(errno)
<< " (" << errno << ").";
return fd_ != -1;
}
bool InputFile::IsOpen() const { return fd_ != -1; }
@ -154,18 +147,21 @@ bool InputFile::Peek(uint8_t *data, size_t size) {
return true;
}
size_t InputFile::GetSize() {
size_t current = GetPosition();
size_t size = SetPosition(Position::RELATIVE_TO_END, 0);
SetPosition(Position::SET, current);
std::optional<size_t> InputFile::GetSize() {
auto current = GetPosition();
if (!current) return std::nullopt;
auto size = SetPosition(Position::RELATIVE_TO_END, 0);
if (!size) return std::nullopt;
if (!SetPosition(Position::SET, *current)) return std::nullopt;
return size;
}
size_t InputFile::GetPosition() {
std::optional<size_t> InputFile::GetPosition() {
return SetPosition(Position::RELATIVE_TO_CURRENT, 0);
}
size_t InputFile::SetPosition(Position position, ssize_t offset) {
std::optional<size_t> InputFile::SetPosition(Position position,
ssize_t offset) {
int whence;
switch (position) {
case Position::SET:
@ -183,14 +179,14 @@ size_t InputFile::SetPosition(Position position, ssize_t offset) {
if (pos == -1 && errno == EINTR) {
continue;
}
CHECK(pos >= 0) << "While trying to set the position in " << path_
<< " an error occurred: " << strerror(errno) << " ("
<< errno << ").";
if (pos < 0) return std::nullopt;
return pos;
}
}
void InputFile::Close() noexcept {
if (!IsOpen()) return;
int ret = 0;
while (true) {
ret = close(fd_);
@ -204,9 +200,11 @@ void InputFile::Close() noexcept {
}
}
CHECK(ret == 0) << "While trying to close " << path_
<< " an error occurred: " << strerror(errno) << " (" << errno
<< ").";
if (ret != 0) {
LOG(ERROR) << "While trying to close " << path_
<< " an error occurred: " << strerror(errno) << " (" << errno
<< ").";
}
fd_ = -1;
}

26
src/utils/file.hpp
View File

@ -7,6 +7,7 @@
#pragma once
#include <filesystem>
#include <optional>
#include <string>
#include <string_view>
#include <vector>
@ -63,8 +64,8 @@ class InputFile {
InputFile &operator=(InputFile &&other) noexcept;
/// This method opens the file used for reading. If the file can't be opened
/// or doesn't exist it crashes the program.
void Open(const std::filesystem::path &path);
/// or doesn't exist it returns `false`.
bool Open(const std::filesystem::path &path);
/// Returns a boolean indicating whether a file is opened.
bool IsOpen() const;
@ -83,22 +84,21 @@ class InputFile {
/// doesn't change the current position in the file.
bool Peek(uint8_t *data, size_t size);
/// This method gets the size of the file. On failure and misuse it crashes
/// the program.
size_t GetSize();
/// This method gets the size of the file. On failure it returns
/// `std::nullopt`.
std::optional<size_t> GetSize();
/// This method gets the current absolute position in the file. On failure and
/// misuse it crashes the program.
size_t GetPosition();
/// This method gets the current absolute position in the file. On failure it
/// returns `std::nullopt`.
std::optional<size_t> GetPosition();
/// This method sets the current position in the file and returns the absolute
/// set position in the file. The position is set to `offset` with the
/// starting point taken from `position`. On failure and misuse it crashes the
/// program.
size_t SetPosition(Position position, ssize_t offset);
/// starting point taken from `position`. On failure it returns
/// `std::nullopt`.
std::optional<size_t> SetPosition(Position position, ssize_t offset);
/// Closes the currently opened file. On failure and misuse it crashes the
/// program.
/// Closes the currently opened file. On failure it crashes the program.
void Close() noexcept;
private:

27
tests/unit/storage_v2_decoder_encoder.cpp
View File

@ -59,7 +59,9 @@ TEST_F(DecoderEncoderTest, ReadMarker) {
}
ASSERT_FALSE(decoder.ReadMarker());
ASSERT_FALSE(decoder.ReadMarker());
ASSERT_EQ(decoder.GetPosition(), decoder.GetSize());
auto pos = decoder.GetPosition();
ASSERT_TRUE(pos);
ASSERT_EQ(pos, decoder.GetSize());
}
}
@ -91,7 +93,9 @@ TEST_F(DecoderEncoderTest, ReadMarker) {
} \
ASSERT_FALSE(decoder.Read##name()); \
ASSERT_FALSE(decoder.Read##name()); \
ASSERT_EQ(decoder.GetPosition(), decoder.GetSize()); \
auto pos = decoder.GetPosition(); \
ASSERT_TRUE(pos); \
ASSERT_EQ(pos, decoder.GetSize()); \
} \
}
@ -148,7 +152,9 @@ GENERATE_READ_TEST(
} \
ASSERT_FALSE(decoder.Skip##name()); \
ASSERT_FALSE(decoder.Skip##name()); \
ASSERT_EQ(decoder.GetPosition(), decoder.GetSize()); \
auto pos = decoder.GetPosition(); \
ASSERT_TRUE(pos); \
ASSERT_EQ(pos, decoder.GetSize()); \
} \
}
@ -179,7 +185,7 @@ GENERATE_SKIP_TEST(
{ \
utils::InputFile ifile; \
utils::OutputFile ofile; \
ifile.Open(storage_file); \
ASSERT_TRUE(ifile.Open(storage_file)); \
ofile.Open(alternate_file, utils::OutputFile::Mode::OVERWRITE_EXISTING); \
auto size = ifile.GetSize(); \
for (size_t i = 0; i <= size; ++i) { \
@ -245,7 +251,7 @@ GENERATE_PARTIAL_READ_TEST(
{ \
utils::InputFile ifile; \
utils::OutputFile ofile; \
ifile.Open(storage_file); \
ASSERT_TRUE(ifile.Open(storage_file)); \
ofile.Open(alternate_file, utils::OutputFile::Mode::OVERWRITE_EXISTING); \
auto size = ifile.GetSize(); \
for (size_t i = 0; i <= size; ++i) { \
@ -388,11 +394,14 @@ TEST_F(DecoderEncoderTest, DecoderPosition) {
ASSERT_TRUE(version);
ASSERT_EQ(*version, kTestVersion);
for (int i = 0; i < 10; ++i) {
decoder.SetPosition(kTestMagic.size() + sizeof(kTestVersion));
ASSERT_TRUE(
decoder.SetPosition(kTestMagic.size() + sizeof(kTestVersion)));
auto decoded = decoder.ReadBool();
ASSERT_TRUE(decoded);
ASSERT_TRUE(*decoded);
ASSERT_EQ(decoder.GetPosition(), decoder.GetSize());
auto pos = decoder.GetPosition();
ASSERT_TRUE(pos);
ASSERT_EQ(pos, decoder.GetSize());
}
}
}
@ -416,6 +425,8 @@ TEST_F(DecoderEncoderTest, EncoderPosition) {
auto decoded = decoder.ReadBool();
ASSERT_TRUE(decoded);
ASSERT_TRUE(*decoded);
ASSERT_EQ(decoder.GetPosition(), decoder.GetSize());
auto pos = decoder.GetPosition();
ASSERT_TRUE(pos);
ASSERT_EQ(pos, decoder.GetSize());
}
}