[PATCH 0/2] Fix wrong copy of encryption 'usage' string
Copying the pointer caused double-free when clearing the domain object. Peter Krempa (2): virSecretLookupDefCopy: Remove return value virStorageEncryptionSecretCopy: Properly copy internals src/util/virsecret.c | 3 +-- src/util/virsecret.h | 4 ++-- src/util/virstorageencryption.c | 8 +++----- src/util/virstoragefile.c | 3 +-- 4 files changed, 7 insertions(+), 11 deletions(-) -- 2.24.1
The function always returns succes so there's no need for a return value. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virsecret.c | 3 +-- src/util/virsecret.h | 4 ++-- src/util/virstoragefile.c | 3 +-- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/util/virsecret.c b/src/util/virsecret.c index f44d964198..54d6bbcb7c 100644 --- a/src/util/virsecret.c +++ b/src/util/virsecret.c @@ -48,7 +48,7 @@ virSecretLookupDefClear(virSecretLookupTypeDefPtr def) } -int +void virSecretLookupDefCopy(virSecretLookupTypeDefPtr dst, const virSecretLookupTypeDef *src) { @@ -58,7 +58,6 @@ virSecretLookupDefCopy(virSecretLookupTypeDefPtr dst, } else if (dst->type == VIR_SECRET_LOOKUP_TYPE_USAGE) { dst->u.usage = g_strdup(src->u.usage); } - return 0; } diff --git a/src/util/virsecret.h b/src/util/virsecret.h index 8c49cfbc89..cfdf2b6e29 100644 --- a/src/util/virsecret.h +++ b/src/util/virsecret.h @@ -48,8 +48,8 @@ struct _virSecretLookupTypeDef { }; void virSecretLookupDefClear(virSecretLookupTypeDefPtr def); -int virSecretLookupDefCopy(virSecretLookupTypeDefPtr dst, - const virSecretLookupTypeDef *src); +void virSecretLookupDefCopy(virSecretLookupTypeDefPtr dst, + const virSecretLookupTypeDef *src); int virSecretLookupParseSecret(xmlNodePtr secretnode, virSecretLookupTypeDefPtr def); void virSecretLookupFormatSecret(virBufferPtr buf, diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 870c40f446..e723cc9410 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1792,8 +1792,7 @@ virStorageAuthDefCopy(const virStorageAuthDef *src) authdef->secrettype = g_strdup(src->secrettype); authdef->authType = src->authType; - if (virSecretLookupDefCopy(&authdef->seclookupdef, &src->seclookupdef) < 0) - return NULL; + virSecretLookupDefCopy(&authdef->seclookupdef, &src->seclookupdef); return g_steal_pointer(&authdef); } -- 2.24.1
virStorageEncryptionSecretPtr may have a string inside it, thus we must copy the string too. Use virSecretLookupDefCopy to do that. Likely caused by 756b46ddd24. https://bugzilla.redhat.com/show_bug.cgi?id=1814923 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstorageencryption.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/util/virstorageencryption.c b/src/util/virstorageencryption.c index 74836d4a00..6765fdc23a 100644 --- a/src/util/virstorageencryption.c +++ b/src/util/virstorageencryption.c @@ -85,12 +85,10 @@ virStorageEncryptionFree(virStorageEncryptionPtr enc) static virStorageEncryptionSecretPtr virStorageEncryptionSecretCopy(const virStorageEncryptionSecret *src) { - virStorageEncryptionSecretPtr ret; - - if (VIR_ALLOC(ret) < 0) - return NULL; + virStorageEncryptionSecretPtr ret = g_new0(virStorageEncryptionSecret, 1); - memcpy(ret, src, sizeof(*src)); + ret->type = src->type; + virSecretLookupDefCopy(&ret->seclookupdef, &src->seclookupdef); return ret; } -- 2.24.1
On a Thursday in 2020, Peter Krempa wrote:
virStorageEncryptionSecretPtr may have a string inside it, thus we must copy the string too. Use virSecretLookupDefCopy to do that.
Likely caused by 756b46ddd24.
Please remove the period from the end. Also, at the times of that commit, memcpy was sufficient: struct _virStorageEncryptionSecret { int type; /* virStorageEncryptionSecretType */ unsigned char uuid[VIR_UUID_BUFLEN]; }; So this actually fixes commit 47e88b33b which switched to using virSecretLookupDef.
https://bugzilla.redhat.com/show_bug.cgi?id=1814923
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstorageencryption.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
On Thu, Mar 19, 2020 at 18:14:15 +0100, Ján Tomko wrote:
On a Thursday in 2020, Peter Krempa wrote:
virStorageEncryptionSecretPtr may have a string inside it, thus we must copy the string too. Use virSecretLookupDefCopy to do that.
Likely caused by 756b46ddd24.
Please remove the period from the end.
Also, at the times of that commit, memcpy was sufficient: struct _virStorageEncryptionSecret { int type; /* virStorageEncryptionSecretType */ unsigned char uuid[VIR_UUID_BUFLEN]; };
So this actually fixes commit 47e88b33b which switched to using virSecretLookupDef.
Oh, okay, I didn't look as closely into the history. I'll update the problematic commit.
On a Thursday in 2020, Peter Krempa wrote:
On Thu, Mar 19, 2020 at 18:14:15 +0100, Ján Tomko wrote:
On a Thursday in 2020, Peter Krempa wrote:
virStorageEncryptionSecretPtr may have a string inside it, thus we must copy the string too. Use virSecretLookupDefCopy to do that.
Likely caused by 756b46ddd24.
Please remove the period from the end.
Also, at the times of that commit, memcpy was sufficient: struct _virStorageEncryptionSecret { int type; /* virStorageEncryptionSecretType */ unsigned char uuid[VIR_UUID_BUFLEN]; };
So this actually fixes commit 47e88b33b which switched to using virSecretLookupDef.
Oh, okay, I didn't look as closely into the history. I'll update the problematic commit.
Well, 756b46ddd24 is also problematic - something like this is easy to overlook. Jano
On a Thursday in 2020, Peter Krempa wrote:
Copying the pointer caused double-free when clearing the domain object.
Peter Krempa (2): virSecretLookupDefCopy: Remove return value virStorageEncryptionSecretCopy: Properly copy internals
src/util/virsecret.c | 3 +-- src/util/virsecret.h | 4 ++-- src/util/virstorageencryption.c | 8 +++----- src/util/virstoragefile.c | 3 +-- 4 files changed, 7 insertions(+), 11 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 19. 3. 2020 17:48, Peter Krempa wrote:
Copying the pointer caused double-free when clearing the domain object.
Peter Krempa (2): virSecretLookupDefCopy: Remove return value virStorageEncryptionSecretCopy: Properly copy internals
src/util/virsecret.c | 3 +-- src/util/virsecret.h | 4 ++-- src/util/virstorageencryption.c | 8 +++----- src/util/virstoragefile.c | 3 +-- 4 files changed, 7 insertions(+), 11 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Ján Tomko -
Michal Prívozník -
Peter Krempa