Re: [libvirt] [PATCH v3] Avoid a possible NULL pointer dereference in qemuDomainGetTLSObjects

On 09/20/2017 09:11 AM, Erik Skultety wrote: > On Wed, Sep 20, 2017 at 05:32:29AM -0700, Ashish Mittal wrote: >> Passing a NULL value for the argument secAlias to the function >> qemuDomainGetTLSObjects would cause a segmentation fault in >> libvirtd. >> >> Changed code to not dereference a NULL secAlias. >> >> Signed-off-by: Ashish Mittal <ashmit602(a)gmail.com&gt; >> --- >> src/qemu/qemu_hotplug.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c >> index 7dd6e5f..9ecdf0a 100644 >> --- a/src/qemu/qemu_hotplug.c >> +++ b/src/qemu/qemu_hotplug.c >> @@ -1643,7 +1643,8 @@ qemuDomainGetTLSObjects(virQEMUCapsPtr qemuCaps, >> } >> >> if (qemuBuildTLSx509BackendProps(tlsCertdir, tlsListen, tlsVerify, >> - *secAlias, qemuCaps, tlsProps) < 0) >> + secAlias ? *secAlias : NULL, qemuCaps, >> + tlsProps) < 0) >> return -1; >> >> if (!(*tlsAlias = qemuAliasTLSObjFromSrcAlias(srcAlias))) > > A few lines above this context, we check whether we have a valid reference to > @secinfo and if so, we try to fill the @secAlias. Can we guarantee that when > @secinfo is passed, @secAlias has been set as well? I'm asking because I > haven't touched the TLS code yet and I just ran a few argument combinations > mentally and this one got me wondering. If the combination I'm describing is a > pure non-sense, the patch can go straight in. > True, right, ugh. A result of multiple rewrites along the way in patch review resulting in me missing something. In the case of the Veritas series, it won't matter because secinfo will be NULL, but that condition should be if (secinfo && tlsAlias)