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.