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

[John Ferlan]

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.
>