On Tue, Jul 08, 2008 at 05:51:05PM +0400, Evgeniy Sokolov wrote:

+int +openvzReadConfigParam(int vpsid ,const char * param, char *value, int maxlen) +{ + char conf_file[PATH_MAX] ; + char line[PATH_MAX] ; + int ret, found = 0; + char * conf_dir; + int fd ; + char * sf, * token; + char *saveptr = NULL; + + + conf_dir = openvzLocateConfDir(); + if (conf_dir == NULL) + return -1; + + sprintf(conf_file,"%s/%d.conf",conf_dir,vpsid);

Please use snprintf & check the return value, even if you think it'll never overflow PATH_MAX. Or even use asprintf().

+ VIR_FREE(conf_dir); + + value[0] = 0; + + fd = open(conf_file, O_RDWR);

You're only reading the config use O_RDONLY. THe O_RDWR will generate an abort() under FORTIFY_SOURCE too, because you're not supplying the 3rd mode arg which is mandatory when opening for write access.

+ if (fd == -1) + return -1; + + while(1) { + ret = openvz_readline(fd, line, sizeof(line)); + if(ret <= 0) + break; + saveptr = NULL; + if (STREQLEN(line, param, strlen(param))) { + sf = line; + sf += strlen(param); + if (sf[0] == '=' && (token = strtok_r(sf,"\"\t=\n", &saveptr)) != NULL) { + strncpy(value, token, maxlen) ;

Potentially non-terminated string there - if there is no null byte among the first maxlen bytes of token, the string placed in value will not be null terminated.

+openvzDomainSetAutostart(virDomainPtr dom, int autostart) +{ + char cmdbuf[CMDBUF_LEN], *cmdExec[OPENVZ_MAX_ARG]; + int ret, pid, outfd, errfd; + virConnectPtr conn= dom->conn; + struct openvz_driver *driver = (struct openvz_driver *) conn->privateData; + struct openvz_vm *vm = openvzFindVMByUUID(driver, dom->uuid); + + if (!vm) { + error(conn, VIR_ERR_INVALID_DOMAIN, _("no domain with matching uuid")); + return -1; + } + + snprintf(cmdbuf, CMDBUF_LEN - 1, VZCTL " set %s --onboot %s --save", vm->vmdef->name, + autostart ? "yes" : "no"); + + if((ret = convCmdbufExec(cmdbuf, cmdExec)) == -1) + { + openvzLog(OPENVZ_ERR, "%s", _("Error in parsing Options to OPENVZ")); + goto bail_out5; + } + ret = virExec(conn, (char **)cmdExec, &pid, -1, &outfd, &errfd);

I realize you are just following the existing pattern used in OpenVZ driver, but this piece of code is horrible. sprintf'ing into a string, then parsing that string and turning it back into a list of argv[] strings, with no escaping of special characters, or quoting. eg if the vm name had a space in it it'll mis-parse it. Just declare the command argv straight into a char*[], eg const char *prog[] = { VZCTL, "set", vm->vmdef->name, "--onboot", autostart ? "yes" : "no", "--save" } ret = virExec(conn, prog, &pid, -1, &outfd, &errfd) See the storage_backend_logical.c file for examples of this kind of approach We should put other uses of convCmdbufExec() on the TODO list for removal in the future.

+static int +openvzDomainGetAutostart(virDomainPtr dom, int *autostart) +{ + virConnectPtr conn= dom->conn; + struct openvz_driver *driver = (struct openvz_driver *) conn->privateData; + struct openvz_vm *vm = openvzFindVMByUUID(driver, dom->uuid); + char value[1024]; + + if (!vm) { + error(conn, VIR_ERR_INVALID_DOMAIN, _("no domain with matching uuid")); + return -1; + } + + if (openvzReadConfigParam(vm->vpsid , "ONBOOT", value, sizeof(value)) < 0) { + openvzLog(OPENVZ_ERR, "%s", _("Cound not read container config"));

This should raise a VIR_ERR_INTERNAL_ERROR otherwise the details are never seen by the calling app. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|