[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] Updated patch for virt-login-shell for joing libvirt lxc containers.



On 08/02/2013 11:22 AM, Daniel J Walsh wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> THis patch fixes all of Eric's and Daniels comments.
> 
> [PATCH] virt-login-shell joins users into lxc container.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.14 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
> 
> iEYEARECAAYFAlH7zp8ACgkQrlYvE4MpobNx3gCbBtxw7T4fzIfHSyEEKKyjojXR
> BUUAoOToptiTOi+RC6Bdcp+zvg/xzfRh
> =7zpw
> -----END PGP SIGNATURE-----
> 
> 
> 
The overnight run of Coverity found two issues with this module. I've 
cut-n-pasted the relevant portions of the error/traces:

#1: DEADCODE: virLoginShellAllowedUser()


86   	                if (pp->str[0] == '%') {

(1) Event assignment: 	Assigning: "ptr" = "pp->str + 1".
Also see events: 	[notnull][dead_error_condition][dead_error_line]

87   	                    ptr = &pp->str[1];

(2) Event notnull: 	At condition "ptr", the value of "ptr" cannot be NULL.
(3) Event dead_error_condition: 	The condition "!ptr" cannot be true.
Also see events: 	[assignment][dead_error_line]

88   	                    if (!ptr)

(4) Event dead_error_line: 	Execution cannot reach this statement "continue;".
Also see events: 	[assignment][notnull][dead_error_condition]

89   	                        continue;

Did you perhaps mean (!*ptr) (eg, the contents not the value?)


#2: USE_AFTER_FREE: main()

252  	    if ((ngroups = virGetGroupList(uid, gid, &groups)) < 0)
253  	        goto cleanup;
254  	

(18) Event freed_arg: 	"virLoginShellAllowedUser(virConfPtr, char const *, gid_t *)" frees "groups". [details]
(19) Event cond_false: 	Condition "virLoginShellAllowedUser(conf, name, groups) < 0", taking false branch
Also see events: 	[double_free]

255  	    if (virLoginShellAllowedUser(conf, name, groups) < 0)
256  	        goto cleanup;
257  	


Which is true:

...

61   	
62   	static int virLoginShellAllowedUser(virConfPtr conf,
63   	                                    const char *name,
64   	                                    gid_t *groups)
65   	{
...
(6) Event label: 	Reached label "cleanup"

110  	cleanup:
111  	    VIR_FREE(gname);

(7) Event freed_arg: 	"virFree(void *)" frees parameter "groups". [details]

112  	    VIR_FREE(groups);
113  	    return ret;
114  	}

...

Then back in main()

338  	

(24) Event label: 	Reached label "cleanup"

339  	cleanup:
340  	    virConfFree(conf);
341  	    virLoginShellFini(conn, dom);
342  	    virStringFreeList(shargv);
343  	    VIR_FREE(name);
344  	    VIR_FREE(homedir);
345  	    VIR_FREE(seclabel);
346  	    VIR_FREE(secmodel);

(25) Event double_free: 	Calling "virFree(void *)" frees pointer "groups" which has already been freed. [details]
Also see events: 	[freed_arg]

347  	    VIR_FREE(groups);


Not quite sure how you want to approach fixing the second error.  There are
two VIR_FREE(groups) in virLoginShellAllowedUser() and both would need to 
be handled properly.  Additionally code in main() seems to perhaps use
groups() after the call - so freeing it could have other consequences too.


John


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]