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