Hi On Tue, Oct 27, 2026 at 6:56 PM Kostiantyn Kostiuk <[email protected]> wrote: > > > > > On Tue, Oct 27, 2026 at 4:02 PM Marc-André Lureau > <[email protected]> wrote: >> >> Hi >> >> On Tue, Oct 27, 2026 at 5:54 PM Kostiantyn Kostiuk <[email protected]> >> wrote: >> > >> > >> > >> > On Tue, Oct 27, 2026 at 3:31 PM <[email protected]> wrote: >> >> >> >> From: Marc-André Lureau <[email protected]> >> >> >> >> Since commit 374f4676788c, the provider's GetSnapshotProperties() >> >> returns S_OK for any snapshot ID without filling in the properties. VSS >> >> consults every registered provider when a requester looks up a snapshot, >> >> so lookups of snapshots owned by other providers now succeed with an >> >> all-zero VSS_SNAPSHOT_PROP. Backup software relying on it fails, e.g. >> >> Veeam: "Original volume name is not specified for the shadow (shadow >> >> volume id: '{00000000-0000-0000-0000-000000000000}')". >> >> >> >> Simply reverting is not an option: 374f4676788c switched to S_OK because >> >> returning VSS_E_OBJECT_NOT_FOUND unconditionally broke qemu-ga's own >> >> snapshot sets. The provider must answer for its own snapshots only. It >> >> does not create real shadow copies, but it does take part in the >> >> snapshot set during qemu-ga fsfreeze cycles. Remember the snapshot IDs >> >> received in BeginPrepareSnapshot(), return their >> >> (minimal) properties, and return VSS_E_OBJECT_NOT_FOUND for anything >> >> else. >> >> >> >> Entries are dropped when VSS deletes the snapshots or the set, or when >> >> the set is aborted (AbortSnapshots()). >> >> >> >> COM+ may hand out several CQGAVssProvider instances (and serve several >> >> client processes), so this registry is module-global. It is per-process >> >> by design and safe because the COM+ application is hosted by a single NT >> >> service (see CreateServiceForApplication() in install.cpp): COM+ forbids >> >> pooling and recycling for service-hosted applications, so exactly one >> >> surrogate process ever hosts the provider. >> >> >> >> On the requester side, the snapshot set is created in the persistent >> >> VSS_CTX_APP_ROLLBACK context, so VSS keeps it after the freeze/thaw >> >> cycle and may keep querying providers about it. The provider registry >> >> does not outlive the cycle (requester_thaw() stops the hosting service, >> >> and the provider process with it), so later queries would get "not >> >> found" for a set VSS still believes exists. Make requester_thaw() delete >> >> the snapshot set explicitly at the end of the cycle, whatever its >> >> outcome (including BackupComplete() failure and aborted snapshot sets). >> >> A failure to delete is only logged, since the guest filesystems have >> >> already been thawed. >> >> >> >> Cc: [email protected] >> >> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4584 >> >> Buglink: https://redhat.atlassian.net/browse/RHEL-250662 >> >> Fixes: 374f4676788c ("qga: Fix VSS provider GetSnapshotProperties return >> >> value") >> >> Signed-off-by: Marc-André Lureau <[email protected]> >> >> --- >> >> qga/vss-win32/install.cpp | 1 + >> >> qga/vss-win32/provider.cpp | 153 +++++++++++++++++++++++++++++++++++- >> >> qga/vss-win32/requester.cpp | 26 ++++++ >> >> 3 files changed, 179 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp >> >> index 09d94edd4f53..3b58dc840520 100644 >> >> --- a/qga/vss-win32/install.cpp >> >> +++ b/qga/vss-win32/install.cpp >> >> @@ -331,6 +331,7 @@ STDAPI COMRegister(void) >> >> chk(put_Value(pObj, L"Authentication", short(6))); >> >> chk(put_Value(pObj, L"AuthenticationCapability", short(2))); >> >> chk(put_Value(pObj, L"ImpersonationLevel", short(2))); >> >> + /* NT-service-hosted apps have ConcurrentApps=1 enforced */ >> >> chk(pApps->SaveChanges(&n)); >> >> >> >> /* The app should be deleted if something fails after SaveChanges */ >> >> diff --git a/qga/vss-win32/provider.cpp b/qga/vss-win32/provider.cpp >> >> index 1c8eb236e57f..ad2f3ee763e6 100644 >> >> --- a/qga/vss-win32/provider.cpp >> >> +++ b/qga/vss-win32/provider.cpp >> >> @@ -11,6 +11,11 @@ >> >> */ >> >> >> >> #include "qemu/osdep.h" >> >> + >> >> +#include <vector> >> >> +#include <algorithm> >> >> +#include <string> >> >> + >> >> #include "vss-common.h" >> >> #include "vss-debug.h" >> >> #ifdef HAVE_VSS_SDK >> >> @@ -138,6 +143,123 @@ STDMETHODIMP >> >> CQGAVSSEnumObject::Clone(IVssEnumObject **ppenum) >> >> return E_NOTIMPL; >> >> } >> >> >> >> +/* Registry of the snapshot IDs for this provider */ >> >> +class QGAVssSnapshotRegistry >> >> +{ >> >> +public: >> >> + bool Track(VSS_ID set_id, VSS_ID snapshot_id, >> >> + VSS_PWSZ volume_name, LONG context); >> >> + HRESULT GetProperties(VSS_ID snapshot_id, VSS_SNAPSHOT_PROP *prop) >> >> const; >> >> + LONG RemoveSet(VSS_ID set_id); /* returns how many were removed */ >> >> + bool Remove(VSS_ID snapshot_id); >> >> + >> >> +private: >> >> + struct Entry { >> >> + VSS_ID set_id; >> >> + VSS_ID snapshot_id; >> >> + std::wstring volume_name; >> >> + LONG context; >> >> + }; >> >> + >> >> + std::vector<Entry> m_entries; >> >> + mutable SRWLOCK m_lock = SRWLOCK_INIT; >> >> +}; >> >> + >> >> +static QGAVssSnapshotRegistry qga_vss_registry; >> >> + >> >> +bool QGAVssSnapshotRegistry::Track(VSS_ID set_id, VSS_ID snapshot_id, >> >> + VSS_PWSZ volume_name, LONG context) >> >> +{ >> >> + bool ok; >> >> + >> >> + AcquireSRWLockExclusive(&m_lock); >> >> + try { >> >> + m_entries.push_back(Entry { set_id, snapshot_id, volume_name, >> >> + context }); >> >> + ok = true; >> >> + } catch (...) { >> >> + ok = false; >> >> + } >> >> + ReleaseSRWLockExclusive(&m_lock); >> >> + return ok; >> >> +} >> >> + >> >> +HRESULT QGAVssSnapshotRegistry::GetProperties(VSS_ID snapshot_id, >> >> + VSS_SNAPSHOT_PROP *prop) >> >> const >> >> +{ >> >> + HRESULT hr = S_OK; >> >> + >> >> + AcquireSRWLockShared(&m_lock); >> >> + auto it = std::find_if(m_entries.begin(), m_entries.end(), >> >> + [&](const Entry &e) { >> >> + return IsEqualGUID(e.snapshot_id, snapshot_id); >> >> + }); >> >> + if (it == m_entries.end()) { >> >> + hr = VSS_E_OBJECT_NOT_FOUND; >> >> + goto out; >> >> + } >> >> + >> >> + { >> >> + const Entry &e = *it; >> >> + size_t len = (e.volume_name.size() + 1) * sizeof(WCHAR); >> >> + VSS_PWSZ volume = (VSS_PWSZ)CoTaskMemAlloc(len); >> > >> > >> > I'm afraid of this part. We allocate memory but don't call CoTaskMemFree. >> > This function is called from IVssBackupComponents::GetSnapshotProperties >> > From >> > https://learn.microsoft.com/en-us/windows/win32/api/vsbackup/nf-vsbackup-ivssbackupcomponents-getsnapshotproperties >> > "When the structure is no longer needed, the software provider is >> > responsible for freeing these strings by calling the >> > VssFreeSnapshotProperties function." >> > >> > So we allocate memory but don't free it with VssFreeSnapshotProperties or >> > CoTaskMemFree. >> >> I also wondered, but this is apparently what is expected: >> >> https://github.com/MicrosoftDocs/sdk-api/blob/docs/sdk-api-src/content/vss/ns-vss-vss_object_prop.md >> >> >> A requester obtains VSS_OBJECT_PROP structures by using >> IVssEnumObject::Next to iterate over the list of objects returned by a >> call to IVssBackupComponents::Query. >> > Yes, but there is nothing about cleaning memory > > >> >> As its members are filled by a COM interface, prior to deleting the >> property structures VSS_SNAPSHOT_PROP and VSS_PROVIDER_PROP, the >> memory they contain must be released by calling CoTaskMemFree for >> every string and byte array value contained in each structure. >> >> In the case of VSS_SNAPSHOT_PROP, this can be done manually, or the >> utility function VssFreeSnapshotProperties can be used. >> > but we don't store the pointers
I don't follow you. https://learn.microsoft.com/en-us/windows/win32/api/vsprov/nf-vsprov-ivsssoftwaresnapshotprovider-getsnapshotproperties "All members are required except m_pwszExposedName and m_pwszExposedPath, which the provider can set to NULL. The provider allocates memory for all string members that it sets in the structure. When the structure is no longer needed, the caller is responsible for freeing these strings by calling the VssFreeSnapshotProperties function." So it looks like we should actually fill the other fields too > >> >> >> > >> >> + >> >> + if (!volume) { >> >> + hr = E_OUTOFMEMORY; >> >> + goto out; >> >> + } >> >> + memcpy(volume, e.volume_name.c_str(), len); >> >> + >> >> + /* not a real snapshot: no device object, exposure or timestamp >> >> */ >> >> + memset(prop, 0, sizeof(*prop)); >> >> + prop->m_SnapshotId = e.snapshot_id; >> >> + prop->m_SnapshotSetId = e.set_id; >> >> + prop->m_lSnapshotsCount = std::count_if(m_entries.begin(), >> >> + m_entries.end(), [&](const Entry &o) { >> >> + return IsEqualGUID(o.set_id, e.set_id); >> >> + }); >> >> + prop->m_pwszOriginalVolumeName = volume; >> >> + prop->m_ProviderId = g_gProviderId; >> >> + prop->m_lSnapshotAttributes = e.context; >> >> + prop->m_eStatus = VSS_SS_CREATED; >> >> + } >> >> + >> >> +out: >> >> + ReleaseSRWLockShared(&m_lock); >> >> + return hr; >> >> +} >> >> + >> >> +LONG QGAVssSnapshotRegistry::RemoveSet(VSS_ID set_id) >> >> +{ >> >> + LONG removed; >> >> + >> >> + AcquireSRWLockExclusive(&m_lock); >> >> + auto it = std::remove_if(m_entries.begin(), m_entries.end(), >> >> + [&](const Entry &e) { >> >> + return IsEqualGUID(e.set_id, set_id); >> >> + }); >> >> + removed = m_entries.end() - it; >> >> + m_entries.erase(it, m_entries.end()); >> >> + ReleaseSRWLockExclusive(&m_lock); >> >> + return removed; >> >> +} >> >> + >> >> +bool QGAVssSnapshotRegistry::Remove(VSS_ID snapshot_id) >> >> +{ >> >> + bool found; >> >> + >> >> + AcquireSRWLockExclusive(&m_lock); >> >> + auto it = std::find_if(m_entries.begin(), m_entries.end(), >> >> + [&](const Entry &e) { >> >> + return IsEqualGUID(e.snapshot_id, snapshot_id); >> >> + }); >> >> + found = it != m_entries.end(); >> >> + if (found) { >> >> + m_entries.erase(it); >> >> + } >> >> + ReleaseSRWLockExclusive(&m_lock); >> >> + return found; >> >> +} >> >> >> >> /* QGAVssProvider */ >> >> >> >> @@ -266,7 +388,7 @@ STDMETHODIMP CQGAVssProvider::SetContext(LONG >> >> lContext) >> >> STDMETHODIMP CQGAVssProvider::GetSnapshotProperties( >> >> VSS_ID SnapshotId, VSS_SNAPSHOT_PROP *pProp) >> >> { >> >> - return S_OK; >> >> + return qga_vss_registry.GetProperties(SnapshotId, pProp); >> >> } >> >> >> >> STDMETHODIMP CQGAVssProvider::Query( >> >> @@ -286,6 +408,27 @@ STDMETHODIMP CQGAVssProvider::DeleteSnapshots( >> >> VSS_ID SourceObjectId, VSS_OBJECT_TYPE eSourceObjectType, >> >> BOOL bForceDelete, LONG *plDeletedSnapshots, VSS_ID >> >> *pNondeletedSnapshotID) >> >> { >> >> + LONG deleted = 0; >> >> + >> >> + switch (eSourceObjectType) { >> >> + case VSS_OBJECT_SNAPSHOT: >> >> + if (qga_vss_registry.Remove(SourceObjectId)) { >> >> + deleted = 1; >> >> + } >> >> + break; >> >> + case VSS_OBJECT_SNAPSHOT_SET: >> >> + deleted = qga_vss_registry.RemoveSet(SourceObjectId); >> >> + break; >> >> + default: >> >> + break; >> >> + } >> >> + >> >> + if (deleted > 0) { >> >> + *plDeletedSnapshots = deleted; >> >> + *pNondeletedSnapshotID = GUID_NULL; >> >> + return S_OK; >> >> + } >> >> + >> >> *plDeletedSnapshots = 0; >> >> *pNondeletedSnapshotID = SourceObjectId; >> >> return S_OK; >> >> @@ -295,6 +438,13 @@ STDMETHODIMP CQGAVssProvider::BeginPrepareSnapshot( >> >> VSS_ID SnapshotSetId, VSS_ID SnapshotId, >> >> VSS_PWSZ pwszVolumeName, LONG lNewContext) >> >> { >> >> + if (!pwszVolumeName) { >> >> + return E_INVALIDARG; >> >> + } >> >> + if (!qga_vss_registry.Track(SnapshotSetId, SnapshotId, >> >> + pwszVolumeName, lNewContext)) { >> >> + return E_OUTOFMEMORY; >> >> + } >> >> return S_OK; >> >> } >> >> >> >> @@ -411,6 +561,7 @@ STDMETHODIMP >> >> CQGAVssProvider::PostFinalCommitSnapshots(VSS_ID SnapshotSetId) >> >> >> >> STDMETHODIMP CQGAVssProvider::AbortSnapshots(VSS_ID SnapshotSetId) >> >> { >> >> + qga_vss_registry.RemoveSet(SnapshotSetId); >> >> return S_OK; >> >> } >> >> >> >> diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp >> >> index 1b0155c691ef..565eefcfb283 100644 >> >> --- a/qga/vss-win32/requester.cpp >> >> +++ b/qga/vss-win32/requester.cpp >> >> @@ -52,6 +52,7 @@ static t_VssFreeSnapshotProperties >> >> pVssFreeSnapshotProperties; >> >> static struct QGAVSSContext { >> >> IVssBackupComponents *pVssbc; /* VSS requester interface */ >> >> IVssAsync *pAsyncSnapshot; /* async info of VSS snapshot >> >> operation */ >> >> + VSS_ID idSnapshotSet; /* snapshot set created for this >> >> cycle */ >> >> HANDLE hEventFrozen; /* notify fs/writer freeze from >> >> provider */ >> >> HANDLE hEventThaw; /* request provider to thaw */ >> >> HANDLE hEventTimeout; /* notify timeout in provider */ >> >> @@ -116,6 +117,7 @@ static void requester_cleanup(void) >> >> vss_ctx.pVssbc->Release(); >> >> vss_ctx.pVssbc = NULL; >> >> } >> >> + vss_ctx.idSnapshotSet = GUID_NULL; >> >> vss_ctx.cFrozenVols = 0; >> >> qga_debug_end; >> >> } >> >> @@ -446,6 +448,7 @@ void requester_freeze(int *num_vols, void >> >> *mountpoints, ErrorSet *errset) >> >> err_set(errset, hr, "failed to start snapshot set"); >> >> goto out; >> >> } >> >> + vss_ctx.idSnapshotSet = guidSnapshotSet; >> >> >> >> if (mountpoints) { >> >> PWCHAR volume_name_wchar; >> >> @@ -680,6 +683,29 @@ void requester_thaw(int *num_vols, void *mountpints, >> >> ErrorSet *errset) >> >> err_set(errset, hr, "failed to do snapshot set"); >> >> } >> >> >> >> + /* >> >> + * The snapshot set is created in VSS_CTX_APP_ROLLBACK context, so >> >> + * it is persistent: if it was committed, VSS keeps it (and keeps >> >> + * querying providers about it) until it is explicitly deleted. The >> >> + * freeze/thaw cycle is over whatever the outcome above, so delete >> >> + * it now. Failure is not fatal here, only log it. >> >> + */ >> >> + { >> >> + LONG lDeletedSnapshots = 0; >> >> + VSS_ID idNonDeletedSnapshot; >> >> + HRESULT hrDelete; >> >> + >> >> + hrDelete = vss_ctx.pVssbc->DeleteSnapshots(vss_ctx.idSnapshotSet, >> >> + >> >> VSS_OBJECT_SNAPSHOT_SET, >> >> + TRUE, >> >> + &lDeletedSnapshots, >> >> + >> >> &idNonDeletedSnapshot); >> >> + if (FAILED(hrDelete)) { >> >> + qga_debug("failed to delete snapshot set after thaw," >> >> + " hr=0x%08lx", hrDelete); >> >> + } >> >> + } >> >> + >> >> if (err_is_set(errset)) { >> >> vss_ctx.pVssbc->AbortBackup(); >> >> } >> >> -- >> >> 2.56.0.116.g6de20f6092dc >> >> >>
