Re: [Qemu-devel] [PATCH 1/2] linux-user: Add setsockopt(SO_ATTACH_FILTER)

[Peter Maydell]

On 31 December 2012 19:37, Laurent Vivier <address@hidden> wrote:
> This is needed to be able to run dhclient.
>
> Signed-off-by: Laurent Vivier <address@hidden>
> ---
>  linux-user/syscall.c      |   34 +++++++++++++++++++++++++++++++++-
>  linux-user/syscall_defs.h |   12 ++++++++++++
>  2 files changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index e99adab..000b640 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -98,6 +98,7 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
>  #include <linux/fb.h>
>  #include <linux/vt.h>
>  #include <linux/dm-ioctl.h>
> +#include <linux/filter.h>
>  #include "linux_loop.h"
>  #include "cpu-uname.h"
>
> @@ -1491,6 +1492,38 @@ static abi_long do_setsockopt(int sockfd, int level, 
> int optname,
>          break;
>      case TARGET_SOL_SOCKET:
>          switch (optname) {
> +        case TARGET_SO_ATTACH_FILTER: {

This brace should go on a line of its own (lined up with the 'c' in case) IMHO.

> +                struct target_sock_fprog *tfprog;
> +                struct target_sock_filter *tfilter;
> +                struct sock_fprog fprog;
> +                struct sock_filter *filter;
> +                int i;
> +
> +                if (optlen != sizeof(*tfprog))
> +                    return -TARGET_EINVAL;

QEMU style requires braces on this if. You can use checkpatch.pl
to catch this kind of thing.

> +                if (!lock_user_struct(VERIFY_READ, tfprog, optval_addr, 0))
> +                    return -TARGET_EFAULT;
> +                if (!lock_user_struct(VERIFY_READ, tfilter,
> +                                      tswapal(tfprog->filter), 0))
> +                    return -TARGET_EFAULT;

This will fail to unlock tfprog in the failure case.

> +
> +                fprog.len = tswap16(tfprog->len);
> +                filter = alloca(fprog.len * sizeof(*filter));

Not sure an unconstrained-size alloca based on data from
the guest binary is a fantastic idea (though we no doubt
do something similar for some other syscalls).

> +                for (i = 0; i < fprog.len; i ++) {
> +                    filter[i].code = tswap16(tfilter[i].code);
> +                    filter[i].jt = tfilter[i].jt;
> +                    filter[i].jf = tfilter[i].jf;
> +                    filter[i].k = tswap32(tfilter[i].k);
> +                }
> +                fprog.filter = filter;
> +
> +                ret = get_errno(setsockopt(sockfd, SOL_SOCKET,
> +                                SO_ATTACH_FILTER, &fprog, sizeof(fprog)));
> +
> +                unlock_user_struct(tfilter, tfprog->filter, 1);
> +                unlock_user_struct(tfprog, optval_addr, 1);
> +                return ret;
> +        }
>              /* Options with 'int' argument.  */
>          case TARGET_SO_DEBUG:
>                 optname = SO_DEBUG;
> @@ -1548,7 +1581,6 @@ static abi_long do_setsockopt(int sockfd, int level, 
> int optname,
>          case TARGET_SO_SNDTIMEO:
>                 optname = SO_SNDTIMEO;
>                 break;
> -            break;

Nice catch, but this is an unrelated change that should go in its own patch.

>          default:
>              goto unimplemented;
>          }

-- PMM