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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
---
 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

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux