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