Skip to content

fix(dfileinfo): fix heap corruption caused by unlocked gfileinfo lifecycle #388

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

Merged
deepin-bot merged 1 commit into from
Sep 8, 2026
Merged

fix(dfileinfo): fix heap corruption caused by unlocked gfileinfo lifecycle #388

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
69 changes: 48 additions & 21 deletions src/dfm-io/dfm-io/dfileinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,32 @@ USING_IO_NAMESPACE
* DFileInfoPrivate
***********************************************/

static gboolean retireUnrefCb(gpointer data)
{
g_object_unref(static_cast<GFileInfo *>(data));
return G_SOURCE_REMOVE;
}

static GFileInfo *atomicLoadGFileInfo(GFileInfo *const *slot)
{
return static_cast<GFileInfo *>(g_atomic_pointer_get(reinterpret_cast<const volatile gpointer *>(slot)));
}

static GFileInfo *refGFileInfo(GFileInfo *info)
{
return info ? static_cast<GFileInfo *>(g_object_ref(info)) : nullptr;
}

static void replaceGFileInfo(GFileInfo **slot, GFileInfo *value)
{
GFileInfo *old = nullptr;
do {
old = atomicLoadGFileInfo(slot);
} while (!g_atomic_pointer_compare_and_exchange(reinterpret_cast<volatile gpointer *>(slot), old, value));
if (old)
g_timeout_add_seconds_full(G_PRIORITY_LOW, 3, retireUnrefCb, old, nullptr);
Comment on lines +33 to +50

@sourcery-ai sourcery-ai Bot Sep 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

issue (bug_risk): The reader performs a load followed by g_object_ref, while replaceGFileInfo() schedules the old object's unref after a fixed three seconds. If a reader is descheduled or otherwise delayed longer than three seconds between those instructions, retireUnrefCb() frees the object before g_object_ref() executes, so the reader still dereferences freed memory.

Triggers: When a reader thread is paused for at least three seconds after atomicLoadGFileInfo() returns and before refGFileInfo() increments the reference count.

Suggested fix: Use a reclamation scheme that guarantees the loaded pointer remains alive until the reader has referenced it, such as a mutex, hazard pointers, epochs, or an atomic reference-count acquisition protocol.

@sourcery-ai sourcery-ai Bot Sep 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

issue (bug_risk): Every replacement with a non-null old pointer queues an unref on the default GLib main context. In a host that does not iterate that context, the timeout never runs and one retired GFileInfo remains referenced for every refresh, causing unbounded memory growth.

Triggers: When this library is used by a CLI, worker, or other host without a running default GLib main loop.

Suggested fix: Provide an explicit reclamation path independent of the default main context, or document and enforce a main-context owner and synchronously drain/cancel retired references during teardown.

}

typedef struct
{
DFileInfo::AttributeAsyncCallback callback;
Expand Down Expand Up @@ -249,11 +275,7 @@ bool DFileInfoPrivate::queryInfoSync()
return false;
}

if (this->gfileinfo) {
g_object_unref(this->gfileinfo);
this->gfileinfo = nullptr;
}
this->gfileinfo = fileinfo;
replaceGFileInfo(&this->gfileinfo, fileinfo);

@sourcery-ai sourcery-ai Bot Sep 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

issue (bug_risk): replaceGFileInfo() updates gfileinfo atomically, but queryInfoSync(), queryInfoAsync(), and initQuerierAsync() still read the same member with ordinary non-atomic accesses. A concurrent refresh therefore creates a C++ data race and undefined behavior despite the new atomic writer.

Triggers: When a refresh callback or queryInfoSync() replaces gfileinfo concurrently with another initialization/query path checking the member.

Suggested fix: Use atomicLoadGFileInfo() for every gfileinfo read, including the early-exit checks, and make destruction/initial construction follow the same ownership protocol.

initFinished = true;
isQuquerying = false;
return true;
Expand Down Expand Up @@ -300,6 +322,7 @@ bool DFileInfoPrivate::ensureStatxCached() const

QVariant DFileInfoPrivate::attributesBySelf(DFileInfo::AttributeID id)
{
g_autoptr(GFileInfo) gfileinfo = refGFileInfo(atomicLoadGFileInfo(&this->gfileinfo));
QVariant retValue;
switch (id) {
case DFileInfo::AttributeID::kStandardIsHidden: {
Expand Down Expand Up @@ -603,6 +626,7 @@ DFile::Permissions DFileInfoPrivate::permissions() const

bool DFileInfoPrivate::exists() const
{
g_autoptr(GFileInfo) gfileinfo = refGFileInfo(atomicLoadGFileInfo(&this->gfileinfo));
if (!gfileinfo)
return false;
return g_file_info_get_file_type(gfileinfo) != G_FILE_TYPE_UNKNOWN;
Expand Down Expand Up @@ -639,7 +663,7 @@ void DFileInfoPrivate::queryInfoAsyncCallback(GObject *sourceObject, GAsyncResul
}

if (data->me) {
data->me->gfileinfo = fileinfo;
replaceGFileInfo(&data->me->gfileinfo, fileinfo);
data->me->initFinished = true;
}

Expand Down Expand Up @@ -677,7 +701,7 @@ void DFileInfoPrivate::queryInfoAsyncCallback2(GObject *sourceObject, GAsyncResu
}

if (data->me) {
data->me->gfileinfo = fileinfo;
replaceGFileInfo(&data->me->gfileinfo, fileinfo);
data->me->initFinished = true;

future->finished();
Expand Down Expand Up @@ -759,16 +783,17 @@ QVariant DFileInfo::attribute(DFileInfo::AttributeID id, bool *success) const
}
}

g_autoptr(GFileInfo) gfileinfo = refGFileInfo(atomicLoadGFileInfo(&d->gfileinfo));
QVariant retValue;
if (id > DFileInfo::AttributeID::kCustomStart) {
const QString &path = d->uri.path();
retValue = DLocalHelper::customAttributeFromPathAndInfo(path, d->gfileinfo, id);
retValue = DLocalHelper::customAttributeFromPathAndInfo(path, gfileinfo, id);
} else {
if (d->gfileinfo) {
if (gfileinfo) {
DFMIOErrorCode errorCode(DFM_IO_ERROR_NONE);
if (!d->attributesRealizationSelf.contains(id)) {
QMutexLocker lk(&d->mutex);
retValue = DLocalHelper::attributeFromGFileInfo(d->gfileinfo, id, errorCode);
retValue = DLocalHelper::attributeFromGFileInfo(gfileinfo, id, errorCode);
if (errorCode != DFM_IO_ERROR_NONE)
const_cast<DFileInfoPrivate *>(d.data())->error.setCode(errorCode);
} else {
Expand Down Expand Up @@ -935,11 +960,12 @@ bool DFileInfo::hasAttribute(DFileInfo::AttributeID id) const
return false;
}

if (d->gfileinfo) {
g_autoptr(GFileInfo) gfileinfo = refGFileInfo(atomicLoadGFileInfo(&d->gfileinfo));
if (gfileinfo) {
const std::string &key = DLocalHelper::attributeStringById(id);
if (key.empty())
return false;
return g_file_info_has_attribute(d->gfileinfo, key.c_str());
return g_file_info_has_attribute(gfileinfo, key.c_str());
}

return false;
Expand Down Expand Up @@ -991,40 +1017,41 @@ QVariant DFileInfo::customAttribute(const char *key, const DFileInfo::DFileAttri
return QVariant();
}

if (!d->gfileinfo)
g_autoptr(GFileInfo) gfileinfo = refGFileInfo(atomicLoadGFileInfo(&d->gfileinfo));
if (!gfileinfo)
return QVariant();

switch (type) {
case DFileInfo::DFileAttributeType::kTypeString: {
const char *ret = g_file_info_get_attribute_string(d->gfileinfo, key);
const char *ret = g_file_info_get_attribute_string(gfileinfo, key);
return QVariant(ret);
}
case DFileInfo::DFileAttributeType::kTypeByteString: {
const char *ret = g_file_info_get_attribute_byte_string(d->gfileinfo, key);
const char *ret = g_file_info_get_attribute_byte_string(gfileinfo, key);
return QVariant(ret);
}
case DFileInfo::DFileAttributeType::kTypeBool: {
bool ret = g_file_info_get_attribute_boolean(d->gfileinfo, key);
bool ret = g_file_info_get_attribute_boolean(gfileinfo, key);
return QVariant(ret);
}
case DFileInfo::DFileAttributeType::kTypeUInt32: {
uint32_t ret = g_file_info_get_attribute_uint32(d->gfileinfo, key);
uint32_t ret = g_file_info_get_attribute_uint32(gfileinfo, key);
return QVariant(ret);
}
case DFileInfo::DFileAttributeType::kTypeInt32: {
int32_t ret = g_file_info_get_attribute_int32(d->gfileinfo, key);
int32_t ret = g_file_info_get_attribute_int32(gfileinfo, key);
return QVariant(ret);
}
case DFileInfo::DFileAttributeType::kTypeUInt64: {
uint64_t ret = g_file_info_get_attribute_uint64(d->gfileinfo, key);
uint64_t ret = g_file_info_get_attribute_uint64(gfileinfo, key);
return QVariant(qulonglong(ret));
}
case DFileInfo::DFileAttributeType::kTypeInt64: {
int64_t ret = g_file_info_get_attribute_int64(d->gfileinfo, key);
int64_t ret = g_file_info_get_attribute_int64(gfileinfo, key);
return QVariant(qulonglong(ret));
}
case DFileInfo::DFileAttributeType::kTypeStringV: {
char **ret = g_file_info_get_attribute_stringv(d->gfileinfo, key);
char **ret = g_file_info_get_attribute_stringv(gfileinfo, key);
QStringList retValue;
for (int i = 0; ret && ret[i]; ++i) {
retValue.append(QString::fromLocal8Bit(ret[i]));
Expand Down
Loading