Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

is_hdd update for windows and linux #4278

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,8 @@ else()
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,--stack,10485760")
if(NOT BUILD_64)
add_definitions(-DWINVER=0x0501 -D_WIN32_WINNT=0x0501)
else()
add_definitions(-DWINVER=0x0601 -D_WIN32_WINNT=0x0601)
endif()
endif()
set(C_WARNINGS "-Waggregate-return -Wnested-externs -Wold-style-definition -Wstrict-prototypes")
Expand Down Expand Up @@ -814,7 +816,7 @@ endif()
include_directories(SYSTEM ${Boost_INCLUDE_DIRS})
if(MINGW)
set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -Wa,-mbig-obj")
set(EXTRA_LIBRARIES mswsock;ws2_32;iphlpapi)
set(EXTRA_LIBRARIES mswsock;ws2_32;iphlpapi;bcrypt)
set(ICU_LIBRARIES ${Boost_LOCALE_LIBRARY} icuio icuin icuuc icudt icutu iconv)
elseif(APPLE OR OPENBSD OR ANDROID)
set(EXTRA_LIBRARIES "")
Expand Down
260 changes: 214 additions & 46 deletions src/common/util.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,13 @@
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/resource.h>
#include <ustat.h>
#include <sys/sysmacros.h>
#include <unistd.h>
#include <dirent.h>
#include <string.h>
#include <ctype.h>
#include <string>
#include <sstream>
#endif

#include "unbound.h"
Expand Down Expand Up @@ -74,6 +75,11 @@ using namespace epee;
#include <boost/asio.hpp>
#include <openssl/sha.h>

#ifdef WIN32
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we had problems with WIN32 before, and _WIN32 was the "best" one to use.

#include <regex>
#include <winioctl.h>
#endif

namespace tools
{
std::function<void(int)> signal_handler::m_handler;
Expand Down Expand Up @@ -733,63 +739,225 @@ std::string get_nix_version_display_string()
#endif
}

bool is_hdd(const char *path)
#if defined(__GLIBC__)
bool is_hdd_sysfs(const char *file_path, bool &result)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat quirky, but boost::optional<bool> might be better suited for this. There is also boost::tribool, but usage of that can be tricky since the library author unfortunately decided to overload operator&& and operator|| which ruins default semantics.

{
#ifdef __GLIBC__
std::string device = "";
struct stat st, dst;
if (stat(path, &st) < 0)
return 0;

DIR *dir = opendir("/dev/block");
if (!dir)
return 0;
struct dirent *de;
while ((de = readdir(dir)))
{
if (strcmp(de->d_name, ".") && strcmp(de->d_name, ".."))
struct stat st;
std::string prefix;
if(stat(file_path, &st) == 0)
{
std::ostringstream s;
s << "/sys/dev/block/" << major(st.st_dev) << ":" << minor(st.st_dev);
prefix = s.str();
}
else
{
return false;
}
std::string attr_path = prefix + "/queue/rotational";
FILE *f = fopen(attr_path.c_str(), "r");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not std::ifstream here? Streams were already in use a few lines above anyway. The primary advantage is that you don't have to worry about the unsigned short / %hu stuff in scanf.

if(f == nullptr)
{
attr_path = prefix + "/../queue/rotational";
f = fopen(attr_path.c_str(), "r");
if(f == nullptr)
{
return false;
}
}
uint16_t val = 0xdead;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably best to make it a unsigned short if using %hu below.

int r = fscanf(f, "%hu", &val);
fclose(f);
if(r == 1)
{
result = val == 1;
return true;
}
return false;
}
#elif defined(_WIN32) and (_WIN32_WINNT >= 0x0601)
//file path to logical volume
bool fp2lv(const char *fp, std::string &result)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be boost::optional<std::string>.

{
HANDLE h = INVALID_HANDLE_VALUE;
h = CreateFile(
fp,
0,
FILE_SHARE_READ | FILE_SHARE_WRITE,
nullptr,
OPEN_EXISTING,
FILE_ATTRIBUTE_NORMAL | FILE_FLAG_BACKUP_SEMANTICS,
nullptr
);
if(h != INVALID_HANDLE_VALUE)
{
char p[MAX_PATH + 1];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does GetFinalPathNameByHandleA always add a null-terminator? Even if the function reaches the MAX_PATH limit? I suggest doing char p[MAX_PATH + 1] = {0} to zero-initialize everything.

DWORD r_size = GetFinalPathNameByHandleA(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this throws (it shouldn't really), then CloseHandle is never called. There is a struct close_handle in this cpp file already that can be used as a helper:

std::unique_ptr<void, close_handle> h{
    CreateFile(...)
};

h,
p,
MAX_PATH,
VOLUME_NAME_NT | FILE_NAME_NORMALIZED
);
CloseHandle(h);
if(r_size > 0 and r_size <= MAX_PATH)
{
std::string dev_path = std::string("/dev/block/") + de->d_name;
char resolved[PATH_MAX];
if (realpath(dev_path.c_str(), resolved) && !strncmp(resolved, "/dev/", 5))
std::regex r("^\\\\Device\\\\([^\\\\]+).*$");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let me walk through this logic, because this should be easy for a boost::spirit usage. It will require more compile-time, but will be much faster since the expression expands into a specific parser at compile-time, instead of a generic NFA runtime implementation that std::regex is likely using.

It looks like you want to grab all characters after \\Device\\ until EOF or next \\. Is that correct? If so the boost::spirit equivalent is:

#include <boost/range/iterator_range.hpp>
#include <boost/spirit/include/qi_char_.hpp>
#include <boost/spirit/include/qi_difference.hpp>
#include <boost/spirit/include/qi_kleene.hpp>
#include <boost/spirit/include/qi_lit.hpp>
#include <boost/spirit/include/qi_sequence.hpp>

namespace qi = boost::spirit::qi

boost::iterator_range<const char> match{};
if (qi::parse(p, p + strlen(p), (qi::lit("\\Device\\") >> *(qi::char_ - "\\")), match))
{
    ...
}

This simply says match \\Device\\ then match zero or more char_acters unless the next two characters are \\. Those char_acters are then stored in the boost::iterator_range<const char*>. The entire expression doesn't allocate any memory, even when a match is found.

... thoughts ?

std::smatch m;
std::string i = p;
if(std::regex_match(i, m, r))
{
if (stat(resolved, &dst) == 0)
if(m.size() == 2)
{
if (dst.st_rdev == st.st_dev)
{
// take out trailing digits (eg, sda1 -> sda)
char *ptr = resolved;
while (*ptr)
++ptr;
while (ptr > resolved && isdigit(*--ptr))
*ptr = 0;
device = resolved + 5;
break;
}
result = m[1].str();
return true;
}
}
}
}
closedir(dir);
return false;
}

if (device.empty())
return 0;
//logical volume to physical volumes
bool lv2pv(const char *lv, std::vector<std::string> &pvs)
{
HANDLE h = INVALID_HANDLE_VALUE;
std::string lv_path = "\\\\?\\";
lv_path += lv;
h = CreateFile(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another std::unique_ptr<void, close_handle> opportunity.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another std::unique_ptr<void, close_handle> case.

lv_path.c_str(),
0,
FILE_SHARE_READ | FILE_SHARE_WRITE,
nullptr,
OPEN_EXISTING,
FILE_ATTRIBUTE_NORMAL,
nullptr
);
if(h != INVALID_HANDLE_VALUE)
{
VOLUME_DISK_EXTENTS r;
DWORD r_size = 0;
BOOL success = DeviceIoControl(
h,
IOCTL_VOLUME_GET_VOLUME_DISK_EXTENTS,
nullptr,
0,
&r,
sizeof(r),
&r_size,
nullptr
);
CloseHandle(h);
if(success and r_size == sizeof(r))
{
for(uint32_t i = 0; i < r.NumberOfDiskExtents; ++i)
{
std::ostringstream ss;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pop this before the loop, then do a .str(std::string{}) every iteration. Some easy performance gains (constructing an std::ostream requires a global synchronization on the current std::locale object). There might be a better way to reset the state of this type if the manual was inspecting further ...

ss << "PhysicalDrive" << r.Extents[i].DiskNumber;
pvs.push_back(ss.str());
}
return true;
}
}
return false;
}

std::string sys_path = "/sys/block/" + device + "/queue/rotational";
FILE *f = fopen(sys_path.c_str(), "r");
if (!f)
bool win_device_has_seek_penalny(const char *pv, bool &result)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

penalty

{
HANDLE h = INVALID_HANDLE_VALUE;
std::string pv_path = "\\\\?\\";
pv_path += pv;
h = CreateFile(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another std::unique_ptr<void, close_handle>.

pv_path.c_str(),
0,
FILE_SHARE_READ | FILE_SHARE_WRITE,
nullptr,
OPEN_EXISTING,
FILE_ATTRIBUTE_NORMAL,
nullptr
);
if(h != INVALID_HANDLE_VALUE)
{
STORAGE_PROPERTY_QUERY q = {
.PropertyId = StorageDeviceSeekPenaltyProperty,
.QueryType = PropertyStandardQuery
};
DEVICE_SEEK_PENALTY_DESCRIPTOR r;
DWORD r_size = 0;
BOOL success = DeviceIoControl(
h,
IOCTL_STORAGE_QUERY_PROPERTY,
&q,
sizeof(q),
&r,
sizeof(r),
&r_size,
nullptr
);
if(success and r_size == sizeof(r))
{
result = r.IncursSeekPenalty;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That path seems to leak h.

return true;
}
CloseHandle(h);
}
return false;
}

bool is_hdd_win_ioctl(const char *path, bool &result)
{
std::string lv;
bool lv_success = fp2lv(path, lv);
if(not lv_success)
{
return false;
char s[8];
char *ptr = fgets(s, sizeof(s), f);
fclose(f);
if (!ptr)
return 0;
s[sizeof(s) - 1] = 0;
int n = atoi(s); // returns 0 on parse error
return n == 1;
#else
return 0;
}
std::vector<std::string> pvs;
bool pv_success = lv2pv(lv.c_str(), pvs);
if(not pv_success)
{
return false;
}
bool a_result = 0;
bool a_success = 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

false

for(auto &v: pvs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto const& v

{
bool q_result;
bool q_success = win_device_has_seek_penalny(v.c_str(), q_result);
a_success |= q_success;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is damn near obfuscation. Some early returns are possible. If boost::optional<bool> suggestion is used:

for (auto const& v : pvs)
{
    const auto success = win_device_has_seek_penalty(v.c_str());
    if (!success)
        return boost::none;
    if (success.value())
        return true;
}
return false;

Should this be inverted though? If any of the backing physical volumes isn't a local HDD, then LMDB may have a bad time.

if(q_success)
{
a_result |= q_result;
}
}
if(a_success)
{
result = a_result;
}
return a_success;
}
#endif
bool is_hdd(const char *path, bool &result)
{
#if defined(_WIN32) and (_WIN32_WINNT >= 0x0601)
return is_hdd_win_ioctl(path, result);
#elif defined(__GLIBC__)
return is_hdd_sysfs(path, result);
#else
return 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

false

#endif
}

bool is_hdd(const char *path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For right now, I recommend dropping this overload in favor of boost::optional<bool> (see above). Then update the callers so that they react to the three states : (1) a harddrive, (2) not a harddrive, and (3) check failure. The third state will likely just have to be a warning message. What to do if the path is not a harddrive? Ask for user confirmation before proceeding? Not really sure - I think a more aggressive error message at least.

Also, hopefully in the not-too-distant future, this can be expect<T>, the actual error code from Windows/Posix will be returned so the warning message can be improved. Need to get on that expect<T> patch ...

{
bool result;
if(is_hdd(path, result))
{
return result;
}
else
{
return 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

false

}
}

namespace
Expand Down
2 changes: 2 additions & 0 deletions src/common/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,8 @@ namespace tools
bool sha256sum(const uint8_t *data, size_t len, crypto::hash &hash);
bool sha256sum(const std::string &filename, crypto::hash &hash);


bool is_hdd(const char *path, bool &result);
bool is_hdd(const char *path);

boost::optional<std::pair<uint32_t, uint32_t>> parse_subaddress_lookahead(const std::string& str);
Expand Down
1 change: 1 addition & 0 deletions src/crypto/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ target_link_libraries(cncrypto
epee
${Boost_SYSTEM_LIBRARY}
PRIVATE
sodium
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant for another patch?

${EXTRA_LIBRARIES})

if (ARM)
Expand Down
1 change: 1 addition & 0 deletions tests/unit_tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ set(unit_tests_sources
ringct.cpp
output_selection.cpp
vercmp.cpp
test_a.cpp
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to something sane

ringdb.cpp)

set(unit_tests_headers
Expand Down
2 changes: 1 addition & 1 deletion tests/unit_tests/ringdb.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ static crypto::chacha_key generate_chacha_key()
{
crypto::chacha_key chacha_key;
uint64_t password = crypto::rand<uint64_t>();
crypto::generate_chacha_key(std::string((const char*)&password, sizeof(password)), chacha_key);
crypto::generate_chacha_key(std::string((const char*)&password, sizeof(password)), chacha_key, 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this changed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated, fixed by #4272

return chacha_key;
}

Expand Down
30 changes: 30 additions & 0 deletions tests/unit_tests/test_a.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
#include "common/util.h"
#include <string>
#include <gtest/gtest.h>

#if defined(__GLIBC__)
TEST(test_a, is_hdd_linux_os_root)
{
bool result;
std::string path = "/";
EXPECT_TRUE(tools::is_hdd(path.c_str(), result));
}
#elif defined(_WIN32) and (_WIN32_WINNT >= 0x0601)
TEST(test_a, is_hdd_win_os_c)
{
bool result;
std::string path = "\\\\?\\C:\\Windows\\System32\\cmd.exe";
EXPECT_TRUE(tools::is_hdd(path.c_str(), result));
path = "\\\\?\\C:\\";
EXPECT_TRUE(tools::is_hdd(path.c_str(), result));
path = "C:\\";
EXPECT_TRUE(tools::is_hdd(path.c_str(), result));
}
#else
TEST(test_a, is_hdd_unknown_os)
{
bool result;
std::string path = "";
EXPECT_FALSE(tools::is_hdd(path.c_str(), result));
}
#endif