Coverity has found an issue (NEGATIVE_RETURNS)
The 'nfdlist' is the culprit.
> diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c
> index 666facf..75a5223 100644
> --- a/tools/virt-login-shell.c
> +++ b/tools/virt-login-shell.c
> @@ -41,24 +41,8 @@
> #include "vircommand.h"
> #define VIR_FROM_THIS VIR_FROM_NONE
>
> -static ssize_t nfdlist;
> -static int *fdlist;
> static const char *conf_file = SYSCONFDIR "/libvirt/virt-login-shell.conf";
>
> -static void virLoginShellFini(virConnectPtr conn, virDomainPtr dom)
> -{
> - size_t i;
> -
> - for (i = 0; i < nfdlist; i++)
> - VIR_FORCE_CLOSE(fdlist[i]);
> - VIR_FREE(fdlist);
> - nfdlist = 0;
> - if (dom)
> - virDomainFree(dom);
> - if (conn)
> - virConnectClose(conn);
> -}
> -
> static int virLoginShellAllowedUser(virConfPtr conf,
> const char *name,
> gid_t *groups)
> @@ -187,7 +171,6 @@ main(int argc, char **argv)
> pid_t cpid;
> int ret = EXIT_FAILURE;
> int status;
> - int status2;
> uid_t uid = getuid();
> gid_t gid = getgid();
> char *name = NULL;
> @@ -201,6 +184,10 @@ main(int argc, char **argv)
> int longindex = -1;
> int ngroups;
> gid_t *groups = NULL;
> + ssize_t nfdlist = 0;
> + int *fdlist = NULL;
> + int openmax;
> + size_t i;
>
> struct option opt[] = {
> {"help", no_argument, NULL, 'h'},
> @@ -298,6 +285,13 @@ main(int argc, char **argv)
> }
> }
>
> + openmax = sysconf(_SC_OPEN_MAX);
> + if (openmax < 0) {
> + virReportSystemError(errno, "%s",
> + _("sysconf(_SC_OPEN_MAX) failed"));
> + goto cleanup;
> + }
> +
> if ((nfdlist = virDomainLxcOpenNamespace(dom, &fdlist, 0)) < 0)
> goto cleanup;
(36) Event negative_return_fn: Function "virDomainLxcOpenNamespace(dom,
&fdlist, 0U)" returns a negative number. [details]
(37) Event var_assign: Assigning: signed variable "nfdlist" =
"virDomainLxcOpenNamespace".
(38) Event cond_true: Condition "(nfdlist =
virDomainLxcOpenNamespace(dom, &fdlist, 0)) < 0", taking true branch
Also see events: [negative_returns]
> if (VIR_ALLOC(secmodel) < 0)
> @@ -308,76 +302,52 @@ main(int argc, char **argv)
> goto cleanup;
> if (virDomainGetSecurityLabel(dom, seclabel) < 0)
> goto cleanup;
> + if (virSetUIDGID(0, 0, NULL, 0) < 0)
> + goto cleanup;
> + if (virDomainLxcEnterSecurityLabel(secmodel, seclabel, NULL, 0) < 0)
> + goto cleanup;
> + if (nfdlist > 0 &&
> + virDomainLxcEnterNamespace(dom, nfdlist, fdlist, NULL, NULL, 0) < 0)
> + goto cleanup;
> + if (virSetUIDGID(uid, gid, groups, ngroups) < 0)
> + goto cleanup;
> + if (chdir(homedir) < 0) {
> + virReportSystemError(errno, _("Unable to chdir(%s)"), homedir);
> + goto cleanup;
> + }
>
> - /* Fork once because we don't want to affect virt-login-shell's
> - * namespace itself. FIXME: is this necessary?
> - */
> + /* A fork is required to create new process in correct pid namespace. */
> if ((cpid = virFork()) < 0)
> goto cleanup;
>
> if (cpid == 0) {
> - pid_t ccpid;
> -
> - int openmax = sysconf(_SC_OPEN_MAX);
> - int fd;
> + int tmpfd;
>
> - if (virSetUIDGID(0, 0, NULL, 0) < 0)
> - return EXIT_FAILURE;
> -
> - if (virDomainLxcEnterSecurityLabel(secmodel,
> - seclabel,
> - NULL,
> - 0) < 0)
> - return EXIT_FAILURE;
> -
> - if (nfdlist > 0) {
> - if (virDomainLxcEnterNamespace(dom,
> - nfdlist,
> - fdlist,
> - NULL,
> - NULL,
> - 0) < 0)
> - return EXIT_FAILURE;
> - }
> -
> - ret = virSetUIDGID(uid, gid, groups, ngroups);
> - VIR_FREE(groups);
> - if (ret < 0)
> - return EXIT_FAILURE;
> -
> - if (openmax < 0) {
> - virReportSystemError(errno, "%s",
> - _("sysconf(_SC_OPEN_MAX) failed"));
> - return EXIT_FAILURE;
> - }
> - for (fd = 3; fd < openmax; fd++) {
> - int tmpfd = fd;
> + for (i = 3; i < openmax; i++) {
> + tmpfd = i;
> VIR_MASS_CLOSE(tmpfd);
> }
> -
> - /* A fork is required to create new process in correct pid
> - * namespace. */
> - if ((ccpid = virFork()) < 0)
> + if (execv(shargv[0], (char *const*) shargv) < 0) {
> + virReportSystemError(errno, _("Unable to exec shell %s"),
> + shargv[0]);
> return EXIT_FAILURE;
> -
> - if (ccpid == 0) {
> - if (chdir(homedir) < 0) {
> - virReportSystemError(errno, _("Unable to chdir(%s)"), homedir);
> - return EXIT_FAILURE;
> - }
> - if (execv(shargv[0], (char *const*) shargv) < 0) {
> - virReportSystemError(errno, _("Unable to exec shell %s"),
> - shargv[0]);
> - return EXIT_FAILURE;
> - }
> }
> - return virProcessWait(ccpid, &status2, true);
> + return EXIT_SUCCESS;
> }
> - ret = virProcessWait(cpid, &status, true);
> +
> + if (virProcessWait(cpid, &status, true) < 0)
> + goto cleanup;
> + ret = EXIT_SUCCESS;
>
> cleanup:
> + for (i = 0; i < nfdlist; i++)
> + VIR_FORCE_CLOSE(fdlist[i]);
(41) Event negative_returns: Using unsigned variable "nfdlist" in a
loop exit condition.
Also see events: [negative_return_fn][var_assign]
> + VIR_FREE(fdlist);
> virConfFree(conf);
> - virLoginShellFini(conn, dom);
> + if (dom)
> + virDomainFree(dom);
> + if (conn)
> + virConnectClose(conn);
> virStringFreeList(shargv);
> VIR_FREE(name);
> VIR_FREE(homedir);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list