[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/libhip into lp:hipl

  • From: Christof Mroz <christof.mroz@xxxxxxxxxxxxxx>
  • To: mp+92480@xxxxxxxxxxxxxxxxxx
  • Date: Tue, 14 Feb 2012 20:34:20 -0000

Review: Needs Fixing

Implementation looks good overall, and splitting the code should make HIPL more 
flexible for users.
You may ignore my comments about code that was not newly written but just 
moved, of course.

> === modified file 'lib/core/hostid.c'
> --- lib/core/hostid.c 2011-11-10 10:35:47 +0000
> +++ lib/core/hostid.c 2012-02-12 20:26:22 +0000
> @@ -743,11 +744,16 @@
>                  goto out_err;
>              }
>          } else if (!use_default) {
> +            char *hi_file_dup = strdup(hi_file);
> +            if ((err = check_and_create_dir(dirname(hi_file_dup), 
> HIP_DIR_MODE))) {
> +                HIP_ERROR("Could not create direcory for path: %s\n", 
> hi_file);
> +                goto out_err;
> +            }

You allocate a copy using strdup(), but I don't see where you free() it.

> === modified file 'lib/core/linkedlist.c'
> --- lib/core/linkedlist.c     2011-08-15 14:11:56 +0000
> +++ lib/core/linkedlist.c     2012-02-12 20:26:22 +0000
> @@ -298,6 +298,31 @@
>  }
>  
>  /**
> + * Deletes the first node in a list with the given element as its data.
> + * If there is no match, does nothing.
> + *
> + * @param linkedlist   list to remoe an element from
> + * @param ptr          pointer by which to identify the node
> + * @param free_element a function pointer to a function for freeing the 
> memory
> + *                     allocated for an element at a node or NULL if the 
> element
> + *                     itself is not to be freed.
> + */
> +void hip_ll_del_by_ptr(struct hip_ll *linkedlist, void *ptr,
> +                       free_elem_fn free_element)
> +{
> +    int                       index   = 0;
> +    const struct hip_ll_node *current = NULL;
> +
> +    while ((current = hip_ll_iterate(linkedlist, current))) {
> +        if (current->ptr == ptr) {
> +            hip_ll_del(linkedlist, index, free_element);
> +            return;
> +        }
> +        index++;
> +    }
> +}

You found the link already, so traversing the list again, using an integer 
index, is redundant.

> === modified file 'lib/core/message.c'
> --- lib/core/message.c        2011-11-08 15:25:41 +0000
> +++ lib/core/message.c        2012-02-12 20:26:22 +0000
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2010 Aalto University and RWTH Aachen University.
> + * Copyright (c) 2010-2012 Aalto University and RWTH Aachen University.
>   *
>   * Permission is hereby granted, free of charge, to any person
>   * obtaining a copy of this software and associated documentation
> @@ -686,6 +686,79 @@
>  }
>  
>  /**
> + * Read a control message over TCP socket.
> + *
> + * @param  sockfd         a socket file descriptor
> + * @param  ctx            a pointer to the packet context
> + * @return                -1 in case of an error, 0 otherwise.
> + */
> +int hip_read_control_msg_tcp(int sockfd, struct hip_packet_context *ctx)
> +{
> +    int                  len, is_ipv4;
> +    struct sockaddr      dst_addr = { 0 };
> +    struct sockaddr      src_addr = { 0 };
> +    struct sockaddr_in  *saddr4;
> +    struct sockaddr_in6 *saddr6;
> +    socklen_t            saddr_len = sizeof(struct sockaddr);
> +
> +    hip_msg_init(ctx->input_msg);
> +
> +    len = recv(sockfd, ctx->input_msg, HIP_MAX_PACKET, 0);
> +    if (len < 0) {
> +        HIP_PERROR("recvfrom(): ");
> +        return -1;
> +    }

Initializing is useless if recv() overwrites the packet anyway I think.

> +
> +    /* Get peer address */
> +    if (getpeername(sockfd, &src_addr, &saddr_len) < 0) {
> +        HIP_PERROR("getpeername(): ");
> +        return -1;
> +    }
> +
> +    is_ipv4 = src_addr.sa_family == AF_INET ? 1 : 0;
> 

The ternary operator is superfluous.

> +int  esp_prot_active               = 0;
> +int  esp_prot_num_transforms       = 0;
> +long esp_prot_num_parallel_hchains = 0;

Unrelated: hopefully whoever originally wrote this realized that long is still 
just 32 bit or more.


> +int hip_set_libhip_mode()
> +{
> +    hipd_library_mode = 1;
> +    return 0;
> +}

This could be void. Also, it lacks documentation.

> +int libhipd_init(void)
> +{
> +    int                err         = 0;
> +    int                keypath_len = 0;
> +    char              *key_path    = NULL;
> +    struct hip_common *msg         = NULL;
> +    struct passwd     *pwd;
> +
> +    hip_set_libhip_mode();
> +    hip_nat_status = 1;
> +#ifdef CONFIG_HIP_FIREWALL
> +    hipfw_status = 0;
> +#endif
> +
> +    hip_init_hadb();
> +    hip_init_hostid_db();
> +    hip_netdev_init_addresses();
> +    libhip_init_handle_functions();
> +
> +    /* Load default key from ~/.hipl/ */
> +    if ((pwd = getpwuid(getuid())) == NULL) {
> +        return -1;
> +    }
> +
> +    /* +2 because we need a slash after pwd and a NULL for termination */
> +    keypath_len = strlen(pwd->pw_dir) +
> +                  strlen(HIP_USER_DIR) +
> +                  strlen(DEFAULT_HOST_RSA_KEY_FILE_NAME) +
> +                  strlen(DEFAULT_PUB_HI_FILE_NAME_SUFFIX) + 2;
> +    HIP_IFEL(!(key_path = malloc(keypath_len)), -1, "malloc() failed");

You could statically allocate the string buffer here.

[We indeed used to have a policy of forcing putting all variables at the 
beginning of the outermost local scope, but this was removed. Mostly to allow 
stricter const-correctness.]

> +static void set_hip_connection_parameters(int sock_fd, int local_port,
> +                                          int remote_port)
> +{
> +    HIP_DEBUG("fd = %d, lport = %d, rport= %d\n", sock_fd, local_port, 
> remote_port);
> +    hip_nat_sock_output_udp = sock_fd;
> +    hip_raw_sock_output_v4  = sock_fd;
> +    hip_raw_sock_output_v6  = sock_fd;
> +    hip_set_local_nat_udp_port(local_port);
> +    hip_set_peer_nat_udp_port(remote_port);
> +}

Unrelated: missing docs.

> +/**
> + * Receive data from the peer associated with a socket.
> + * Waits for base exchange if no host association exists.
> + *
> + * @note Data is currently sent unencrypted.
> + *
> + * @param fd        file descriptor of the socket to receive from
> + * @param buf       buffer for received data
> + * @param len       size of buf
> + * @param flags     recvfrom() flags
> + * @param addr      buffer for the associated peer HIT
> + * @param addr_len  size of dst_hit
> + * @return          number of bytes received on success, -1 otherwise
> + */
> +int hip_recvfrom(int fd, void *buf, size_t len, int flags,
> +                 struct sockaddr *addr, socklen_t *addr_len)
> +{
> +    int                       err       = 0;
> +    socklen_t                 socklen   = *addr_len;
> +    struct sockaddr_in6      *peer_hit  = (struct sockaddr_in6 *) addr;
> +    struct in6_addr           peer_addr = { { { 0 } } };
> +    struct in6_addr          *peer_addr6;
> +    struct in_addr           *peer_addr4;
> +    struct hip_fd_info       *fd_info = NULL;
> +    struct hip_packet_context ctx     = { 0 };
> +    int                       (*read_control_msg)(int, struct 
> hip_packet_context *, int) = NULL;
> +
> +
> +    if ((fd_info = hip_socket_get_info(fd)) == NULL) {
> +        HIP_ERROR("Fd %d is not a hip socket, exiting.\n", fd);
> +        return -1;
> +    }
> +
> +    /* Bind to a ephemeral port if the src port hasn't been bound yet */
> +    if (fd_info->bound_port == 0) {
> +        if (auto_bind(fd_info)) {
> +            HIP_ERROR("Fail to bind the hip socket.\n");
> +            return -1;
> +        }
> +    }
> +
> +    /* Handle BEX if HA hasn't establised */
> +    if (!fd_info->ha) {
> +        if (hip_await_bex(fd_info, addr)) {
> +            HIP_ERROR("Base exchange not successful.\n");
> +            return -1;
> +        }
> +    }
> +
> +    ctx.input_msg    = hip_msg_alloc();
> +    ctx.output_msg   = hip_msg_alloc();
> +    read_control_msg = fd_info->family == AF_INET ? hip_read_control_msg_v4
> +                       : hip_read_control_msg_v6;
> +
> +    /* Loop until we get a non-control packet or a CLOSE packet */
> +    while (fd_info->ha->state == HIP_STATE_ESTABLISHED) {
> +        err = recvfrom(fd, buf, len, flags | MSG_PEEK, addr, &socklen);
> +        HIP_DEBUG("Peek packet len: %d\n", err);
> +        HIP_DEBUG("peer sockaddr: AF = %d, socklen = %d\n", addr->sa_family, 
> socklen);
> +        if (err < 0) {
> +            perror("recvfrom");
> +        }
> +
> +        /* Drop the packet if it doesn't come from the address associated
> +         * with the correct peer. */
> +        if (fd_info->proto == IPPROTO_UDP) {
> +            if (addr->sa_family == AF_INET) {
> +                peer_addr4 = &((struct sockaddr_in *) addr)->sin_addr;
> +                IPV4_TO_IPV6_MAP(peer_addr4, &peer_addr);
> +                peer_addr6 = &peer_addr;
> +            } else {
> +                peer_addr6 = &((struct sockaddr_in6 *) addr)->sin6_addr;
> +            }
> +            if (ipv6_addr_cmp(&fd_info->ha->peer_addr, peer_addr6)) {
> +                HIP_DEBUG("Packet not from associated address. Dropping.\n");
> +                HIP_DEBUG_IN6ADDR("expected", &fd_info->ha->peer_addr);
> +                HIP_DEBUG_IN6ADDR("got", peer_addr6);
> +                err = recvfrom(fd, buf, 1, flags, addr, &socklen);
> +                HIP_IFEL(err < 0, err, "recvfrom()\n");
> +                continue;
> +            }
> +        }
> +
> +        /* Receive message */
> +        if (hip_is_control_msg(buf, err, fd_info)) {
> +            HIP_DEBUG("receive a hip control message.\n");
> +            if (fd_info->proto == IPPROTO_TCP) {
> +                if (!hip_read_control_msg_tcp(fd, &ctx)) {
> +                    hip_receive_control_packet(&ctx);
> +                }
> +            } else if (!read_control_msg(fd, &ctx, HIP_UDP_ZERO_BYTES_LEN)) {
> +                hip_receive_control_packet(&ctx);
> +            } else {
> +                HIP_ERROR("Error reading control packet\n");
> +            }
> +            err = 0;
> +        } else {
> +            HIP_DEBUG("receive a non hip control message.\n");
> +            err = recvfrom(fd, buf, len, flags, addr, &socklen);
> +            HIP_IFEL(err < 0, err, "recvfrom() error\n");
> +            break;
> +        }
> +    }

Unrelated: Could this be a possible DOS, by crafting a packet that triggers one 
of the nested recvfrom()s but never sends anything after that, causing it to 
recv forever?
If this is the "main socket", it would probably not hang but we could slurp 
bytes from genuine clients this way.

> +int hip_accept(int fd, struct sockaddr *new_peer_hit, socklen_t *hit_len)
> +{
> +    int                     new_fd;
> +    struct hip_fd_info     *fd_info     = NULL;
> +    struct hip_fd_info     *fd_info_new = NULL;
> +    struct sockaddr_storage ss          = { 0 };
> +    socklen_t               ss_len      = sizeof(struct sockaddr_storage);
> +    struct sockaddr_in6    *phit        = NULL;
> +
> +    if (*hit_len < sizeof(struct sockaddr_in6)) {
> +        return -1;
> +    }
> +
> +    if ((fd_info = hip_socket_get_info(fd)) == NULL) {
> +        HIP_ERROR("Fd %d is not a hip socket, exiting.\n", fd);
> +        return -1;
> +    }
> +
> +    new_fd = accept(fd, (struct sockaddr *) &ss, &ss_len);
> +    if (new_fd < 0) {
> +        HIP_PERROR("accept(): ");
> +        return -1;
> +    }
> +
> +    fd_info_new = create_new_fd_info(new_fd, fd_info->bound_port,
> +                                     fd_info->family, fd_info->proto);
> +
> +    if (hip_await_bex(fd_info_new, (struct sockaddr *) &ss) < 0) {
> +        HIP_ERROR("Base exchange not successful.\n");
> +        return -1;
> +    }
> +
> +    if (new_peer_hit) {
> +        phit = (struct sockaddr_in6 *) new_peer_hit;
> +        memset(phit, 0, *hit_len);
> +        memcpy(&phit->sin6_addr, &fd_info_new->ha->hit_peer,
> +               sizeof(struct in6_addr));
> +        phit->sin6_port = get_port_from_saddr((struct sockaddr *) &ss);
> +        *hit_len        = sizeof(struct sockaddr_in6);
> +    }
> +
> +    return new_fd;
> +}

fd_info_new is not freed.

> +    /* start hipnetcat server */
> +    if ((pid = fork()) > 0) {
> +        serv_pid = pid;
> +        printf("server pid: %d\n", serv_pid);
> +    }
> +
> +    if (pid == 0) {
> +        if (execv("test/hipnetcat", serv_argv)) {
> +            perror("execv");
> +            return;
> +        }
> +    }
> +
> +    /* start hipnetcat client */
> +    if ((pid = fork()) > 0) {
> +        client_pid = pid;
> +        printf("client_pid: %d\n", client_pid);
> +    }
> +
> +    if (pid == 0) {
> +        sleep(1);
> +        if (execv("test/hipnetcat", client_argv)) {
> +            perror("execv");
> +            return;
> +        }
> +    }

Why sleep() here but not above?

> +    /* check server & client status */
> +    for (i = 0; i < TEST_HIPNC_TIMEOUT; i++) {
> +        tv.tv_sec  = 1;
> +        tv.tv_usec = 0;
> +        select(0, NULL, NULL, NULL, &tv);
> +        round = remain_cld;
> +        for (j = 0; j < round; j++) {
> +            pid = waitpid(-1, &status, WNOHANG);
> +            fail_if(pid > 0 && status != 0, "hipnetcat failed");
> +            if (pid > 0 && status == 0) {
> +                remain_cld--;
> +                if (remain_cld == 0) {
> +                    break;
> +                }
> +            }
> +        }
> +    }
> +
> +    fail_if(remain_cld > 0, "hipnetcat test timeout!");
> +}

I guess that the daemon and firewall store much (all?) of their state 
statically, and this forces you to spawn separate processes even though that's 
an ugly thing to do in a test (which should be self-contained without side 
effects), correct?

For completeness' sake I'd suggest an OpenGL-ish approach: simply provide two 
functions that retrieve or store the global state, respectively. Only those 
portions relevant to library users, of course.
Then you can restructure the test as follows

void *ctx[2];

libhip_init();
ctx[0] = libhip_get_context();
libhip_init();
ctx[1] = libhip_get_context();

libhip_set_context(ctx[0]);
send(...);
libhip_set_context(ctx[1]);
recv(...);
check stuff

libip_destroy_context(ctx[0]);
libip_destroy_context(ctx[1]);

Note that returning anything else than void* will break legacy libhip users 
whenever the context struct changes.
I can understand if you don't have time to waste on that interface, though :)

> +static int hipnc_run_client(const unsigned int flag,
> +                            struct sockaddr *local_addr, socklen_t addr_len,
> +                            struct sockaddr_in6 *peer_hit_saddr)
> +{
> +    int  fd, err = 0, len = 0;
> +    char buf[256] = { 0 };
> +
> +    if ((fd = create_hip_socket(flag)) < 0) {
> +        HIP_PERROR("Failed to create hip socket.");
> +        return -1;
> +    }
> +
> +    HIP_IFEL(hip_bind(fd, local_addr, addr_len), -1, "hip_bind error\n");
> +
> +    if (!(flag & HIPNC_UDP_MODE)) {
> +        HIP_IFEL(hip_connect(fd, (struct sockaddr *) peer_hit_saddr,
> +                             sizeof(struct sockaddr_in6)),
> +                 -1, "connect()\n");
> +    }
> +
> +    sprintf(buf, "Hello, sailor!");

You could init the buffer statically.

> +    len = hip_sendto(fd, buf, sizeof(buf), 0,
> +                     (struct sockaddr *) peer_hit_saddr,
> +                     sizeof(struct sockaddr_in6));
> +
> +    HIP_INFO("Sent %d bytes\n", len);
> +    if (len < 0) {
> +        err = -1;
> +    }

You actually want to check (len < sizeof(buf))

> +    HIP_IFEL(hip_bind(fd, local_addr, addr_len), -1, "hip_bind error\n");
> +
> +    if (!(flag & HIPNC_UDP_MODE)) {
> +        HIP_IFEL(listen(fd, 0), -1, "listen()\n");
> +        new_fd = hip_accept(fd, (struct sockaddr *) &peer_hit_saddr, 
> &socklen);
> +        HIP_IFEL(new_fd < 0, -1, "accept()\n");

perror()/errno would be more helpful (applies to all the error reporting in 
hipnc).

> +    while ((len = hip_recvfrom(new_fd, buf, sizeof(buf), 0,
> +                               (struct sockaddr *) &peer_hit_saddr,
> +                               &socklen)) > 0) {
> +        HIP_INFO("Received %d bytes\n", len);
> +        printf("%s\n", buf);

nit: unknown strings should be null-terminated explicitly before using.

> +static int parse_source_to_ss(const char *ip, int port, struct 
> sockaddr_storage *ss)
> +{
> +    struct sockaddr_in  *sa4;
> +    struct sockaddr_in6 *sa6;
> +    int                  err = 0;
> +
> +    sa4 = (struct sockaddr_in *) ss;
> +    sa6 = (struct sockaddr_in6 *) ss;
> +
> +    ss->ss_family = AF_INET;
> +    sa4->sin_port = htons(port);
> +    hipnc_flag   &= ~HIPNC_IP6;
> +    err           = inet_pton(AF_INET, ip, &sa4->sin_addr);
> +
> +    if (err <= 0) {
> +        memset(ss, 0, sizeof(*ss));
> +        ss->ss_family  = AF_INET6;
> +        sa6->sin6_port = htons(port);
> +        hipnc_flag    |= HIPNC_IP6;
> +        err            = inet_pton(AF_INET6, ip, &sa6->sin6_addr);
> +    }
> +
> +    return (err <= 0) ? -1 : 0;
> +}

Wouldn't the following be clearer?

if(inet_pton) > 0) {
        // ipv4
} else {
        // ipv6
}

> +    /* parse identifiers */
> +    while (optind < argc) {
> +        arg = argv[optind++];
> +        if (inet_pton(AF_INET, arg, &inaddr4)) {
> +            /* if it is a V4 addr, map it to V6 and add it to locator_list */
> +            HIP_DEBUG("%s is parsed to v4", arg);
> +            sprintf(addr_buf, "::FFFF:");
> +            strcat(addr_buf, arg);
> +            inet_pton(AF_INET6, addr_buf, &inaddr6);
> +            HIP_DEBUG(". Map to v6: %s\n", addr_buf);
> +            if ((paddr6 = malloc(sizeof(struct in6_addr))) == NULL) {
> +                HIP_PERROR("malloc() failed");
> +                return -1;
> +            }
> +            memcpy(paddr6, &inaddr6, sizeof(struct in6_addr));
> +            hip_ll_add_last(&locator_list, paddr6);
> +        } else if (inet_pton(AF_INET6, arg, (void *) &inaddr6)) {
> +            /* if it is a V6 addr, add it to locator_list or hit_list */
> +            HIP_DEBUG("%s is parsed to v6, is HIT: %d\n", arg,
> +                      ipv6_addr_is_hit(&inaddr6));
> +            if ((paddr6 = malloc(sizeof(struct in6_addr))) == NULL) {
> +                HIP_PERROR("malloc() failed");
> +                return -1;
> +            }
> +            memcpy(paddr6, &inaddr6, sizeof(struct in6_addr));
> +            if (ipv6_addr_is_hit(&inaddr6)) {
> +                hip_ll_add_last(&hit_list, paddr6);
> +            } else {
> +                hip_ll_add_last(&locator_list, paddr6);
> +            }
> +        } else {
> +            //Parse hostname
> +            struct addrinfo     *res;
> +            struct addrinfo      addr_hint = { 0 };
> +            struct sockaddr_in  *sa4;
> +            struct sockaddr_in6 *sa6;
> +
> +            addr_hint.ai_family = AF_UNSPEC;
> +
> +            HIP_DEBUG("Parse identifier: %s\n", arg);
> +            err = getaddrinfo(arg, NULL, &addr_hint, &res);
> +            HIP_IFEL(err, -1,
> +                     "failed to parse: %s, %s\n", arg, gai_strerror(err));
> +            for (; res != NULL; res = res->ai_next) {
> +                paddr6 = malloc(sizeof(struct in6_addr));
> +                HIP_IFEL(!paddr6, -1, "malloc() failed\n");
> +                if (res->ai_family == AF_INET) {
> +                    sa4 = (struct sockaddr_in *) res->ai_addr;
> +                    IPV4_TO_IPV6_MAP(&sa4->sin_addr, paddr6);
> +                    hip_ll_add_last(&locator_list, paddr6);
> +                } else if (res->ai_family == AF_INET6) {
> +                    sa6 = (struct sockaddr_in6 *) res->ai_addr;
> +                    memcpy(paddr6, &sa6->sin6_addr, sizeof(struct in6_addr));
> +                    if (ipv6_addr_is_hit(paddr6)) {
> +                        hip_ll_add_last(&hit_list, paddr6);
> +                    } else {
> +                        hip_ll_add_last(&locator_list, paddr6);
> +                    }
> +                }
> +            }
> +            freeaddrinfo(res);
> +        }
> +    }

getaddrinfo() can handle numerical addresses just fine, AFAIK.
-- 
https://code.launchpad.net/~hipl-core/hipl/libhip/+merge/92480
Your team HIPL core team is subscribed to branch lp:hipl.

Other related posts: