Skip to content

RDKEMW-10995: Added proper fd validation #406

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.

Sign up for GitHub

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

Closed
ks734 wants to merge 8 commits into from
Closed

RDKEMW-10995: Added proper fd validation #406

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c796b85
RDKEMW-10995: Proper fd validation and avoid double-close
ks734 Jan 28, 2026
8a64096
RDKEMW-10995: Fix copilot review changes
ks734 Feb 3, 2026
de96cb1
RDKEMW-10995: Fix copilot review changes
ks734 Feb 3, 2026
d207aae
RDKEMW-10995: Fix copilot review changes
ks734 Feb 3, 2026
7cc605a
RDKEMW-10995: Fix copilot review changes
ks734 Feb 3, 2026
cb49379
RDKEMW-10995: Fix copilot review changes
ks734 Feb 3, 2026
233f872
RDKEMW-10995: Proper fd validation
ks734 Feb 3, 2026
20cc805
Update FileSink.h
ks734 Feb 3, 2026
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
135 changes: 119 additions & 16 deletions rdkPlugins/Logging/source/FileSink.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <sys/socket.h>
#include <sys/un.h>
#include <limits.h>
#include <errno.h>

/**
* @brief A logging sink that sends the contents of the container stdout/err to a given file. The file
Expand All @@ -41,7 +42,11 @@
FileSink::FileSink(const std::string &containerId, std::shared_ptr<rt_dobby_schema> &containerConfig)
: mContainerConfig(containerConfig),
mContainerId(containerId),
mFileSizeLimit(SSIZE_MAX),
mOutputFileFd(-1),
mDevNullFd(-1),
mLimitHit(false),
mClosed(false),
mBuf{}
{
AI_LOG_FN_ENTRY();
Expand Down Expand Up @@ -71,13 +76,19 @@ FileSink::FileSink(const std::string &containerId, std::shared_ptr<rt_dobby_sche
mFileSizeLimit = SSIZE_MAX;
}
}
else
{
// Config says "log to file" but file options are missing; default to /dev/null
AI_LOG_WARN("No file options provided for container log; sending output to /dev/null");
mOutputFilePath = "/dev/null";
}

mOutputFileFd = openFile(mOutputFilePath);
if (mOutputFileFd < 0)
{
// Couldn't open our output file, send to /dev/null to avoid blocking
AI_LOG_SYS_ERROR(errno, "Failed to open container logfile - sending to /dev/null");
if (mDevNullFd > 0)
if (mDevNullFd >= 0)
{
mOutputFileFd = mDevNullFd;
}
Expand All @@ -88,19 +99,36 @@ FileSink::FileSink(const std::string &containerId, std::shared_ptr<rt_dobby_sche

FileSink::~FileSink()
{
// Close everything we opened
if (close(mDevNullFd) < 0)
int devNullFdToClose = -1;
int outputFdToClose = -1;

{
AI_LOG_SYS_ERROR(errno, "Failed to close /dev/null");
// Prevent races with DumpLog/process (both take mLock)
std::lock_guard<std::mutex> locker(mLock);
mClosed = true;

devNullFdToClose = mDevNullFd;
outputFdToClose = mOutputFileFd;
mDevNullFd = -1;
mOutputFileFd = -1;
}

if (mOutputFileFd > 0)
// Close everything we opened; avoid closing the same fd twice
if (outputFdToClose >= 0 && outputFdToClose != devNullFdToClose)
{
if (close(mOutputFileFd) < 0)
if (close(outputFdToClose) < 0)
{
AI_LOG_SYS_ERROR(errno, "Failed to close output file");
}
}

if (devNullFdToClose >= 0)
{
if (close(devNullFdToClose) < 0)
{
AI_LOG_SYS_ERROR(errno, "Failed to close /dev/null");
}
}
}

/**
Expand All @@ -117,6 +145,11 @@ void FileSink::DumpLog(const int bufferFd)

std::lock_guard<std::mutex> locker(mLock);

if (mClosed)
{
return;
}

ssize_t ret;
ssize_t offset = 0;

Expand All @@ -134,9 +167,37 @@ void FileSink::DumpLog(const int bufferFd)
if (offset <= mFileSizeLimit)
{
// Write to the output file
if (write(mOutputFileFd, mBuf, ret) < 0)
if (mOutputFileFd >= 0)
{
AI_LOG_SYS_ERROR(errno, "Write failed");
if (TEMP_FAILURE_RETRY(write(mOutputFileFd, mBuf, ret)) < 0)
{
AI_LOG_SYS_ERROR(errno, "Write failed to fd %d", mOutputFileFd);

// Handle persistent errors that require fallback
if (errno == EBADF || errno == EPIPE || errno == ENOSPC || errno == EIO)
{
if (mOutputFileFd != mDevNullFd && mDevNullFd >= 0)
{
// Fall back to /dev/null if it's a different, valid fd
AI_LOG_WARN("Switching to /dev/null for container %s", mContainerId.c_str());
mOutputFileFd = mDevNullFd;
}
else if (mOutputFileFd == mDevNullFd)
{
// Already writing to /dev/null and that failed
AI_LOG_ERROR("Write to /dev/null failed - disabling logging for container %s",
mContainerId.c_str());
mOutputFileFd = -1;
}
else
{
AI_LOG_ERROR("Cannot switch logging for container %s to /dev/null: invalid fd %d",
mContainerId.c_str(), mDevNullFd);
mOutputFileFd = -1;
}
}
// Note: EINTR is handled by TEMP_FAILURE_RETRY
}
}
}
else
Expand All @@ -148,17 +209,24 @@ void FileSink::DumpLog(const int bufferFd)
mContainerId.c_str(), mFileSizeLimit);
}
mLimitHit = true;
write(mDevNullFd, mBuf, ret);
if (mDevNullFd >= 0)
{
TEMP_FAILURE_RETRY(write(mDevNullFd, mBuf, ret));
}
}
}

#if (AI_BUILD_TYPE == AI_DEBUG)
// Separate sections of log file for reabability
// Separate sections of log file for readability
// (useful if we're writing lots of buffer dumps)
if (!mLimitHit)
if (!mLimitHit && mOutputFileFd >= 0)
{
std::string marker = "---------------------------------------------\n";
write(mOutputFileFd, marker.c_str(), marker.length());
if (TEMP_FAILURE_RETRY(write(mOutputFileFd, marker.c_str(), marker.length())) < 0)
{
// Silent failure acceptable for debug marker, but handle gracefully
AI_LOG_SYS_ERROR(errno, "Failed to write debug marker");
}
}
#endif
}
Expand All @@ -172,6 +240,11 @@ void FileSink::process(const std::shared_ptr<AICommon::IPollLoop> &pollLoop, epo
{
std::lock_guard<std::mutex> locker(mLock);

if (mClosed)
{
return;
}

if (event.events & EPOLLIN)
{
ssize_t ret;
Expand Down Expand Up @@ -199,9 +272,36 @@ void FileSink::process(const std::shared_ptr<AICommon::IPollLoop> &pollLoop, epo
if (offset <= mFileSizeLimit)
{
// Write to the output file
if (write(mOutputFileFd, mBuf, ret) < 0)
if (mOutputFileFd >= 0)
{
AI_LOG_SYS_ERROR(errno, "Write to %s failed", mOutputFilePath.c_str());
if (TEMP_FAILURE_RETRY(write(mOutputFileFd, mBuf, ret)) < 0)
{
AI_LOG_SYS_ERROR(errno, "Write to %s failed", mOutputFilePath.c_str());

// Handle persistent errors that require fallback
if (errno == EBADF || errno == EPIPE || errno == ENOSPC || errno == EIO)
{
if (mOutputFileFd != mDevNullFd && mDevNullFd >= 0)
{
// Fall back to /dev/null if it's a different, valid fd
AI_LOG_WARN("Switching to /dev/null for container %s", mContainerId.c_str());
mOutputFileFd = mDevNullFd;
}
else if (mOutputFileFd == mDevNullFd)
{
// Already writing to /dev/null and that failed
AI_LOG_ERROR("Write to /dev/null failed - disabling logging for container %s",
mContainerId.c_str());
mOutputFileFd = -1;
}
else
{
AI_LOG_ERROR("Cannot switch logging for container %s to /dev/null: invalid fd %d",
mContainerId.c_str(), mDevNullFd);
mOutputFileFd = -1;
}
}
}
}
}
else
Expand All @@ -213,7 +313,10 @@ void FileSink::process(const std::shared_ptr<AICommon::IPollLoop> &pollLoop, epo
mContainerId.c_str(), mFileSizeLimit);
}
mLimitHit = true;
write(mDevNullFd, mBuf, ret);
if (mDevNullFd >= 0)
{
TEMP_FAILURE_RETRY(write(mDevNullFd, mBuf, ret));
}
}
}

Expand Down Expand Up @@ -264,4 +367,4 @@ int FileSink::openFile(const std::string &pathName)
}

return openedFd;
}
}
3 changes: 2 additions & 1 deletion rdkPlugins/Logging/source/FileSink.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ class FileSink : public ILoggingSink
int mDevNullFd;

bool mLimitHit;
bool mClosed;
char mBuf[PTY_BUFFER_SIZE];

std::mutex mLock;
};
};
Loading