Re: [libvirt] [PATCH 01/11] tlscontext: Make sure to get proper pointer to name

On 01/31/2013 03:44 AM, Osier Yang wrote: > On 2013年01月31日 03:36, John Ferlan wrote: >> The 'dname' string was only filled in within the loop when available; >> however, the TRACE macros used it unconditionally and caused Coverity >> to compain about BAD_SIZEOF. Using a dnameptr keeps Coverity at bay and s/compain/complain/ >> makes sure dname was properly filled before attempting the TRACE message. >> --- >> src/rpc/virnettlscontext.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> @@ -950,6 +950,7 @@ static int >> virNetTLSContextValidCertificate(virNetTLSContextPtr ctxt, >> unsigned int nCerts, i; >> char dname[256]; >> size_t dnamesize = sizeof(dname); >> + char *dnameptr = NULL; Would it be any simpler to just 0-initialize dname, as in: char dname[256] = ""; >> >> PROBE(RPC_TLS_CONTEXT_SESSION_ALLOW, >> "ctxt=%p sess=%p dname=%s", >> - ctxt, sess, dname); At which point, the PROBE(..., dname) would be guaranteed to have a NUL terminator within range? If I understand it, Coverity is complaining that if dname is uninitialized, then the PROBE() may read beyond 256 bytes while looking for the end of a string. > > I guess dname[0] is guaranteed to be not nul as long as > gnutls_x509_crt_get_dn succeeded. Not unless we pre-initialize dname[0].

> > If so, the patch can be simplified as: > > dname[0] ? dname : "(unknown)" Using a conditional would make the difference between a probe stating 'dname=' vs. 'dname=(unknown)'; I don't think it adds that much to need a ternary ?: in the PROBE.