[libvirt] [PATCH 3/3] virISCSIGetSession: Don't leak memory

[John Ferlan]

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 at 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
>>