On 01/31/2013 11:41 AM, Eric Blake wrote: > 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/ > fixed this... fingers don't always type what the brain tells them to :-) >>> 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] = ""; > > As Osier points out there is a memset(dname, 0, dnamesize) in the code Changing the code to use the above still results in Coverity complaint for each PROBE: 1062 (1) Event bad_sizeof: Taking the size of "dname", which is the address of an object, is suspicious. Did you intend the size of the object itself? 1063 PROBE(RPC_TLS_CONTEXT_SESSION_ALLOW, 1064 "ctxt=%p sess=%p dname=%s", 1065 ctxt, sess, dname); This is the only PROBE I found using a stack buffer of a specific size. If I changed the code to VIR_ALLOC_N(dname, dnamesize);, then the message goes away too. I believe the 256 was chosen since that the max length of a distinguished name field from what I've read >>> >>> 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. > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list