| Message ID | CAPtdW16hg7JCiXK9pA5nc=VV6cHaJr6mPUFrs0BN7fAX-fXf4Q@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 09/30/2015 11:32 PM, Harmandeep Kaur wrote: > Convert malloc()/calloc() calls to g_malloc()/g_try_malloc()/g_new0() > in linux-user/syscall.c file This part is fine, > > v1->v2 convert the free() call in host_to_target_semarray() > to g_free() and calls g_try_malloc(count) instead of > g_try_malloc(sizeof(count)) but this part belongs... > > Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com> > --- ...here, after the --- separator. It is useful information to mail reviewers, but worthless in the qemu.git history (a year from now, we won't care how many versions it went through on the list, only the version that got checked in). > +++ b/linux-user/syscall.c > @@ -1554,12 +1554,7 @@ set_timeout: > } > > fprog.len = tswap16(tfprog->len); > - filter = malloc(fprog.len * sizeof(*filter)); > - if (filter == NULL) { > - unlock_user_struct(tfilter, tfprog->filter, 1); > - unlock_user_struct(tfprog, optval_addr, 1); > - return -TARGET_ENOMEM; > - } > + filter = g_malloc(fprog.len * sizeof(*filter)); This should use g_new(TYPE, fprog.len) to avoid overflow issues.
Thank you Eric for guiding me in the right direction. I look forward to implement this correctly. On Thu, Oct 1, 2015 at 9:27 PM, Eric Blake <eblake@redhat.com> wrote: > On 09/30/2015 11:32 PM, Harmandeep Kaur wrote: > > Convert malloc()/calloc() calls to g_malloc()/g_try_malloc()/g_new0() > > in linux-user/syscall.c file > > This part is fine, > > > > > v1->v2 convert the free() call in host_to_target_semarray() > > to g_free() and calls g_try_malloc(count) instead of > > g_try_malloc(sizeof(count)) > > but this part belongs... > > > > > Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com> > > --- > > ...here, after the --- separator. It is useful information to mail > reviewers, but worthless in the qemu.git history (a year from now, we > won't care how many versions it went through on the list, only the > version that got checked in). > > > > +++ b/linux-user/syscall.c > > @@ -1554,12 +1554,7 @@ set_timeout: > > } > > > > fprog.len = tswap16(tfprog->len); > > - filter = malloc(fprog.len * sizeof(*filter)); > > - if (filter == NULL) { > > - unlock_user_struct(tfilter, tfprog->filter, 1); > > - unlock_user_struct(tfprog, optval_addr, 1); > > - return -TARGET_ENOMEM; > > - } > > + filter = g_malloc(fprog.len * sizeof(*filter)); > > This should use g_new(TYPE, fprog.len) to avoid overflow issues. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > >
On 1 October 2015 at 06:32, Harmandeep Kaur <write.harmandeep@gmail.com> wrote: > Convert malloc()/calloc() calls to g_malloc()/g_try_malloc()/g_new0() > in linux-user/syscall.c file > > v1->v2 convert the free() call in host_to_target_semarray() > to g_free() and calls g_try_malloc(count) instead of > g_try_malloc(sizeof(count)) > > Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com> > --- > linux-user/syscall.c | 38 +++++++++++--------------------------- > 1 file changed, 11 insertions(+), 27 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index d1d3eb2..c79e862 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -1554,12 +1554,7 @@ set_timeout: > } > > fprog.len = tswap16(tfprog->len); > - filter = malloc(fprog.len * sizeof(*filter)); > - if (filter == NULL) { > - unlock_user_struct(tfilter, tfprog->filter, 1); > - unlock_user_struct(tfprog, optval_addr, 1); > - return -TARGET_ENOMEM; > - } > > + filter = g_malloc(fprog.len * sizeof(*filter)); fprog.len comes from the guest -- you can't use g_malloc. > for (i = 0; i < fprog.len; i++) { > filter[i].code = tswap16(tfilter[i].code); > filter[i].jt = tfilter[i].jt; > @@ -1570,7 +1565,7 @@ set_timeout: > > ret = get_errno(setsockopt(sockfd, SOL_SOCKET, > SO_ATTACH_FILTER, &fprog, sizeof(fprog))); > - free(filter); > + g_free(filter); > > unlock_user_struct(tfilter, tfprog->filter, 1); > unlock_user_struct(tfprog, optval_addr, 1); > @@ -1881,11 +1876,7 @@ static struct iovec *lock_iovec(int type, abi_ulong > target_addr, > return NULL; > } > > - vec = calloc(count, sizeof(struct iovec)); > - if (vec == NULL) { > - errno = ENOMEM; > - return NULL; > - } > + vec = g_new0(struct iovec, count); count comes from the guest -- you can't use g_new0. > > target_vec = lock_user(VERIFY_READ, target_addr, > count * sizeof(struct target_iovec), 1); > @@ -1945,7 +1936,7 @@ static struct iovec *lock_iovec(int type, abi_ulong > target_addr, > } > unlock_user(target_vec, target_addr, 0); > fail2: > - free(vec); > + g_free(vec); > errno = err; > return NULL; > } > @@ -2672,14 +2663,11 @@ static inline abi_long target_to_host_semarray(int > semid, unsigned short **host_ > > nsems = semid_ds.sem_nsems; > > - *host_array = malloc(nsems*sizeof(unsigned short)); > - if (!*host_array) { > - return -TARGET_ENOMEM; > - } > + *host_array = g_malloc(nsems*sizeof(unsigned short)); > array = lock_user(VERIFY_READ, target_addr, > nsems*sizeof(unsigned short), 1); I don't think we can guarantee that nsems is small -- you can't use g_malloc. > if (!array) { > - free(*host_array); > + g_free(*host_array); > return -TARGET_EFAULT; > } > > @@ -2716,7 +2704,7 @@ static inline abi_long host_to_target_semarray(int > semid, abi_ulong target_addr, > for(i=0; i<nsems; i++) { > __put_user((*host_array)[i], &array[i]); > } > - free(*host_array); > + g_free(*host_array); > unlock_user(array, target_addr, 1); > > return 0; > @@ -2975,15 +2963,11 @@ static inline abi_long do_msgsnd(int msqid, abi_long > msgp, > > if (!lock_user_struct(VERIFY_READ, target_mb, msgp, 0)) > return -TARGET_EFAULT; > - host_mb = malloc(msgsz+sizeof(long)); > - if (!host_mb) { > - unlock_user_struct(target_mb, msgp, 0); > - return -TARGET_ENOMEM; > - } > + host_mb = g_malloc(msgsz+sizeof(long)); msgsz comes from the guest -- you can't use g_malloc. > host_mb->mtype = (abi_long) tswapal(target_mb->mtype); > memcpy(host_mb->mtext, target_mb->mtext, msgsz); > ret = get_errno(msgsnd(msqid, host_mb, msgsz, msgflg)); > - free(host_mb); > + g_free(host_mb); > unlock_user_struct(target_mb, msgp, 0); > > return ret; > @@ -7625,7 +7609,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long > arg1, > struct linux_dirent *dirp; > abi_long count = arg3; > > - dirp = malloc(count); > + dirp = g_try_malloc(count); > if (!dirp) { > ret = -TARGET_ENOMEM; > goto fail; > @@ -7662,7 +7646,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long > arg1, > ret = count1; > unlock_user(target_dirp, arg2, ret); > } > - free(dirp); > + g_free(dirp); > } > #else > { > -- > 1.9.1 thanks -- PMM
Hi Peter, Can you please tell me if I should use other glib functions, or implement it in any other way? I am newbie here :) Thank you for reviewing my patch. On Thu, Oct 1, 2015 at 9:33 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 1 October 2015 at 06:32, Harmandeep Kaur <write.harmandeep@gmail.com> > wrote: > > Convert malloc()/calloc() calls to g_malloc()/g_try_malloc()/g_new0() > > in linux-user/syscall.c file > > > > v1->v2 convert the free() call in host_to_target_semarray() > > to g_free() and calls g_try_malloc(count) instead of > > g_try_malloc(sizeof(count)) > > > > Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com> > > --- > > linux-user/syscall.c | 38 +++++++++++--------------------------- > > 1 file changed, 11 insertions(+), 27 deletions(-) > > > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > > index d1d3eb2..c79e862 100644 > > --- a/linux-user/syscall.c > > +++ b/linux-user/syscall.c > > @@ -1554,12 +1554,7 @@ set_timeout: > > } > > > > fprog.len = tswap16(tfprog->len); > > - filter = malloc(fprog.len * sizeof(*filter)); > > - if (filter == NULL) { > > - unlock_user_struct(tfilter, tfprog->filter, 1); > > - unlock_user_struct(tfprog, optval_addr, 1); > > - return -TARGET_ENOMEM; > > - } > > > > + filter = g_malloc(fprog.len * sizeof(*filter)); > > fprog.len comes from the guest -- you can't use g_malloc. > > > for (i = 0; i < fprog.len; i++) { > > filter[i].code = tswap16(tfilter[i].code); > > filter[i].jt = tfilter[i].jt; > > @@ -1570,7 +1565,7 @@ set_timeout: > > > > ret = get_errno(setsockopt(sockfd, SOL_SOCKET, > > SO_ATTACH_FILTER, &fprog, > sizeof(fprog))); > > - free(filter); > > + g_free(filter); > > > > unlock_user_struct(tfilter, tfprog->filter, 1); > > unlock_user_struct(tfprog, optval_addr, 1); > > @@ -1881,11 +1876,7 @@ static struct iovec *lock_iovec(int type, > abi_ulong > > target_addr, > > return NULL; > > } > > > > - vec = calloc(count, sizeof(struct iovec)); > > - if (vec == NULL) { > > - errno = ENOMEM; > > - return NULL; > > - } > > + vec = g_new0(struct iovec, count); > > count comes from the guest -- you can't use g_new0. > > > > > target_vec = lock_user(VERIFY_READ, target_addr, > > count * sizeof(struct target_iovec), 1); > > @@ -1945,7 +1936,7 @@ static struct iovec *lock_iovec(int type, abi_ulong > > target_addr, > > } > > unlock_user(target_vec, target_addr, 0); > > fail2: > > - free(vec); > > + g_free(vec); > > errno = err; > > return NULL; > > } > > @@ -2672,14 +2663,11 @@ static inline abi_long > target_to_host_semarray(int > > semid, unsigned short **host_ > > > > nsems = semid_ds.sem_nsems; > > > > - *host_array = malloc(nsems*sizeof(unsigned short)); > > - if (!*host_array) { > > - return -TARGET_ENOMEM; > > - } > > + *host_array = g_malloc(nsems*sizeof(unsigned short)); > > array = lock_user(VERIFY_READ, target_addr, > > nsems*sizeof(unsigned short), 1); > > I don't think we can guarantee that nsems is small -- you can't > use g_malloc. > > > if (!array) { > > - free(*host_array); > > + g_free(*host_array); > > return -TARGET_EFAULT; > > } > > > > @@ -2716,7 +2704,7 @@ static inline abi_long host_to_target_semarray(int > > semid, abi_ulong target_addr, > > for(i=0; i<nsems; i++) { > > __put_user((*host_array)[i], &array[i]); > > } > > - free(*host_array); > > + g_free(*host_array); > > unlock_user(array, target_addr, 1); > > > > return 0; > > @@ -2975,15 +2963,11 @@ static inline abi_long do_msgsnd(int msqid, > abi_long > > msgp, > > > > if (!lock_user_struct(VERIFY_READ, target_mb, msgp, 0)) > > return -TARGET_EFAULT; > > - host_mb = malloc(msgsz+sizeof(long)); > > - if (!host_mb) { > > - unlock_user_struct(target_mb, msgp, 0); > > - return -TARGET_ENOMEM; > > - } > > + host_mb = g_malloc(msgsz+sizeof(long)); > > msgsz comes from the guest -- you can't use g_malloc. > > > host_mb->mtype = (abi_long) tswapal(target_mb->mtype); > > memcpy(host_mb->mtext, target_mb->mtext, msgsz); > > ret = get_errno(msgsnd(msqid, host_mb, msgsz, msgflg)); > > - free(host_mb); > > + g_free(host_mb); > > unlock_user_struct(target_mb, msgp, 0); > > > > return ret; > > @@ -7625,7 +7609,7 @@ abi_long do_syscall(void *cpu_env, int num, > abi_long > > arg1, > > struct linux_dirent *dirp; > > abi_long count = arg3; > > > > - dirp = malloc(count); > > + dirp = g_try_malloc(count); > > if (!dirp) { > > ret = -TARGET_ENOMEM; > > goto fail; > > @@ -7662,7 +7646,7 @@ abi_long do_syscall(void *cpu_env, int num, > abi_long > > arg1, > > ret = count1; > > unlock_user(target_dirp, arg2, ret); > > } > > - free(dirp); > > + g_free(dirp); > > } > > #else > > { > > -- > > 1.9.1 > > thanks > -- PMM >
I meant last query just for following part of the code. > @@ -2975,15 +2963,11 @@ static inline abi_long do_msgsnd(int msqid, abi_long > msgp, > > if (!lock_user_struct(VERIFY_READ, target_mb, msgp, 0)) > return -TARGET_EFAULT; > - host_mb = malloc(msgsz+sizeof(long)); > - if (!host_mb) { > - unlock_user_struct(target_mb, msgp, 0); > - return -TARGET_ENOMEM; > - } > + host_mb = g_malloc(msgsz+sizeof(long)); On Thu, Oct 1, 2015 at 9:42 PM, Harmandeep Kaur <write.harmandeep@gmail.com> wrote: > Hi Peter, > > Can you please tell me if I should use other glib > functions, or implement it in any other way? > I am newbie here :) > > Thank you for reviewing my patch. > > On Thu, Oct 1, 2015 at 9:33 PM, Peter Maydell <peter.maydell@linaro.org> > wrote: > >> On 1 October 2015 at 06:32, Harmandeep Kaur <write.harmandeep@gmail.com> >> wrote: >> > Convert malloc()/calloc() calls to g_malloc()/g_try_malloc()/g_new0() >> > in linux-user/syscall.c file >> > >> > v1->v2 convert the free() call in host_to_target_semarray() >> > to g_free() and calls g_try_malloc(count) instead of >> > g_try_malloc(sizeof(count)) >> > >> > Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com> >> > --- >> > linux-user/syscall.c | 38 +++++++++++--------------------------- >> > 1 file changed, 11 insertions(+), 27 deletions(-) >> > >> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c >> > index d1d3eb2..c79e862 100644 >> > --- a/linux-user/syscall.c >> > +++ b/linux-user/syscall.c >> > @@ -1554,12 +1554,7 @@ set_timeout: >> > } >> > >> > fprog.len = tswap16(tfprog->len); >> > - filter = malloc(fprog.len * sizeof(*filter)); >> > - if (filter == NULL) { >> > - unlock_user_struct(tfilter, tfprog->filter, 1); >> > - unlock_user_struct(tfprog, optval_addr, 1); >> > - return -TARGET_ENOMEM; >> > - } >> > >> > + filter = g_malloc(fprog.len * sizeof(*filter)); >> >> fprog.len comes from the guest -- you can't use g_malloc. >> >> > for (i = 0; i < fprog.len; i++) { >> > filter[i].code = tswap16(tfilter[i].code); >> > filter[i].jt = tfilter[i].jt; >> > @@ -1570,7 +1565,7 @@ set_timeout: >> > >> > ret = get_errno(setsockopt(sockfd, SOL_SOCKET, >> > SO_ATTACH_FILTER, &fprog, >> sizeof(fprog))); >> > - free(filter); >> > + g_free(filter); >> > >> > unlock_user_struct(tfilter, tfprog->filter, 1); >> > unlock_user_struct(tfprog, optval_addr, 1); >> > @@ -1881,11 +1876,7 @@ static struct iovec *lock_iovec(int type, >> abi_ulong >> > target_addr, >> > return NULL; >> > } >> > >> > - vec = calloc(count, sizeof(struct iovec)); >> > - if (vec == NULL) { >> > - errno = ENOMEM; >> > - return NULL; >> > - } >> > + vec = g_new0(struct iovec, count); >> >> count comes from the guest -- you can't use g_new0. >> >> > >> > target_vec = lock_user(VERIFY_READ, target_addr, >> > count * sizeof(struct target_iovec), 1); >> > @@ -1945,7 +1936,7 @@ static struct iovec *lock_iovec(int type, >> abi_ulong >> > target_addr, >> > } >> > unlock_user(target_vec, target_addr, 0); >> > fail2: >> > - free(vec); >> > + g_free(vec); >> > errno = err; >> > return NULL; >> > } >> > @@ -2672,14 +2663,11 @@ static inline abi_long >> target_to_host_semarray(int >> > semid, unsigned short **host_ >> > >> > nsems = semid_ds.sem_nsems; >> > >> > - *host_array = malloc(nsems*sizeof(unsigned short)); >> > - if (!*host_array) { >> > - return -TARGET_ENOMEM; >> > - } >> > + *host_array = g_malloc(nsems*sizeof(unsigned short)); >> > array = lock_user(VERIFY_READ, target_addr, >> > nsems*sizeof(unsigned short), 1); >> >> I don't think we can guarantee that nsems is small -- you can't >> use g_malloc. >> >> > if (!array) { >> > - free(*host_array); >> > + g_free(*host_array); >> > return -TARGET_EFAULT; >> > } >> > >> > @@ -2716,7 +2704,7 @@ static inline abi_long host_to_target_semarray(int >> > semid, abi_ulong target_addr, >> > for(i=0; i<nsems; i++) { >> > __put_user((*host_array)[i], &array[i]); >> > } >> > - free(*host_array); >> > + g_free(*host_array); >> > unlock_user(array, target_addr, 1); >> > >> > return 0; >> > @@ -2975,15 +2963,11 @@ static inline abi_long do_msgsnd(int msqid, >> abi_long >> > msgp, >> > >> > if (!lock_user_struct(VERIFY_READ, target_mb, msgp, 0)) >> > return -TARGET_EFAULT; >> > - host_mb = malloc(msgsz+sizeof(long)); >> > - if (!host_mb) { >> > - unlock_user_struct(target_mb, msgp, 0); >> > - return -TARGET_ENOMEM; >> > - } >> > + host_mb = g_malloc(msgsz+sizeof(long)); >> >> msgsz comes from the guest -- you can't use g_malloc. >> >> > host_mb->mtype = (abi_long) tswapal(target_mb->mtype); >> > memcpy(host_mb->mtext, target_mb->mtext, msgsz); >> > ret = get_errno(msgsnd(msqid, host_mb, msgsz, msgflg)); >> > - free(host_mb); >> > + g_free(host_mb); >> > unlock_user_struct(target_mb, msgp, 0); >> > >> > return ret; >> > @@ -7625,7 +7609,7 @@ abi_long do_syscall(void *cpu_env, int num, >> abi_long >> > arg1, >> > struct linux_dirent *dirp; >> > abi_long count = arg3; >> > >> > - dirp = malloc(count); >> > + dirp = g_try_malloc(count); >> > if (!dirp) { >> > ret = -TARGET_ENOMEM; >> > goto fail; >> > @@ -7662,7 +7646,7 @@ abi_long do_syscall(void *cpu_env, int num, >> abi_long >> > arg1, >> > ret = count1; >> > unlock_user(target_dirp, arg2, ret); >> > } >> > - free(dirp); >> > + g_free(dirp); >> > } >> > #else >> > { >> > -- >> > 1.9.1 >> >> thanks >> -- PMM >> > >
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index d1d3eb2..c79e862 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -1554,12 +1554,7 @@ set_timeout: } fprog.len = tswap16(tfprog->len); - filter = malloc(fprog.len * sizeof(*filter)); - if (filter == NULL) { - unlock_user_struct(tfilter, tfprog->filter, 1); - unlock_user_struct(tfprog, optval_addr, 1); - return -TARGET_ENOMEM; - } + filter = g_malloc(fprog.len * sizeof(*filter)); for (i = 0; i < fprog.len; i++) { filter[i].code = tswap16(tfilter[i].code); filter[i].jt = tfilter[i].jt; @@ -1570,7 +1565,7 @@ set_timeout: ret = get_errno(setsockopt(sockfd, SOL_SOCKET, SO_ATTACH_FILTER, &fprog, sizeof(fprog))); - free(filter); + g_free(filter); unlock_user_struct(tfilter, tfprog->filter, 1); unlock_user_struct(tfprog, optval_addr, 1); @@ -1881,11 +1876,7 @@ static struct iovec *lock_iovec(int type, abi_ulong target_addr, return NULL; } - vec = calloc(count, sizeof(struct iovec)); - if (vec == NULL) { - errno = ENOMEM; - return NULL; - } + vec = g_new0(struct iovec, count); target_vec = lock_user(VERIFY_READ, target_addr,
Convert malloc()/calloc() calls to g_malloc()/g_try_malloc()/g_new0() in linux-user/syscall.c file v1->v2 convert the free() call in host_to_target_semarray() to g_free() and calls g_try_malloc(count) instead of g_try_malloc(sizeof(count)) Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com> --- linux-user/syscall.c | 38 +++++++++++--------------------------- 1 file changed, 11 insertions(+), 27 deletions(-) count * sizeof(struct target_iovec), 1); @@ -1945,7 +1936,7 @@ static struct iovec *lock_iovec(int type, abi_ulong target_addr, } unlock_user(target_vec, target_addr, 0); fail2: - free(vec); + g_free(vec); errno = err; return NULL; } @@ -2672,14 +2663,11 @@ static inline abi_long target_to_host_semarray(int semid, unsigned short **host_ nsems = semid_ds.sem_nsems; - *host_array = malloc(nsems*sizeof(unsigned short)); - if (!*host_array) { - return -TARGET_ENOMEM; - } + *host_array = g_malloc(nsems*sizeof(unsigned short)); array = lock_user(VERIFY_READ, target_addr, nsems*sizeof(unsigned short), 1); if (!array) { - free(*host_array); + g_free(*host_array); return -TARGET_EFAULT; } @@ -2716,7 +2704,7 @@ static inline abi_long host_to_target_semarray(int semid, abi_ulong target_addr, for(i=0; i<nsems; i++) { __put_user((*host_array)[i], &array[i]); } - free(*host_array); + g_free(*host_array); unlock_user(array, target_addr, 1); return 0; @@ -2975,15 +2963,11 @@ static inline abi_long do_msgsnd(int msqid, abi_long msgp, if (!lock_user_struct(VERIFY_READ, target_mb, msgp, 0)) return -TARGET_EFAULT; - host_mb = malloc(msgsz+sizeof(long)); - if (!host_mb) { - unlock_user_struct(target_mb, msgp, 0); - return -TARGET_ENOMEM; - } + host_mb = g_malloc(msgsz+sizeof(long)); host_mb->mtype = (abi_long) tswapal(target_mb->mtype); memcpy(host_mb->mtext, target_mb->mtext, msgsz); ret = get_errno(msgsnd(msqid, host_mb, msgsz, msgflg)); - free(host_mb); + g_free(host_mb); unlock_user_struct(target_mb, msgp, 0); return ret; @@ -7625,7 +7609,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, struct linux_dirent *dirp; abi_long count = arg3; - dirp = malloc(count); + dirp = g_try_malloc(count); if (!dirp) { ret = -TARGET_ENOMEM; goto fail; @@ -7662,7 +7646,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, ret = count1; unlock_user(target_dirp, arg2, ret); } - free(dirp); + g_free(dirp); } #else {