Skip to content

Commit

Permalink
[Linux] Fix potential race when saving storage file (#35428)
Browse files Browse the repository at this point in the history
* [Linux] Do not print misleading KVS init log message

* [Linux] Fix race condition when saving comfiguration

The mkstemp() call creates a unique file and returns file descriptor for
opened file which should be used to write data. However, the previous
implementation, instead of using file descriptor opened the temp file by
the name, which could fail because of file removal between these calls.

* Sync NuttX and WebOS implementation with Linux

* Add exception

* Improve readability
  • Loading branch information
arkq authored Sep 6, 2024
1 parent 9b58d4c commit 513f241
Show file tree
Hide file tree
Showing 8 changed files with 228 additions and 93 deletions.
1 change: 1 addition & 0 deletions scripts/tools/check_includes_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@
'src/lib/support/jsontlv/JsonToTlv.h': {'string'},
'src/lib/support/jsontlv/TlvToJson.h': {'string'},
'src/lib/support/jsontlv/TextFormat.h': {'string'},
'src/lib/support/TemporaryFileStream.h': {'ostream', 'streambuf', 'string'},
'src/app/icd/client/DefaultICDClientStorage.cpp': {'vector'},
'src/app/icd/client/DefaultICDClientStorage.h': {'vector'},
'src/app/icd/client/DefaultICDStorageKey.h': {'vector'},
Expand Down
2 changes: 2 additions & 0 deletions src/lib/support/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ static_library("support") {
"Defer.h",
"FibonacciUtils.cpp",
"FibonacciUtils.h",
"FileDescriptor.h",
"FixedBufferAllocator.cpp",
"FixedBufferAllocator.h",
"Fold.h",
Expand Down Expand Up @@ -239,6 +240,7 @@ static_library("support") {
"StringBuilder.cpp",
"StringBuilder.h",
"StringSplitter.h",
"TemporaryFileStream.h",
"ThreadOperationalDataset.cpp",
"ThreadOperationalDataset.h",
"TimeUtils.cpp",
Expand Down
62 changes: 62 additions & 0 deletions src/lib/support/FileDescriptor.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
*
* Copyright (c) 2024 Project CHIP Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#pragma once

#include <unistd.h>
#include <utility>

namespace chip {

/// Unix file descriptor wrapper with RAII semantics.
class FileDescriptor
{
public:
FileDescriptor() = default;
explicit FileDescriptor(int fd) : mFd(fd) {}
~FileDescriptor() { Close(); }

/// Disallow copy and assignment.
FileDescriptor(const FileDescriptor &) = delete;
FileDescriptor & operator=(const FileDescriptor &) = delete;

FileDescriptor(FileDescriptor && other) noexcept : mFd(other.Release()) {}
FileDescriptor & operator=(FileDescriptor && other) noexcept
{
Close();
mFd = other.Release();
return *this;
}

int Get() const { return mFd; }

int Release() { return std::exchange(mFd, -1); }

int Close()
{
if (mFd != -1)
{
return close(std::exchange(mFd, -1));
}
return 0;
}

private:
int mFd = -1;
};

} // namespace chip
107 changes: 107 additions & 0 deletions src/lib/support/TemporaryFileStream.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/*
*
* Copyright (c) 2024 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#pragma once

#include <cstdlib>
#include <ostream>
#include <streambuf>
#include <string>
#include <unistd.h>

#include <lib/support/FileDescriptor.h>

namespace chip {
namespace DeviceLayer {
namespace Internal {

class FileDescriptorStreamBuf : public std::streambuf
{
public:
FileDescriptorStreamBuf() = default;
explicit FileDescriptorStreamBuf(int fd) : mFd(fd) {}

FileDescriptorStreamBuf(FileDescriptorStreamBuf &) = delete;
FileDescriptorStreamBuf & operator=(FileDescriptorStreamBuf &) = delete;

FileDescriptorStreamBuf(FileDescriptorStreamBuf && other) = default;
FileDescriptorStreamBuf & operator=(FileDescriptorStreamBuf && other) = default;

protected:
int overflow(int c) override
{
if (c != EOF)
{
char z = c;
if (write(mFd, &z, 1) != 1)
{
return EOF;
}
}
return c;
}

std::streamsize xsputn(const char * s, std::streamsize n) override { return write(mFd, s, static_cast<size_t>(n)); }

private:
int mFd = -1;
};

/// File stream for a temporary file compatible with std::ostream.
class TemporaryFileStream : public std::ostream
{
public:
TemporaryFileStream() : std::ostream(&mBuf) {}
explicit TemporaryFileStream(std::string nameTemplate) : std::ostream(&mBuf) { Open(std::move(nameTemplate)); };

/// Disallow copy and assignment.
TemporaryFileStream(const TemporaryFileStream &) = delete;
TemporaryFileStream & operator=(const TemporaryFileStream &) = delete;

/// Open a temporary file with a given name template.
///
/// In order to check if the file was opened successfully, use IsOpen().
void Open(std::string nameTemplate)
{
mFileName = std::move(nameTemplate);
mFd = FileDescriptor(mkstemp(mFileName.data()));
mBuf = FileDescriptorStreamBuf(mFd.Get());
}

/// Check if the file was opened successfully.
///
/// In case of failure, the error can be retrieved using errno.
bool IsOpen() const { return mFd.Get() != -1; };

/// Synchronize the file's contents with the underlying storage device.
///
/// In case of failure, the error can be retrieved using errno.
bool DataSync() { return fdatasync(mFd.Get()) == 0; }

/// Get the name of created temporary file.
const std::string & GetFileName() const { return mFileName; }

private:
FileDescriptor mFd;
FileDescriptorStreamBuf mBuf;
std::string mFileName;
};

} // namespace Internal
} // namespace DeviceLayer
} // namespace chip
3 changes: 2 additions & 1 deletion src/platform/Linux/CHIPLinuxStorage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,15 @@ CHIP_ERROR ChipLinuxStorage::Init(const char * configFile)
{
CHIP_ERROR retval = CHIP_NO_ERROR;

ChipLogDetail(DeviceLayer, "ChipLinuxStorage::Init: Using KVS config file: %s", StringOrNullMarker(configFile));
if (mInitialized)
{
ChipLogError(DeviceLayer, "ChipLinuxStorage::Init: Attempt to re-initialize with KVS config file: %s",
StringOrNullMarker(configFile));
return CHIP_NO_ERROR;
}

ChipLogDetail(DeviceLayer, "ChipLinuxStorage::Init: Using KVS config file: %s", StringOrNullMarker(configFile));

mConfigPath.assign(configFile);
retval = ChipLinuxStorageIni::Init();

Expand Down
46 changes: 18 additions & 28 deletions src/platform/Linux/CHIPLinuxStorageIni.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <lib/support/CHIPMem.h>
#include <lib/support/CodeUtils.h>
#include <lib/support/IniEscaping.h>
#include <lib/support/TemporaryFileStream.h>
#include <lib/support/logging/CHIPLogging.h>
#include <platform/Linux/CHIPLinuxStorageIni.h>
#include <platform/internal/CHIPDeviceLayerInternal.h>
Expand Down Expand Up @@ -91,34 +92,23 @@ CHIP_ERROR ChipLinuxStorageIni::AddConfig(const std::string & configFile)
// 3. Using rename() to overwrite the existing file
CHIP_ERROR ChipLinuxStorageIni::CommitConfig(const std::string & configFile)
{
CHIP_ERROR retval = CHIP_NO_ERROR;
std::string tmpPath = configFile + "-XXXXXX";

int fd = mkstemp(&tmpPath[0]);
if (fd != -1)
{
std::ofstream ofs;
ofs.open(tmpPath, std::ofstream::out | std::ofstream::trunc);
mConfigStore.generate(ofs);
close(fd);

if (rename(tmpPath.c_str(), configFile.c_str()) == 0)
{
ChipLogDetail(DeviceLayer, "wrote settings to %s", configFile.c_str());
}
else
{
ChipLogError(DeviceLayer, "failed to rename (%s), %s (%d)", tmpPath.c_str(), strerror(errno), errno);
retval = CHIP_ERROR_WRITE_FAILED;
}
}
else
{
ChipLogError(DeviceLayer, "failed to open file (%s) for writing", tmpPath.c_str());
retval = CHIP_ERROR_OPEN_FAILED;
}

return retval;
TemporaryFileStream tmpFile(configFile + "-XXXXXX");
VerifyOrReturnError(
tmpFile.IsOpen(), CHIP_ERROR_OPEN_FAILED,
ChipLogError(DeviceLayer, "Failed to create temp file %s: %s", tmpFile.GetFileName().c_str(), strerror(errno)));

mConfigStore.generate(tmpFile);
VerifyOrReturnError(
tmpFile.DataSync(), CHIP_ERROR_WRITE_FAILED,
ChipLogError(DeviceLayer, "Failed to sync temp file %s: %s", tmpFile.GetFileName().c_str(), strerror(errno)));

int rv = rename(tmpFile.GetFileName().c_str(), configFile.c_str());
VerifyOrReturnError(rv == 0, CHIP_ERROR_WRITE_FAILED,
ChipLogError(DeviceLayer, "Failed to rename %s to %s: %s", tmpFile.GetFileName().c_str(),
configFile.c_str(), strerror(errno)));

ChipLogDetail(DeviceLayer, "Wrote settings to %s", configFile.c_str());
return CHIP_NO_ERROR;
}

CHIP_ERROR ChipLinuxStorageIni::GetUInt16Value(const char * key, uint16_t & val)
Expand Down
50 changes: 18 additions & 32 deletions src/platform/NuttX/CHIPLinuxStorageIni.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <lib/support/CHIPMem.h>
#include <lib/support/CodeUtils.h>
#include <lib/support/IniEscaping.h>
#include <lib/support/TemporaryFileStream.h>
#include <lib/support/logging/CHIPLogging.h>
#include <platform/NuttX/CHIPLinuxStorageIni.h>
#include <platform/internal/CHIPDeviceLayerInternal.h>
Expand Down Expand Up @@ -91,38 +92,23 @@ CHIP_ERROR ChipLinuxStorageIni::AddConfig(const std::string & configFile)
// 3. Using rename() to overwrite the existing file
CHIP_ERROR ChipLinuxStorageIni::CommitConfig(const std::string & configFile)
{
CHIP_ERROR retval = CHIP_NO_ERROR;
std::string tmpPath = configFile + "-XXXXXX";

int fd = mkstemp(&tmpPath[0]);
if (fd != -1)
{
std::ofstream ofs;

ChipLogProgress(DeviceLayer, "writing settings to file (%s)", tmpPath.c_str());

ofs.open(tmpPath, std::ofstream::out | std::ofstream::trunc);
mConfigStore.generate(ofs);

close(fd);

if (rename(tmpPath.c_str(), configFile.c_str()) == 0)
{
ChipLogProgress(DeviceLayer, "renamed tmp file to file (%s)", configFile.c_str());
}
else
{
ChipLogError(DeviceLayer, "failed to rename (%s), %s (%d)", tmpPath.c_str(), strerror(errno), errno);
retval = CHIP_ERROR_WRITE_FAILED;
}
}
else
{
ChipLogError(DeviceLayer, "failed to open file (%s) for writing", tmpPath.c_str());
retval = CHIP_ERROR_OPEN_FAILED;
}

return retval;
TemporaryFileStream tmpFile(configFile + "-XXXXXX");
VerifyOrReturnError(
tmpFile.IsOpen(), CHIP_ERROR_OPEN_FAILED,
ChipLogError(DeviceLayer, "Failed to create temp file %s: %s", tmpFile.GetFileName().c_str(), strerror(errno)));

mConfigStore.generate(tmpFile);
VerifyOrReturnError(
tmpFile.DataSync(), CHIP_ERROR_WRITE_FAILED,
ChipLogError(DeviceLayer, "Failed to sync temp file %s: %s", tmpFile.GetFileName().c_str(), strerror(errno)));

int rv = rename(tmpFile.GetFileName().c_str(), configFile.c_str());
VerifyOrReturnError(rv == 0, CHIP_ERROR_WRITE_FAILED,
ChipLogError(DeviceLayer, "Failed to rename %s to %s: %s", tmpFile.GetFileName().c_str(),
configFile.c_str(), strerror(errno)));

ChipLogDetail(DeviceLayer, "Wrote settings to %s", configFile.c_str());
return CHIP_NO_ERROR;
}

CHIP_ERROR ChipLinuxStorageIni::GetUInt16Value(const char * key, uint16_t & val)
Expand Down
50 changes: 18 additions & 32 deletions src/platform/webos/CHIPWebOSStorageIni.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <lib/support/Base64.h>
#include <lib/support/CHIPMem.h>
#include <lib/support/CodeUtils.h>
#include <lib/support/TemporaryFileStream.h>
#include <lib/support/logging/CHIPLogging.h>
#include <platform/internal/CHIPDeviceLayerInternal.h>
#include <platform/webos/CHIPWebOSStorageIni.h>
Expand Down Expand Up @@ -70,38 +71,23 @@ CHIP_ERROR ChipLinuxStorageIni::AddConfig(const std::string & configFile)
// 3. Using rename() to overwrite the existing file
CHIP_ERROR ChipLinuxStorageIni::CommitConfig(const std::string & configFile)
{
CHIP_ERROR retval = CHIP_NO_ERROR;
std::string tmpPath = configFile + "-XXXXXX";

int fd = mkstemp(&tmpPath[0]);
if (fd != -1)
{
std::ofstream ofs;

ChipLogProgress(DeviceLayer, "writing settings to file (%s)", tmpPath.c_str());

ofs.open(tmpPath, std::ofstream::out | std::ofstream::trunc);
mConfigStore.generate(ofs);

close(fd);

if (rename(tmpPath.c_str(), configFile.c_str()) == 0)
{
ChipLogProgress(DeviceLayer, "renamed tmp file to file (%s)", configFile.c_str());
}
else
{
ChipLogError(DeviceLayer, "failed to rename (%s), %s (%d)", tmpPath.c_str(), strerror(errno), errno);
retval = CHIP_ERROR_WRITE_FAILED;
}
}
else
{
ChipLogError(DeviceLayer, "failed to open file (%s) for writing", tmpPath.c_str());
retval = CHIP_ERROR_OPEN_FAILED;
}

return retval;
TemporaryFileStream tmpFile(configFile + "-XXXXXX");
VerifyOrReturnError(
tmpFile.IsOpen(), CHIP_ERROR_OPEN_FAILED,
ChipLogError(DeviceLayer, "Failed to create temp file %s: %s", tmpFile.GetFileName().c_str(), strerror(errno)));

mConfigStore.generate(tmpFile);
VerifyOrReturnError(
tmpFile.DataSync(), CHIP_ERROR_WRITE_FAILED,
ChipLogError(DeviceLayer, "Failed to sync temp file %s: %s", tmpFile.GetFileName().c_str(), strerror(errno)));

int rv = rename(tmpFile.GetFileName().c_str(), configFile.c_str());
VerifyOrReturnError(rv == 0, CHIP_ERROR_WRITE_FAILED,
ChipLogError(DeviceLayer, "Failed to rename %s to %s: %s", tmpFile.GetFileName().c_str(),
configFile.c_str(), strerror(errno)));

ChipLogDetail(DeviceLayer, "Wrote settings to %s", configFile.c_str());
return CHIP_NO_ERROR;
}

CHIP_ERROR ChipLinuxStorageIni::GetUInt16Value(const char * key, uint16_t & val)
Expand Down

0 comments on commit 513f241

Please sign in to comment.