From a267658a0bf04034cf0ed5de8a801ac13f1ebc0b Mon Sep 17 00:00:00 2001 From: jsteemann Date: Mon, 19 Jul 2021 22:53:05 +0200 Subject: [PATCH 1/3] Improve startup file copying of JavaScript files * Make `--javascript.copy-installation` also copy the `node_modules` sub directory. This is required so we have a full copy of the JavaScript dependencies and not one that excludes some infrequently changed modules. In addition, file copying now intentionally excludes .map files as they are not needed. --- CHANGELOG | 10 + arangod/V8Server/V8DealerFeature.cpp | 51 +++-- lib/Basics/FileUtils.cpp | 192 +++++++++++------- lib/Basics/files.cpp | 177 +++++++++------- lib/Basics/files.h | 8 + lib/Basics/operating-system.h | 12 +- .../test-copy-installation.js | 22 +- 7 files changed, 285 insertions(+), 187 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index c0805e6ef7d7..c279b90a4eae 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,13 @@ +v3.8.1 (XXXX-XX-XX) +------------------- + +* Make `--javascript.copy-installation` also copy the `node_modules` sub + directory. This is required so we have a full copy of the JavaScript + dependencies and not one that excludes some infrequently changed modules. + In addition, file copying now intentionally excludes .map files as they + are not needed. + + v3.8.0 (XXXX-XX-XX) ------------------- diff --git a/arangod/V8Server/V8DealerFeature.cpp b/arangod/V8Server/V8DealerFeature.cpp index 6abeda068654..dd38e409273b 100644 --- a/arangod/V8Server/V8DealerFeature.cpp +++ b/arangod/V8Server/V8DealerFeature.cpp @@ -557,9 +557,9 @@ void V8DealerFeature::copyInstallationFiles() { _nodeModulesDirectory = _startupDirectory; - const std::string checksumFile = + std::string const checksumFile = FileUtils::buildFilename(_startupDirectory, StaticStrings::checksumFileJs); - const std::string copyChecksumFile = + std::string const copyChecksumFile = FileUtils::buildFilename(copyJSPath, StaticStrings::checksumFileJs); bool overwriteCopy = false; @@ -569,7 +569,7 @@ void V8DealerFeature::copyInstallationFiles() { } else { try { overwriteCopy = - (FileUtils::slurp(copyChecksumFile) != FileUtils::slurp(checksumFile)); + (StringUtils::trim(FileUtils::slurp(copyChecksumFile)) != StringUtils::trim(FileUtils::slurp(checksumFile))); } catch (basics::Exception const& e) { LOG_TOPIC("efa47", ERR, Logger::V8) << "Error reading '" << StaticStrings::checksumFileJs << "' from disk: " << e.what(); @@ -578,7 +578,7 @@ void V8DealerFeature::copyInstallationFiles() { } if (overwriteCopy) { - // basics security checks before removing an existing directory: + // basic security checks before removing an existing directory: // check if for some reason we will be trying to remove the entire database // directory... if (FileUtils::exists(FileUtils::buildFilename(copyJSPath, "ENGINE"))) { @@ -587,7 +587,7 @@ void V8DealerFeature::copyInstallationFiles() { FATAL_ERROR_EXIT(); } - LOG_TOPIC("dd1c0", DEBUG, Logger::V8) + LOG_TOPIC("dd1c0", INFO, Logger::V8) << "Copying JS installation files from '" << _startupDirectory << "' to '" << copyJSPath << "'"; auto res = TRI_ERROR_NO_ERROR; @@ -606,40 +606,46 @@ void V8DealerFeature::copyInstallationFiles() { FATAL_ERROR_EXIT(); } - // intentionally do not copy js/node/node_modules... + // intentionally do not copy js/node/node_modules/estlint! // we avoid copying this directory because it contains 5000+ files at the - // moment, and copying them one by one is darn slow at least on Windows... + // moment, and copying them one by one is slow. In addition, eslint is not + // needed in release builds std::string const versionAppendix = std::regex_replace(rest::Version::getServerVersion(), std::regex("-.*$"), ""); - std::string const nodeModulesPath = - FileUtils::buildFilename("js", "node", "node_modules"); - std::string const nodeModulesPathVersioned = - basics::FileUtils::buildFilename("js", versionAppendix, "node", - "node_modules"); + std::string const eslintPath = + FileUtils::buildFilename("js", "node", "node_modules", "eslint"); - std::regex const binRegex("[/\\\\]\\.bin[/\\\\]", std::regex::ECMAScript); + // .bin directories could be harmful, and .map files are large and unnecessary + std::string const binDirectory = std::string(TRI_DIR_SEPARATOR_STR) + ".bin" + TRI_DIR_SEPARATOR_STR; - auto filter = [&nodeModulesPath, &nodeModulesPathVersioned, &binRegex](std::string const& filename) -> bool { - if (std::regex_search(filename, binRegex)) { + size_t copied = 0; + + auto filter = [&eslintPath, &binDirectory, &copied](std::string const& filename) -> bool { + if (filename.size() >= 4 && filename.compare(filename.size() - 4, 4, ".map") == 0) { + // filename ends with ".map". filter it out! + return true; + } + if (filename.find(binDirectory) != std::string::npos) { // don't copy files in .bin return true; } + std::string normalized = filename; FileUtils::normalizePath(normalized); - if ((!nodeModulesPath.empty() && - normalized.size() >= nodeModulesPath.size() && - normalized.substr(normalized.size() - nodeModulesPath.size(), nodeModulesPath.size()) == nodeModulesPath) || - (!nodeModulesPathVersioned.empty() && - normalized.size() >= nodeModulesPathVersioned.size() && - normalized.substr(normalized.size() - nodeModulesPathVersioned.size(), nodeModulesPathVersioned.size()) == nodeModulesPathVersioned)) { + if ((normalized.size() >= eslintPath.size() && + normalized.compare(normalized.size() - eslintPath.size(), eslintPath.size(), eslintPath) == 0)) { // filter it out! return true; } + // let the file/directory pass through + ++copied; return false; }; + double start = TRI_microtime(); + std::string error; if (!FileUtils::copyRecursive(_startupDirectory, copyJSPath, filter, error)) { LOG_TOPIC("45261", FATAL, Logger::V8) << "Error copying JS installation files to '" @@ -662,6 +668,9 @@ void V8DealerFeature::copyInstallationFiles() { << copyJSPath << "': " << error; } } + + LOG_TOPIC("38e1e", INFO, Logger::V8) + << "copying " << copied << " JS installation file(s) took " << Logger::FIXED(TRI_microtime() - start, 6) << "s"; } _startupDirectory = copyJSPath; } diff --git a/lib/Basics/FileUtils.cpp b/lib/Basics/FileUtils.cpp index 389f9d1496d2..17d45cbef2c5 100755 --- a/lib/Basics/FileUtils.cpp +++ b/lib/Basics/FileUtils.cpp @@ -64,8 +64,50 @@ namespace { std::function const passAllFilter = [](std::string const&) { return false; }; + +enum class StatResultType { + Error, // in case it cannot be determined + Directory, + SymLink, + File, + Other // potentially file +}; + +StatResultType statResultType(TRI_stat_t const& stbuf) { +#ifdef _WIN32 + if ((stbuf.st_mode & S_IFMT) == S_IFDIR) { + return StatResultType::Directory; + } +#else + if (S_ISDIR(stbuf.st_mode)) { + return StatResultType::Directory; + } +#endif + +#ifndef TRI_HAVE_WIN32_SYMBOLIC_LINK + if (S_ISLNK(stbuf.st_mode)) { + return StatResultType::SymLink; + } +#endif + + if ((stbuf.st_mode & S_IFMT) == S_IFREG) { + return StatResultType::File; + } + + return StatResultType::Other; } +StatResultType statResultType(std::string const& path) { + TRI_stat_t stbuf; + int res = TRI_STAT(path.c_str(), &stbuf); + if (res != 0) { + return StatResultType::Error; + } + return statResultType(stbuf); +} + +} // namespace + namespace arangodb { namespace basics { namespace FileUtils { @@ -401,12 +443,15 @@ bool copyRecursive(std::string const& source, std::string const& target, bool copyDirectoryRecursive(std::string const& source, std::string const& target, std::function const& filter, std::string& error) { - char* fn = nullptr; bool rc_bool = true; + + // these strings will be recycled over and over + std::string dst = target + TRI_DIR_SEPARATOR_STR; + size_t const dstPrefixLength = dst.size(); + std::string src = source + TRI_DIR_SEPARATOR_STR; + size_t const srcPrefixLength = src.size(); + - auto isSubDirectory = [](std::string const& name) -> bool { - return isDirectory(name); - }; #ifdef TRI_HAVE_WIN32_LIST_FILES struct _wfinddata_t oneItem; intptr_t handle; @@ -427,7 +472,7 @@ bool copyDirectoryRecursive(std::string const& source, std::string const& target rcs.clear(); icu::UnicodeString d((wchar_t*)oneItem.name, static_cast(wcslen(oneItem.name))); d.toUTF8String(rcs); - fn = (char*)rcs.c_str(); + char const* fn = (char*)rcs.c_str(); #else DIR* filedir = opendir(source.c_str()); @@ -445,7 +490,7 @@ bool copyDirectoryRecursive(std::string const& source, std::string const& target // to be thread-safe in reality, and newer versions of POSIX may require its // thread-safety formally, and in addition obsolete readdir_r() altogether while ((oneItem = (readdir(filedir))) != nullptr && rc_bool) { - fn = oneItem->d_name; + char const* fn = oneItem->d_name; #endif // Now iterate over the items. @@ -453,49 +498,71 @@ bool copyDirectoryRecursive(std::string const& source, std::string const& target if (!strcmp(fn, ".") || !strcmp(fn, "..")) { continue; } - std::string dst = target + TRI_DIR_SEPARATOR_STR + fn; - std::string src = source + TRI_DIR_SEPARATOR_STR + fn; - switch (filter(src)) { - case TRI_COPY_IGNORE: - break; + // add current filename to prefix + src.resize(srcPrefixLength); + TRI_ASSERT(src.back() == TRI_DIR_SEPARATOR_CHAR); + src.append(fn); + + auto filterResult = filter(src); + + if (filterResult != TRI_COPY_IGNORE) { + // prepare dst filename + dst.resize(dstPrefixLength); + TRI_ASSERT(dst.back() == TRI_DIR_SEPARATOR_CHAR); + dst.append(fn); + + // figure out the type of the directory entry. + StatResultType type = StatResultType::Error; + TRI_stat_t stbuf; + int res = TRI_STAT(src.c_str(), &stbuf); + if (res == 0) { + type = ::statResultType(stbuf); + } - case TRI_COPY_COPY: - // Handle subdirectories: - if (isSubDirectory(src)) { - long systemError; - auto rc = TRI_CreateDirectory(dst.c_str(), systemError, error); - if (rc != TRI_ERROR_NO_ERROR && rc != TRI_ERROR_FILE_EXISTS) { - rc_bool = false; - break; - } - if (!copyDirectoryRecursive(src, dst, filter, error)) { - rc_bool = false; - break; - } - if (!TRI_CopyAttributes(src, dst, error)) { - rc_bool = false; - break; - } -#ifndef _WIN32 - } else if (isSymbolicLink(src)) { - if (!TRI_CopySymlink(src, dst, error)) { - rc_bool = false; - } + switch (filterResult) { + case TRI_COPY_IGNORE: + TRI_ASSERT(false); + break; + + case TRI_COPY_COPY: + // Handle subdirectories: + if (type == StatResultType::Directory) { + long systemError; + auto rc = TRI_CreateDirectory(dst.c_str(), systemError, error); + if (rc != TRI_ERROR_NO_ERROR && rc != TRI_ERROR_FILE_EXISTS) { + rc_bool = false; + break; + } + if (!copyDirectoryRecursive(src, dst, filter, error)) { + rc_bool = false; + break; + } + if (!TRI_CopyAttributes(src, dst, error)) { + rc_bool = false; + break; + } + } else if (type == StatResultType::SymLink) { + if (!TRI_CopySymlink(src, dst, error)) { + rc_bool = false; + } + } else { +#ifdef _WIN32 + rc_bool = TRI_CopyFile(src, dst, error); +#else + // optimized version that reuses the already retrieved stat data + rc_bool = TRI_CopyFile(src, dst, error, &stbuf); #endif - } else { - if (!TRI_CopyFile(src, dst, error)) { - rc_bool = false; } - } - break; + break; - case TRI_COPY_LINK: - if (!TRI_CreateHardlink(src, dst, error)) { - rc_bool = false; - } // if - break; - } // switch + case TRI_COPY_LINK: + if (!TRI_CreateHardlink(src, dst, error)) { + rc_bool = false; + } // if + break; + } // switch + } #ifdef TRI_HAVE_WIN32_LIST_FILES } while (_wfindnext(handle, &oneItem) != -1 && rc_bool); @@ -578,48 +645,19 @@ std::vector listFiles(std::string const& directory) { } bool isDirectory(std::string const& path) { - TRI_stat_t stbuf; - int res = TRI_STAT(path.c_str(), &stbuf); - -#ifdef _WIN32 - return (res == 0) && ((stbuf.st_mode & S_IFMT) == S_IFDIR); -#else - return (res == 0) && S_ISDIR(stbuf.st_mode); -#endif + return ::statResultType(path) == ::StatResultType::Directory; } bool isSymbolicLink(std::string const& path) { -#ifdef TRI_HAVE_WIN32_SYMBOLIC_LINK - - // ..................................................................... - // TODO: On the NTFS file system, there are the following file links: - // hard links - - // junctions - - // symbolic links - - // ..................................................................... - return false; - -#else - - struct stat stbuf; - int res = TRI_STAT(path.c_str(), &stbuf); - - return (res == 0) && S_ISLNK(stbuf.st_mode); - -#endif + return ::statResultType(path) == ::StatResultType::SymLink; } bool isRegularFile(std::string const& path) { - TRI_stat_t stbuf; - int res = TRI_STAT(path.c_str(), &stbuf); - return (res == 0) && ((stbuf.st_mode & S_IFMT) == S_IFREG); + return ::statResultType(path) == ::StatResultType::File; } bool exists(std::string const& path) { - TRI_stat_t stbuf; - int res = TRI_STAT(path.c_str(), &stbuf); - - return res == 0; + return ::statResultType(path) != ::StatResultType::Error; } off_t size(std::string const& path) { diff --git a/lib/Basics/files.cpp b/lib/Basics/files.cpp index 0a4ee833ad75..5a9ac2deb700 100644 --- a/lib/Basics/files.cpp +++ b/lib/Basics/files.cpp @@ -1796,7 +1796,10 @@ std::string TRI_GetInstallRoot(std::string const& binaryPath, char const* instal static bool CopyFileContents(int srcFD, int dstFD, TRI_read_t fileSize, std::string& error) { bool rc = true; -#if TRI_LINUX_SPLICE +#ifdef __linux__ + // Linux-specific file-copying code based on splice() + // The splice() system call first appeared in Linux 2.6.17; library support was added to glibc in version 2.5. + // libmusl also has bindings for it. so we simply assume it is there on Linux. int splicePipe[2]; ssize_t pipeSize = 0; long chunkSendRemain = fileSize; @@ -1806,25 +1809,30 @@ static bool CopyFileContents(int srcFD, int dstFD, TRI_read_t fileSize, std::str error = std::string("splice failed to create pipes: ") + strerror(errno); return false; } - while (chunkSendRemain > 0) { - if (pipeSize == 0) { - pipeSize = splice(srcFD, &totalSentAlready, splicePipe[1], nullptr, - chunkSendRemain, SPLICE_F_MOVE); - if (pipeSize == -1) { + try { + while (chunkSendRemain > 0) { + if (pipeSize == 0) { + pipeSize = splice(srcFD, &totalSentAlready, splicePipe[1], nullptr, + chunkSendRemain, SPLICE_F_MOVE); + if (pipeSize == -1) { + error = std::string("splice read failed: ") + strerror(errno); + rc = false; + break; + } + } + ssize_t sent = splice(splicePipe[0], nullptr, dstFD, nullptr, pipeSize, + SPLICE_F_MORE | SPLICE_F_MOVE | SPLICE_F_NONBLOCK); + if (sent == -1) { error = std::string("splice read failed: ") + strerror(errno); rc = false; break; } + pipeSize -= sent; + chunkSendRemain -= sent; } - ssize_t sent = splice(splicePipe[0], nullptr, dstFD, nullptr, pipeSize, - SPLICE_F_MORE | SPLICE_F_MOVE | SPLICE_F_NONBLOCK); - if (sent == -1) { - error = std::string("splice read failed: ") + strerror(errno); - rc = false; - break; - } - pipeSize -= sent; - chunkSendRemain -= sent; + } catch (...) { + // make sure we always close the pipes + rc = false; } close(splicePipe[0]); close(splicePipe[1]); @@ -1838,42 +1846,47 @@ static bool CopyFileContents(int srcFD, int dstFD, TRI_read_t fileSize, std::str rc = false; } - TRI_read_t chunkRemain = fileSize; - while (rc && (chunkRemain > 0)) { - auto readChunk = static_cast((std::min)(C128, static_cast(chunkRemain))); - TRI_read_return_t nRead = TRI_READ(srcFD, buf, readChunk); - - if (nRead < 0) { - error = std::string("failed to read a chunk: ") + strerror(errno); - rc = false; - break; - } - - if (nRead == 0) { - // EOF. done - break; - } + try { + TRI_read_t chunkRemain = fileSize; + while (rc && (chunkRemain > 0)) { + auto readChunk = static_cast((std::min)(C128, static_cast(chunkRemain))); + TRI_read_return_t nRead = TRI_READ(srcFD, buf, readChunk); - size_t writeOffset = 0; - size_t writeRemaining = static_cast(nRead); - while (writeRemaining > 0) { - // write can write less data than requested. so we must go on writing - // until we have written out all data - ssize_t nWritten = TRI_WRITE(dstFD, buf + writeOffset, - static_cast(writeRemaining)); - - if (nWritten < 0) { - // error during write + if (nRead < 0) { error = std::string("failed to read a chunk: ") + strerror(errno); rc = false; break; } - writeOffset += static_cast(nWritten); - writeRemaining -= static_cast(nWritten); - } + if (nRead == 0) { + // EOF. done + break; + } + + size_t writeOffset = 0; + size_t writeRemaining = static_cast(nRead); + while (writeRemaining > 0) { + // write can write less data than requested. so we must go on writing + // until we have written out all data + ssize_t nWritten = TRI_WRITE(dstFD, buf + writeOffset, + static_cast(writeRemaining)); + + if (nWritten < 0) { + // error during write + error = std::string("failed to read a chunk: ") + strerror(errno); + rc = false; + break; + } - chunkRemain -= nRead; + writeOffset += static_cast(nWritten); + writeRemaining -= static_cast(nWritten); + } + + chunkRemain -= nRead; + } + } catch (...) { + // make sure we always close the buffer + rc = false; } TRI_Free(buf); @@ -1885,8 +1898,8 @@ static bool CopyFileContents(int srcFD, int dstFD, TRI_read_t fileSize, std::str /// @brief copies the contents of a file //////////////////////////////////////////////////////////////////////////////// -bool TRI_CopyFile(std::string const& src, std::string const& dst, std::string& error) { #ifdef _WIN32 +bool TRI_CopyFile(std::string const& src, std::string const& dst, std::string& error) { TRI_ERRORBUF; bool rc = CopyFileW(toWString(src).data(), toWString(dst).data(), true) != 0; @@ -1896,57 +1909,72 @@ bool TRI_CopyFile(std::string const& src, std::string const& dst, std::string& e } return rc; +} #else - size_t dsize; - int srcFD, dstFD; - struct stat statbuf; - - srcFD = open(src.c_str(), O_RDONLY); +bool TRI_CopyFile(std::string const& src, std::string const& dst, std::string& error, struct stat* statbuf /*= nullptr*/) { + int srcFD = open(src.c_str(), O_RDONLY); if (srcFD < 0) { error = "failed to open source file " + src + ": " + strerror(errno); return false; } - dstFD = open(dst.c_str(), O_EXCL | O_CREAT | O_NONBLOCK | O_WRONLY, S_IRUSR | S_IWUSR); + int dstFD = open(dst.c_str(), O_EXCL | O_CREAT | O_NONBLOCK | O_WRONLY, S_IRUSR | S_IWUSR); if (dstFD < 0) { close(srcFD); error = "failed to open destination file " + dst + ": " + strerror(errno); return false; } - TRI_FSTAT(srcFD, &statbuf); - dsize = statbuf.st_size; + bool rc = true; + try { + struct stat localStat; - bool rc = CopyFileContents(srcFD, dstFD, dsize, error); - timeval times[2]; - memset(times, 0, sizeof(times)); - times[0].tv_sec = TRI_STAT_ATIME_SEC(statbuf); - times[1].tv_sec = TRI_STAT_MTIME_SEC(statbuf); + if (statbuf == nullptr) { + TRI_FSTAT(srcFD, &localStat); + statbuf = &localStat; + } + TRI_ASSERT(statbuf != nullptr); - if (fchown(dstFD, -1 /*statbuf.st_uid*/, statbuf.st_gid) != 0) { - error = std::string("failed to chown ") + dst + ": " + strerror(errno); - // rc = false; no, this is not fatal... - } - if (fchmod(dstFD, statbuf.st_mode) != 0) { - error = std::string("failed to chmod ") + dst + ": " + strerror(errno); - rc = false; - } + size_t dsize = statbuf->st_size; + + if (dsize > 0) { + // only copy non-empty files + rc = CopyFileContents(srcFD, dstFD, dsize, error); + } + timeval times[2]; + memset(times, 0, sizeof(times)); + times[0].tv_sec = TRI_STAT_ATIME_SEC(*statbuf); + times[1].tv_sec = TRI_STAT_MTIME_SEC(*statbuf); + + if (fchown(dstFD, -1 /*statbuf.st_uid*/, statbuf->st_gid) != 0) { + error = std::string("failed to chown ") + dst + ": " + strerror(errno); + // rc = false; no, this is not fatal... + } + if (fchmod(dstFD, statbuf->st_mode) != 0) { + error = std::string("failed to chmod ") + dst + ": " + strerror(errno); + rc = false; + } #ifdef HAVE_FUTIMES - if (futimes(dstFD, times) != 0) { - error = std::string("failed to adjust age: ") + dst + ": " + strerror(errno); - rc = false; - } + if (futimes(dstFD, times) != 0) { + error = std::string("failed to adjust age: ") + dst + ": " + strerror(errno); + rc = false; + } #else - if (utimes(dst.c_str(), times) != 0) { - error = std::string("failed to adjust age: ") + dst + ": " + strerror(errno); + if (utimes(dst.c_str(), times) != 0) { + error = std::string("failed to adjust age: ") + dst + ": " + strerror(errno); + rc = false; + } +#endif + } catch (...) { + // make sure we always close file handles rc = false; } -#endif + close(srcFD); close(dstFD); return rc; -#endif } +#endif //////////////////////////////////////////////////////////////////////////////// /// @brief copies the filesystem attributes of a file @@ -2005,7 +2033,6 @@ bool TRI_CopySymlink(std::string const& srcItem, std::string const& dstItem, return true; } - //////////////////////////////////////////////////////////////////////////////// /// @brief creates a hard link; the link target is not altered. //////////////////////////////////////////////////////////////////////////////// diff --git a/lib/Basics/files.h b/lib/Basics/files.h index ee366c31cebb..a6e220bb02c9 100644 --- a/lib/Basics/files.h +++ b/lib/Basics/files.h @@ -43,6 +43,8 @@ #include "Enterprise/Encryption/EncryptionFeature.h" #endif +struct stat; + //////////////////////////////////////////////////////////////////////////////// /// @brief returns the size of a file /// @@ -352,7 +354,13 @@ ErrorCode TRI_GetTempName(char const* directory, std::string& result, bool creat /// @brief copies a file from source to dest. //////////////////////////////////////////////////////////////////////////////// +#ifdef _WIN32 bool TRI_CopyFile(std::string const& src, std::string const& dst, std::string& error); +#else +// this API allows passing already retrieved stat info to the copy routine, in +// order to avoid extra stat calls +bool TRI_CopyFile(std::string const& src, std::string const& dst, std::string& error, struct stat* statbuf = nullptr); +#endif //////////////////////////////////////////////////////////////////////////////// /// @brief copies the file Attributes from source to dest. diff --git a/lib/Basics/operating-system.h b/lib/Basics/operating-system.h index 37dde3f48c5c..00b710323f70 100644 --- a/lib/Basics/operating-system.h +++ b/lib/Basics/operating-system.h @@ -188,8 +188,8 @@ #define TRI_DUP ::dup #define TRI_RMDIR ::rmdir #define TRI_STAT ::stat -#define TRI_STAT_ATIME_SEC(statbuf) statbuf.st_atimespec.tv_sec -#define TRI_STAT_MTIME_SEC(statbuf) statbuf.st_mtimespec.tv_sec +#define TRI_STAT_ATIME_SEC(statbuf) (statbuf).st_atimespec.tv_sec +#define TRI_STAT_MTIME_SEC(statbuf) (statbuf).st_mtimespec.tv_sec #define TRI_UNLINK ::unlink #define TRI_WRITE ::write #define TRI_FDOPEN(a, b) ::fdopen((a), (b)) @@ -334,8 +334,8 @@ #define TRI_DUP ::dup #define TRI_RMDIR ::rmdir #define TRI_STAT ::stat -#define TRI_STAT_ATIME_SEC(statbuf) statbuf.st_atimespec.tv_sec -#define TRI_STAT_MTIME_SEC(statbuf) statbuf.st_mtimespec.tv_sec +#define TRI_STAT_ATIME_SEC(statbuf) (statbuf).st_atimespec.tv_sec +#define TRI_STAT_MTIME_SEC(statbuf) (statbuf).st_mtimespec.tv_sec #define TRI_UNLINK ::unlink #define TRI_WRITE ::write #define TRI_FDOPEN(a, b) ::fdopen((a), (b)) @@ -493,8 +493,8 @@ #define TRI_DUP ::dup #define TRI_RMDIR ::rmdir #define TRI_STAT ::stat -#define TRI_STAT_ATIME_SEC(statbuf) statbuf.st_atim.tv_sec -#define TRI_STAT_MTIME_SEC(statbuf) statbuf.st_mtim.tv_sec +#define TRI_STAT_ATIME_SEC(statbuf) (statbuf).st_atim.tv_sec +#define TRI_STAT_MTIME_SEC(statbuf) (statbuf).st_mtim.tv_sec #define TRI_UNLINK ::unlink #define TRI_WRITE ::write #define TRI_FDOPEN(a, b) ::fdopen((a), (b)) diff --git a/tests/js/client/server_parameters/test-copy-installation.js b/tests/js/client/server_parameters/test-copy-installation.js index 2514859d1846..7e763a143049 100644 --- a/tests/js/client/server_parameters/test-copy-installation.js +++ b/tests/js/client/server_parameters/test-copy-installation.js @@ -1,5 +1,5 @@ /*jshint globalstrict:false, strict:false */ -/* global getOptions, assertEqual, arango */ +/* global getOptions, assertEqual, assertTrue, assertFalse, arango */ //////////////////////////////////////////////////////////////////////////////// /// @brief test for security-related server options @@ -34,7 +34,10 @@ if (getOptions === true) { 'javascript.allow-admin-execute': 'true', }; } -var jsunity = require('jsunity'); + +const db = require("@arangodb").db; +const fs = require("fs"); +const jsunity = require('jsunity'); function testSuite() { const errors = require('@arangodb').errors; @@ -44,12 +47,15 @@ function testSuite() { tearDown: function() {}, testCanExecuteAction : function() { - // this test just does something, and it doesn't really matter - // the key thing in this test is to get the server started using - // the `--javascript.copy-installation true` setting without failing - let data = "return 'test!'"; - let result = arango.POST("/_admin/execute", data); - assertEqual("test!", result); + // fetch server-side database directory name + let data = "return require('@arangodb').db._path();"; + let dbPath = arango.POST("/_admin/execute", data); + let jsPath = fs.join(dbPath, "js"); + assertTrue(fs.exists(jsPath)); + assertTrue(fs.exists(fs.join(jsPath, "node"))); + assertTrue(fs.exists(fs.join(jsPath, "node", "node_modules"))); + assertTrue(fs.exists(fs.join(jsPath, "node", "node_modules", "lodash"))); + assertFalse(fs.exists(fs.join(jsPath, "node", "eslint"))); }, }; From f786d8c6e069015b181dffc49d2391780b24346c Mon Sep 17 00:00:00 2001 From: jsteemann Date: Tue, 20 Jul 2021 10:50:09 +0200 Subject: [PATCH 2/3] more fixes for node_moduels copying --- arangod/V8Server/V8DealerFeature.cpp | 53 +++++++++++----------------- 1 file changed, 20 insertions(+), 33 deletions(-) diff --git a/arangod/V8Server/V8DealerFeature.cpp b/arangod/V8Server/V8DealerFeature.cpp index dd38e409273b..990461eb26bd 100644 --- a/arangod/V8Server/V8DealerFeature.cpp +++ b/arangod/V8Server/V8DealerFeature.cpp @@ -343,33 +343,7 @@ void V8DealerFeature::validateOptions(std::shared_ptr options) { v8security.addToInternalAllowList(_startupDirectory, FSAccessType::READ); ctx->normalizePath(_moduleDirectories, "javascript.module-directory", false); - - // try to append the current version name to the startup directory, - // so instead of "/path/to/js" we will get "/path/to/js/3.4.0" - std::string const versionAppendix = - std::regex_replace(rest::Version::getServerVersion(), std::regex("-.*$"), - ""); - std::string versionedPath = - basics::FileUtils::buildFilename(_startupDirectory, versionAppendix); - - LOG_TOPIC("604da", DEBUG, Logger::V8) - << "checking for existence of version-specific startup-directory '" - << versionedPath << "'"; - if (basics::FileUtils::isDirectory(versionedPath)) { - // version-specific js path exists! - _startupDirectory = versionedPath; - } - for (auto& it : _moduleDirectories) { - versionedPath = basics::FileUtils::buildFilename(it, versionAppendix); - - LOG_TOPIC("8e21a", DEBUG, Logger::V8) - << "checking for existence of version-specific module-directory '" - << versionedPath << "'"; - if (basics::FileUtils::isDirectory(versionedPath)) { - // version-specific js path exists! - it = versionedPath; - } v8security.addToInternalAllowList(it, FSAccessType::READ); } @@ -409,26 +383,36 @@ void V8DealerFeature::start() { // now check if we have a js directory inside the database directory, and if // it looks good auto& dbPathFeature = server().getFeature(); - const std::string dbJSPath = + std::string const dbJSPath = FileUtils::buildFilename(dbPathFeature.directory(), "js"); - const std::string checksumFile = + std::string const checksumFile = FileUtils::buildFilename(dbJSPath, StaticStrings::checksumFileJs); - const std::string serverPath = FileUtils::buildFilename(dbJSPath, "server"); - const std::string commonPath = FileUtils::buildFilename(dbJSPath, "common"); + std::string const serverPath = FileUtils::buildFilename(dbJSPath, "server"); + std::string const commonPath = FileUtils::buildFilename(dbJSPath, "common"); + std::string const nodeModulesPath = FileUtils::buildFilename(dbJSPath, "node", "node_modules"); if (FileUtils::isDirectory(dbJSPath) && FileUtils::exists(checksumFile) && FileUtils::isDirectory(serverPath) && FileUtils::isDirectory(commonPath)) { - // only load node modules from original startup path - _nodeModulesDirectory = _startupDirectory; // js directory inside database directory looks good. now use it! _startupDirectory = dbJSPath; + // older versions didn't copy node_modules. so check if it exists inside the + // database directory or not. + if (FileUtils::isDirectory(nodeModulesPath)) { + _nodeModulesDirectory = nodeModulesPath; + } else { + _nodeModulesDirectory = _startupDirectory; + } } } + + V8SecurityFeature& v8security = server().getFeature(); + v8security.addToInternalAllowList(_startupDirectory, FSAccessType::READ); + v8security.addToInternalAllowList(_nodeModulesDirectory, FSAccessType::READ); LOG_TOPIC("77c97", DEBUG, Logger::V8) << "effective startup-directory: " << _startupDirectory << ", effective module-directories: " << _moduleDirectories << ", node-modules-directory: " << _nodeModulesDirectory; - + _startupLoader.setDirectory(_startupDirectory); // dump paths @@ -672,7 +656,10 @@ void V8DealerFeature::copyInstallationFiles() { LOG_TOPIC("38e1e", INFO, Logger::V8) << "copying " << copied << " JS installation file(s) took " << Logger::FIXED(TRI_microtime() - start, 6) << "s"; } + + // finally switch over the paths _startupDirectory = copyJSPath; + _nodeModulesDirectory = basics::FileUtils::buildFilename(copyJSPath, "node", "node_modules"); } V8Context* V8DealerFeature::addContext() { From e67cb2a9fd0c00cdf12328a7265780425b164745 Mon Sep 17 00:00:00 2001 From: jsteemann Date: Tue, 20 Jul 2021 12:37:38 +0200 Subject: [PATCH 3/3] fix allowlisting --- arangod/V8Server/V8DealerFeature.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/arangod/V8Server/V8DealerFeature.cpp b/arangod/V8Server/V8DealerFeature.cpp index 990461eb26bd..d68fefa1568f 100644 --- a/arangod/V8Server/V8DealerFeature.cpp +++ b/arangod/V8Server/V8DealerFeature.cpp @@ -343,8 +343,10 @@ void V8DealerFeature::validateOptions(std::shared_ptr options) { v8security.addToInternalAllowList(_startupDirectory, FSAccessType::READ); ctx->normalizePath(_moduleDirectories, "javascript.module-directory", false); - for (auto& it : _moduleDirectories) { - v8security.addToInternalAllowList(it, FSAccessType::READ); + for (auto const& it : _moduleDirectories) { + if (!it.empty()) { + v8security.addToInternalAllowList(it, FSAccessType::READ); + } } // check whether app-path was specified @@ -406,7 +408,9 @@ void V8DealerFeature::start() { V8SecurityFeature& v8security = server().getFeature(); v8security.addToInternalAllowList(_startupDirectory, FSAccessType::READ); - v8security.addToInternalAllowList(_nodeModulesDirectory, FSAccessType::READ); + if (!_nodeModulesDirectory.empty()) { + v8security.addToInternalAllowList(_nodeModulesDirectory, FSAccessType::READ); + } LOG_TOPIC("77c97", DEBUG, Logger::V8) << "effective startup-directory: " << _startupDirectory