3:50 a.m.
Mostly mem leaks. However, the first one is an actual crasher.
Michal Privoznik (3):
virStorageSourceClear: Don't leave dangling pointers behind
virNetworkObjDispose: Don't leak virMacMap object
virISCSIGetSession: Don't leak memory
src/conf/network_conf.c | 1 +
src/util/viriscsi.c | 3 ++-
src/util/virstoragefile.c | 2 ++
3 files changed, 5 insertions(+), 1 deletion(-)
--
2.10.2
3:50 a.m.
New subject: [libvirt] [PATCH 1/3] virStorageSourceClear: Don't leave dangling pointers behind
Imagine that this function is called twice over the same disk
source. While in the first run all allocated memory is freed, not
all pointers are set to NULL (e.g. def->srcpool). So when called
again, these poitners are freed again resulting in double free.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virstoragefile.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 22cdb83..57a298f 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2238,6 +2238,8 @@ virStorageSourceClear(virStorageSourcePtr def)
VIR_FREE(def->nodeformat);
virStorageSourceBackingStoreClear(def);
+
+ memset(def, 0, sizeof(*def));
}
--
2.10.2
3:50 a.m.
New subject: [libvirt] [PATCH 2/3] virNetworkObjDispose: Don't leak virMacMap object
Even though the virMacMap object is not necessarily created at
the same time as the network object, the former makes no sense
without the latter and thus should be unref'd in the network
object dispose function.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/network_conf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 48e0001..dc15fc9 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -443,6 +443,7 @@ virNetworkObjDispose(void *obj)
virNetworkDefFree(net->def);
virNetworkDefFree(net->newDef);
virBitmapFree(net->class_id);
+ virObjectUnref(net->macmap);
}
static void
--
2.10.2
3:50 a.m.
New subject: [libvirt] [PATCH 3/3] virISCSIGetSession: Don't leak memory
This function runs an iscsi command and parses its output.
However, due to the nature of things, virISCSIExtractSession()
callback can be called multiple times. In each run it would
allocate new memory and overwrite the variable where we keep
pointer to it and thus leaking old allocations.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/viriscsi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 504ffbd..9c6fde0 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -52,7 +52,8 @@ virISCSIExtractSession(char **const groups,
{
struct virISCSISessionData *data = opaque;
- if (STREQ(groups[1], data->devpath))
+ if (!data->devpath &&
+ STREQ(groups[1], data->devpath))
return VIR_STRDUP(data->session, groups[0]);
return 0;
}
--
2.10.2
2:11 p.m.
New subject: [libvirt] [PATCH 3/3] virISCSIGetSession: Don't leak memory
On 04/05/2017 04:50 AM, Michal Privoznik wrote:
This function runs an iscsi command and parses its output.
However, due to the nature of things, virISCSIExtractSession()
callback can be called multiple times. In each run it would
allocate new memory and overwrite the variable where we keep
pointer to it and thus leaking old allocations.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/viriscsi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 504ffbd..9c6fde0 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -52,7 +52,8 @@ virISCSIExtractSession(char **const groups,
{
struct virISCSISessionData *data = opaque;
- if (STREQ(groups[1], data->devpath))
+ if (!data->devpath &&
+ STREQ(groups[1], data->devpath))
return VIR_STRDUP(data->session, groups[0]);
return 0;
}
I see you fixed your typo "!data->devpath" to "!data->session"
before
pushing, but the reality is this is a no-op considering at most we can
only match once, but because virCommandRunRegex only bails if the return
from "*func" is "< 0", we just have to continue through the loop.
Not
that it should, but for the purpose of this callback it could.
In any case, we're guaranteed that the "groups[1]" will always be
unique, so all the code does is scan the output looking for the matching
session # and then copy that into data->session. Each session must have
a unique string as that the IQN which must be unique.
As an aside, data->session could change to an int, but that would
require some other changes as well.
John
3:16 p.m.
New subject: [libvirt] [PATCH 3/3] virISCSIGetSession: Don't leak memory
On Wed, Apr 05, 2017 at 03:11:52PM -0400, John Ferlan wrote:
On 04/05/2017 04:50 AM, Michal Privoznik wrote:
> This function runs an iscsi command and parses its output.
> However, due to the nature of things, virISCSIExtractSession()
> callback can be called multiple times. In each run it would
> allocate new memory and overwrite the variable where we keep
> pointer to it and thus leaking old allocations.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/util/viriscsi.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
> index 504ffbd..9c6fde0 100644
> --- a/src/util/viriscsi.c
> +++ b/src/util/viriscsi.c
> @@ -52,7 +52,8 @@ virISCSIExtractSession(char **const groups,
> {
> struct virISCSISessionData *data = opaque;
>
> - if (STREQ(groups[1], data->devpath))
> + if (!data->devpath &&
> + STREQ(groups[1], data->devpath))
> return VIR_STRDUP(data->session, groups[0]);
> return 0;
> }
>
I see you fixed your typo "!data->devpath" to "!data->session"
before
I wonder how I missed that, maybe because this wasn't the first time I
saw the patch O:-)
pushing, but the reality is this is a no-op considering at most we
can
only match once, but because virCommandRunRegex only bails if the return
It is not no-op. The function is ran on every line of output and, as
we've noticed on Michal's machine, he got 2 lines with the same
group[1], so to speak.
from "*func" is "< 0", we just have to continue
through the loop. Not
that it should, but for the purpose of this callback it could.
well, virCommandRunRegex() ends when func() returns negative value, but
it ends as an error. And we wouldn't be able to differentiate between
real error and "func() already found what it needed". We would have to
change virCommandRunRegex() to work with yet another return value and
behave based on that.
In any case, we're guaranteed that the "groups[1]" will
always be
unique, so all the code does is scan the output looking for the matching
session # and then copy that into data->session. Each session must have
a unique string as that the IQN which must be unique.
Then there is an error on Michal's machine then. In any case, it won't
hurt if we don't depend on external tools behaving as predicted. It's
the same thing as segfaulting, we shouldn't segfault on any external
input, no matter how wrong it is.
As an aside, data->session could change to an int, but that would
require some other changes as well.
John
3:34 p.m.
New subject: [libvirt] [PATCH 3/3] virISCSIGetSession: Don't leak memory
On 04/05/2017 04:16 PM, Martin Kletzander wrote:
On Wed, Apr 05, 2017 at 03:11:52PM -0400, John Ferlan wrote:
>
>
> On 04/05/2017 04:50 AM, Michal Privoznik wrote:
>> This function runs an iscsi command and parses its output.
>> However, due to the nature of things, virISCSIExtractSession()
>> callback can be called multiple times. In each run it would
>> allocate new memory and overwrite the variable where we keep
>> pointer to it and thus leaking old allocations.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> src/util/viriscsi.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
>> index 504ffbd..9c6fde0 100644
>> --- a/src/util/viriscsi.c
>> +++ b/src/util/viriscsi.c
>> @@ -52,7 +52,8 @@ virISCSIExtractSession(char **const groups,
>> {
>> struct virISCSISessionData *data = opaque;
>>
>> - if (STREQ(groups[1], data->devpath))
>> + if (!data->devpath &&
>> + STREQ(groups[1], data->devpath))
>> return VIR_STRDUP(data->session, groups[0]);
>> return 0;
>> }
>>
>
> I see you fixed your typo "!data->devpath" to
"!data->session" before
I wonder how I missed that, maybe because this wasn't the first time I
saw the patch O:-)
> pushing, but the reality is this is a no-op considering at most we can
> only match once, but because virCommandRunRegex only bails if the return
It is not no-op. The function is ran on every line of output and, as
we've noticed on Michal's machine, he got 2 lines with the same
group[1], so to speak.
So what's the output from a "iscsiadm --mode session" on Michal's
machine? And then if there is a duplicate - how did it happen?
For my iSCSI config on Fedora, the /etc/tgt/targets.conf is what is used
to set up those IQN's... After that it's a lot of iscsid/tgtd magic in
order to allow the iscsiadm "search" commands work.
> from "*func" is "< 0", we just have to
continue through the loop. Not
> that it should, but for the purpose of this callback it could.
>
well, virCommandRunRegex() ends when func() returns negative value, but
it ends as an error. And we wouldn't be able to differentiate between
real error and "func() already found what it needed". We would have to
change virCommandRunRegex() to work with yet another return value and
behave based on that.
Well logic could be added to checking of the return vs. 0 or 1; where 0
is keep going and 1 is stop processing we found what we wanted. I tend
to try and not think to hard about that regex code - in fact the more I
can forget the better off I am ;-)
John
> In any case, we're guaranteed that the "groups[1]"
will always be
> unique, so all the code does is scan the output looking for the matching
> session # and then copy that into data->session. Each session must have
> a unique string as that the IQN which must be unique.
>
Then there is an error on Michal's machine then. In any case, it won't
hurt if we don't depend on external tools behaving as predicted. It's
the same thing as segfaulting, we shouldn't segfault on any external
input, no matter how wrong it is.
> As an aside, data->session could change to an int, but that would
> require some other changes as well.
>
> John
>
12:21 a.m.
New subject: [libvirt] [PATCH 3/3] virISCSIGetSession: Don't leak memory
On 04/05/2017 10:34 PM, John Ferlan wrote:
On 04/05/2017 04:16 PM, Martin Kletzander wrote:
> On Wed, Apr 05, 2017 at 03:11:52PM -0400, John Ferlan wrote:
>>
>>
>> On 04/05/2017 04:50 AM, Michal Privoznik wrote:
>>> This function runs an iscsi command and parses its output.
>>> However, due to the nature of things, virISCSIExtractSession()
>>> callback can be called multiple times. In each run it would
>>> allocate new memory and overwrite the variable where we keep
>>> pointer to it and thus leaking old allocations.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>>> ---
>>> src/util/viriscsi.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
>>> index 504ffbd..9c6fde0 100644
>>> --- a/src/util/viriscsi.c
>>> +++ b/src/util/viriscsi.c
>>> @@ -52,7 +52,8 @@ virISCSIExtractSession(char **const groups,
>>> {
>>> struct virISCSISessionData *data = opaque;
>>>
>>> - if (STREQ(groups[1], data->devpath))
>>> + if (!data->devpath &&
>>> + STREQ(groups[1], data->devpath))
>>> return VIR_STRDUP(data->session, groups[0]);
>>> return 0;
>>> }
>>>
>>
>> I see you fixed your typo "!data->devpath" to
"!data->session" before
>
> I wonder how I missed that, maybe because this wasn't the first time I
> saw the patch O:-)
>
>> pushing, but the reality is this is a no-op considering at most we can
>> only match once, but because virCommandRunRegex only bails if the return
>
> It is not no-op. The function is ran on every line of output and, as
> we've noticed on Michal's machine, he got 2 lines with the same
> group[1], so to speak.
>
So what's the output from a "iscsiadm --mode session" on Michal's
machine? And then if there is a duplicate - how did it happen?
# iscsiadm --mode session
tcp: [31] 10.34.126.253:3260,1 iqn.2017-03.com.mprivozn:server
tcp: [32] virt-iscsi-server.usersys.redhat.com:3260,1 iqn.2017-03.com.mprivozn:server
where the URL from "line" [32] resolves to the IP from "line" [31].
Michal
6:05 a.m.
New subject: [libvirt] [PATCH 3/3] virISCSIGetSession: Don't leak memory
>
> So what's the output from a "iscsiadm --mode session" on Michal's
> machine? And then if there is a duplicate - how did it happen?
# iscsiadm --mode session
tcp: [31] 10.34.126.253:3260,1 iqn.2017-03.com.mprivozn:server
tcp: [32] virt-iscsi-server.usersys.redhat.com:3260,1 iqn.2017-03.com.mprivozn:server
where the URL from "line" [32] resolves to the IP from "line" [31].
Ah - I see - so some sort of intentional (mis)configuration to avoid the
text comparison because the IP address isn't being resolved... One would
think that would/could be an iscsi(d) type bug.
Does 'iscsiadm -m session -P 3' consider them different?
Can you login to each?
iscsiadm -m node -T $IQN -p $PORTAL --login
where $PORTAL would be in your case 10.34.126.253:3260 or
virt-iscsi-server.usersys.redhat.com:3260.
What about:
iscsiadm -m discovery -t sendtargets -p $PORTAL -P1
or
Wonder how other tools react... "lsscsi -t -g" or "iscsi-ls ...". Do
they see "two" devices which in the end are the same thing? That's kind
of (well) scary.
John
6:09 a.m.
On Wed, Apr 05, 2017 at 10:50:52AM +0200, Michal Privoznik wrote:
Mostly mem leaks. However, the first one is an actual crasher.
Michal Privoznik (3):
virStorageSourceClear: Don't leave dangling pointers behind
virNetworkObjDispose: Don't leak virMacMap object
virISCSIGetSession: Don't leak memory
ACK series. I, honestly, have no idea whether we need some specific
line from the iscsiadm output, so for now the first one will have to be
enough.
src/conf/network_conf.c | 1 +
src/util/viriscsi.c | 3 ++-
src/util/virstoragefile.c | 2 ++
3 files changed, 5 insertions(+), 1 deletion(-)
--
2.10.2
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
2786
days inactive
2787
days old
9 comments
3 participants
participants (3)
-
John Ferlan -
Martin Kletzander -
Michal Privoznik