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.