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

[Daniel P. Berrange]

On Fri, Aug 09, 2013 at 06:51:25AM -0400, John Ferlan wrote:
> 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?)

Yes, that should have been   !*ptr.


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

The virLoginShellAllowedUser() method has absolutely no business free'ing
the 'groups' parameter it is given. That pointer is owned by the caller
who will free it when needed.


I've sent a patch which should fix both these issues.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|