fix: prevent indefinite blocking in fetchWindowPreview pipe read#1487
fix: prevent indefinite blocking in fetchWindowPreview pipe read#1487deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
Conversation
Reviewer's GuideRefactors fetchWindowPreview()’s pipe handling and image read logic to prevent indefinite blocking and out-of-bounds access by using CLOEXEC pipes, a poll() + timeout-based read loop sized by imageStride, and ensuring the QImage owns its data via a deep copy. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
yixinshark
force-pushed
the
fix-readblock
branch
from
599c9fb to
ac0ca8c
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new poll/read loop will still block the UI thread for up to 5 seconds; consider moving this logic off the main thread or using a shorter timeout with an early abort to reduce perceived freezes when KWin stalls.
- In the poll/read loop, it may be safer to explicitly handle EINTR for both poll and read (e.g., by retrying) so that transient signal interruptions don’t cause spurious failures or early aborts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new poll/read loop will still block the UI thread for up to 5 seconds; consider moving this logic off the main thread or using a shorter timeout with an early abort to reduce perceived freezes when KWin stalls.
- In the poll/read loop, it may be safer to explicitly handle EINTR for both poll and read (e.g., by retrying) so that transient signal interruptions don’t cause spurious failures or early aborts.
## Individual Comments
### Comment 1
<location path="panels/dock/taskmanager/x11preview.cpp" line_range="121" />
<code_context>
- QImage image(reinterpret_cast<uchar *>(fileContent.data()), imageWidth, imageHeight, imageStride, qimageFormat);
- // close read
+ // imageStride 是每行实际字节数（含内存对齐 padding），必须用它而非 width*bpp/8
+ qsizetype expectedSize = static_cast<qsizetype>(imageHeight) * imageStride;
+ QByteArray fileContent(expectedSize, Qt::Uninitialized);
+ qsizetype totalRead = 0;
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider bounding `expectedSize` to avoid excessive allocations or overflow risk for malformed inputs.
If `imageHeight` or `imageStride` is unexpectedly large (e.g. from corrupted/hostile input), this can trigger a huge `QByteArray` allocation, causing memory exhaustion or failure. Add a sanity check on `expectedSize` (e.g., cap it based on screen size or a hard max buffer) and abort when it exceeds that limit.
Suggested implementation:
```cpp
QImage::Format qimageFormat = static_cast<QImage::Format>(imageFormat);
// imageStride 是每行实际字节数（含内存对齐 padding），必须用它而非 width*bpp/8
// 为避免恶意 / 损坏输入导致过大分配，这里做上限检查
constexpr qsizetype kMaxPreviewBufferBytes = 64 * 1024 * 1024; // 64 MiB 上限，可视需要调整
if (imageHeight <= 0 || imageStride <= 0) {
qWarning(x11WindowPreview) << "fetchWindowPreview: invalid image dimensions"
<< "height =" << imageHeight
<< "stride =" << imageStride;
return {};
}
qsizetype expectedSize = static_cast<qsizetype>(imageHeight) * imageStride;
if (expectedSize <= 0 || expectedSize > kMaxPreviewBufferBytes) {
qWarning(x11WindowPreview) << "fetchWindowPreview: expected buffer too large or invalid"
<< "expectedSize =" << expectedSize
<< "height =" << imageHeight
<< "stride =" << imageStride;
return {};
}
QByteArray fileContent(expectedSize, Qt::Uninitialized);
qsizetype totalRead = 0;
```
If `fetchWindowPreview` does not return a `QImage` (or another type that can be value-initialized with `{}`), adjust the two `return {};` statements to return an appropriate error value for that function (e.g., `return QImage();`, `return false;`, or propagate the error via your existing error-handling mechanism).
</issue_to_address>
### Comment 2
<location path="panels/dock/taskmanager/x11preview.cpp" line_range="120" />
<code_context>
- QByteArray fileContent = file.read(imageHeight * imageWidth * bitsCountPerPixel / 8);
- QImage image(reinterpret_cast<uchar *>(fileContent.data()), imageWidth, imageHeight, imageStride, qimageFormat);
- // close read
+ // imageStride 是每行实际字节数（含内存对齐 padding），必须用它而非 width*bpp/8
+ qsizetype expectedSize = static_cast<qsizetype>(imageHeight) * imageStride;
+ QByteArray fileContent(expectedSize, Qt::Uninitialized);
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the timeout-based pipe reading and pixmap construction into small helper functions to keep fetchWindowPreview’s main logic linear and easier to follow.
Extracting the low-level I/O into helpers would reduce the local complexity of `fetchWindowPreview` without changing behavior.
You can keep all the new robustness (timeout, partial reads, stride-based sizing) but move it out of the main function:
```cpp
namespace {
// blocking read with timeout for a known-size buffer
bool readWithTimeout(int fd, QByteArray &buffer, qsizetype expectedSize, int timeoutMs)
{
buffer.resize(expectedSize);
qsizetype totalRead = 0;
while (totalRead < expectedSize) {
struct pollfd pfd{fd, POLLIN, 0};
int ret = ::poll(&pfd, 1, timeoutMs);
if (ret == 0) {
qWarning(x11WindowPreview) << "fetchWindowPreview: pipe read timeout, KWin may have stalled";
return false;
}
if (ret < 0) {
qWarning(x11WindowPreview) << "fetchWindowPreview: poll error:" << strerror(errno);
return false;
}
if (pfd.revents & (POLLERR | POLLNVAL)) {
qWarning(x11WindowPreview) << "fetchWindowPreview: pipe error, revents=" << pfd.revents;
return false;
}
const ssize_t n = ::read(fd, buffer.data() + totalRead, expectedSize - totalRead);
if (n <= 0) {
if (n < 0)
qWarning(x11WindowPreview) << "fetchWindowPreview: read error:" << strerror(errno);
return false; // n == 0: EOF before expectedSize
}
totalRead += n;
}
return true;
}
QPixmap pixmapFromRawData(const QByteArray &data,
int width, int height, int stride,
QImage::Format format)
{
QImage image(reinterpret_cast<const uchar *>(data.constData()),
width, height, stride, format);
return QPixmap::fromImage(image.copy()); // deep copy, same as current behavior
}
} // namespace
```
Then `fetchWindowPreview` becomes much closer to the original linear flow:
```cpp
QPixmap fetchWindowPreview(qulonglong winId)
{
// ... unchanged setup code ...
int imageWidth = imageInfo.value("width").toUInt();
int imageHeight = imageInfo.value("height").toUInt();
int imageStride = imageInfo.value("stride").toUInt();
int imageFormat = imageInfo.value("format").toUInt();
if (imageWidth <= 1 || imageHeight <= 1) {
::close(fd[0]);
return QPixmap();
}
QImage::Format qimageFormat = static_cast<QImage::Format>(imageFormat);
const qsizetype expectedSize = static_cast<qsizetype>(imageHeight) * imageStride;
QByteArray fileContent;
constexpr int kTimeoutMs = 5000;
const bool ok = readWithTimeout(fd[0], fileContent, expectedSize, kTimeoutMs);
::close(fd[0]);
if (!ok) {
qWarning(x11WindowPreview) << "fetchWindowPreview: incomplete data, expected" << expectedSize;
return QPixmap();
}
auto pixmap = pixmapFromRawData(fileContent, imageWidth, imageHeight, imageStride, qimageFormat);
if (!pixmap.isNull()) {
s_windowPreviewCache.insert(winId, pixmap);
}
return pixmap;
}
```
Benefits:
- `fetchWindowPreview` no longer mixes protocol, process/pipe setup, polling, and image construction.
- The timeout/read logic is now reusable and easier to unit-test in isolation.
- The raw pointer cast and `image.copy()` semantics are encapsulated in a single helper.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
When KWin's screenshot service encounters an internal error, it may hold the write end of the pipe open without writing any data and without closing it. In this case, the previous blocking `QFile::read()` would wait forever, hanging the taskbar process. Three issues are fixed in fetchWindowPreview(): 1. Replace `pipe()` with `pipe2(fd, O_CLOEXEC)` to prevent the write fd from being inherited by child processes, which would also cause read() to block indefinitely. 2. Replace the blocking `QFile::read()` with a `poll()`-based read loop with a 5-second timeout. If KWin stalls or the pipe produces no data within the timeout, the function aborts gracefully and returns an empty QPixmap instead of hanging. 3. Fix the read buffer size calculation: use `imageHeight * imageStride` instead of `imageWidth * imageHeight * bpp / 8`. imageStride is the actual bytes-per-row (including alignment padding) as reported by KWin, and may be larger than width * bpp/8. The old calculation could result in reading too few bytes, leading to an out-of-bounds access when constructing QImage. Also added image.copy() to ensure the QImage owns its data independently of the fileContent buffer. 当 KWin 截图服务发生内部异常时，可能持有管道写端却不写入任何数据 也不关闭，导致原先的阻塞式 `QFile::read()` 永久等待，任务栏进程卡死。 本次修复了 fetchWindowPreview() 中的三个问题： 1. 将 `pipe()` 改为 `pipe2(fd, O_CLOEXEC)`，防止写端 fd 被 fork 出的子进程继承，否则子进程持有写端也会导致 read() 永久阻塞。 2. 将阻塞式 `QFile::read()` 替换为带 5 秒超时的 `poll()` 读取循环。 若 KWin 卡顿或管道在超时内无数据，函数会提前退出并返回空的 QPixmap，而不是使进程永久挂起。 3. 修正读取缓冲区大小计算：使用 `imageHeight * imageStride` 而非 `imageWidth * imageHeight * bpp / 8`。imageStride 是 KWin 返回的 每行实际字节数（含内存对齐 padding），可能大于 width*bpp/8。 原来的计算会导致读取字节数偏小，在构造 QImage 时发生越界访问。 同时添加 image.copy() 确保 QImage 独立持有数据。 Log: prevent indefinite blocking in fetchWindowPreview pipe read
yixinshark
force-pushed
the
fix-readblock
branch
from
ac0ca8c to
443c96d
Compare
deepin pr auto review这段巴达代码审查。这段代码主要改进了管道通信的健壮性和安全性，以下是详细的审查意见： 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
5. 改进建议
总结这段代码改进了管道通信的健壮性和安全性，主要解决了潜在的阻塞问题和缓冲区溢出风险。代码逻辑清晰，错误处理完善，是一个高质量的改进。建议根据上述改进建议进一步优化代码的可维护性和可配置性。 |
|
#0 __GI___libc_read (nbytes=3840, buf=0x557aa3d9d3f0, fd=54) at ../sysdeps/unix/sysv/linux/read.c:26 用户操作插拔屏幕后，任务栏卡死，以上是堆栈，在读截图管道数据的时候卡住。 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, yixinshark The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/forcemerge |
|
This pr force merged! (status: blocked) |
3 participants
When KWin's screenshot service encounters an internal error, it may hold the write end of the pipe open without writing any data and without closing it. In this case, the previous blocking
QFile::read()would wait forever, hanging the taskbar process.Three issues are fixed in fetchWindowPreview():
Replace
pipe()withpipe2(fd, O_CLOEXEC)to prevent the write fd from being inherited by child processes, which would also cause read() to block indefinitely.Replace the blocking
QFile::read()with apoll()-based read loop with a 5-second timeout. If KWin stalls or the pipe produces no data within the timeout, the function aborts gracefully and returns an empty QPixmap instead of hanging.Fix the read buffer size calculation: use
imageHeight * imageStrideinstead ofimageWidth * imageHeight * bpp / 8. imageStride is the actual bytes-per-row (including alignment padding) as reported by KWin, and may be larger than width * bpp/8. The old calculation could result in reading too few bytes, leading to an out-of-bounds access when constructing QImage. Also added image.copy() to ensure the QImage owns its data independently of the fileContent buffer.当 KWin 截图服务发生内部异常时，可能持有管道写端却不写入任何数据
也不关闭，导致原先的阻塞式
QFile::read()永久等待，任务栏进程卡死。本次修复了 fetchWindowPreview() 中的三个问题：
将
pipe()改为pipe2(fd, O_CLOEXEC)，防止写端 fd 被 fork 出的子进程继承，否则子进程持有写端也会导致 read() 永久阻塞。将阻塞式
QFile::read()替换为带 5 秒超时的poll()读取循环。 若 KWin 卡顿或管道在超时内无数据，函数会提前退出并返回空的 QPixmap，而不是使进程永久挂起。修正读取缓冲区大小计算：使用
imageHeight * imageStride而非imageWidth * imageHeight * bpp / 8。imageStride 是 KWin 返回的 每行实际字节数（含内存对齐 padding），可能大于 width*bpp/8。 原来的计算会导致读取字节数偏小，在构造 QImage 时发生越界访问。 同时添加 image.copy() 确保 QImage 独立持有数据。Log: prevent indefinite blocking in fetchWindowPreview pipe read
Summary by Sourcery
Prevent window preview fetching from hanging when reading from the KWin screenshot pipe and ensure image data is fully and safely read.
Bug Fixes:
Enhancements: