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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
