<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 21, 2017 at 12:21 AM, Erik Skultety <span dir="ltr"><<a href="mailto:eskultet@redhat.com" target="_blank">eskultet@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">On Wed, Sep 20, 2017 at 12:19:16PM -0700, ashish mittal wrote:<br>
> On Wed, Sep 20, 2017 at 6:11 AM, Erik Skultety <<a href="mailto:eskultet@redhat.com">eskultet@redhat.com</a>> wrote:<br>
><br>
> > On Wed, Sep 20, 2017 at 05:32:29AM -0700, Ashish Mittal wrote:<br>
> > > Passing a NULL value for the argument secAlias to the function<br>
> > > qemuDomainGetTLSObjects would cause a segmentation fault in<br>
> > > libvirtd.<br>
> > ><br>
> > > Changed code to not dereference a NULL secAlias.<br>
> > ><br>
> > > Signed-off-by: Ashish Mittal <<a href="mailto:ashmit602@gmail.com">ashmit602@gmail.com</a>><br>
> > > ---<br>
> > > src/qemu/qemu_hotplug.c | 3 ++-<br>
> > > 1 file changed, 2 insertions(+), 1 deletion(-)<br>
> > ><br>
> > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c<br>
> > > index 7dd6e5f..9ecdf0a 100644<br>
> > > --- a/src/qemu/qemu_hotplug.c<br>
> > > +++ b/src/qemu/qemu_hotplug.c<br>
> > > @@ -1643,7 +1643,8 @@ qemuDomainGetTLSObjects(<wbr>virQEMUCapsPtr qemuCaps,<br>
> > > }<br>
> > ><br>
> > > if (qemuBuildTLSx509BackendProps(<wbr>tlsCertdir, tlsListen, tlsVerify,<br>
> > > - *secAlias, qemuCaps, tlsProps) < 0)<br>
> > > + secAlias ? *secAlias : NULL,<br>
> > qemuCaps,<br>
> > > + tlsProps) < 0)<br>
> > > return -1;<br>
> > ><br>
> > > if (!(*tlsAlias = qemuAliasTLSObjFromSrcAlias(<wbr>srcAlias)))<br>
> ><br>
> > A few lines above this context, we check whether we have a valid reference<br>
> > to<br>
> > @secinfo and if so, we try to fill the @secAlias. Can we guarantee that<br>
> > when<br>
> > @secinfo is passed, @secAlias has been set as well?<br>
><br>
><br>
> When secinfo is passed, the line marked below should guarantee that<br>
> secAlias is set to not-null.<br>
><br>
> if (secinfo) {<br>
> if (qemuBuildSecretInfoProps(<wbr>secinfo, secProps) < 0)<br>
> return -1;<br>
><br>
> if (!(*secAlias = qemuDomainGetSecretAESAlias(<wbr>srcAlias, false)))<br>
> <=== this should ensure secAlias != NULL or return -1<br>
> return -1;<br>
> }<br>
><br>
<br>
</div></div>See John's reply to my response, long story short, in case of Veritas, this<br>
couldn't happen, but in general, we should cover the use case I'm describing as<br>
well, which is just a matter of very simple 'if' statement adjustment.<br>
<span class="gmail-HOEnZb"><font color="#888888"><br>
Erik<br>
</font></span></blockquote></div><br></div><div class="gmail_extra"><div class="gmail_extra"> if (secinfo) {</div><div class="gmail_extra"> if (qemuBuildSecretInfoProps(secinfo, secProps) < 0)</div><div class="gmail_extra"> return -1;</div><div class="gmail_extra"><br></div><div class="gmail_extra"> if (!(*secAlias = qemuDomainGetSecretAESAlias(srcAlias, false)))</div><div class="gmail_extra"> return -1;</div><div class="gmail_extra"> }</div><div class="gmail_extra"><br></div><div class="gmail_extra">Above code segment suggests that if secinfo != NULL, then secAlias should never be NULL . If that were not the case, we would have already seen a crash from such a case.</div><div class="gmail_extra"><br></div></div><div class="gmail_extra">I'm afraid changing the above "if" condition to "if (secinfo && secAlias)" is changing the behavior of the code to say "It is OK if secinfo != NULL and secAlias == NULL, I'll just skip the setting of *secAlias". I don't know the code well enough to say if this, or the above, is expected behavior. If the expected behavior is that secAlias should never be NULL when secinfo != NULL, then a safer change might be -</div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra"> if (secinfo) {</div><div class="gmail_extra"> if (!secAlias)</div><div class="gmail_extra"> return -1;</div><div class="gmail_extra"> ....</div><div class="gmail_extra"><br></div><div class="gmail_extra">Regards,</div><div class="gmail_extra">Ashish</div></div></div>