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