[PATCH 4/4] dhcp6c: DHCPv6 client

[Denys Vlasenko]

On Saturday 01 October 2011 18:12, Leonid Lisovskiy wrote:
> This patch contains ported DHCPv6 client. Due to patch is huge
> (230Kb), attach is gzipped.
> 
> With all options enabled applet increases size of bb code approx. to
> 36Kb on x86.

It looks WAY too big.

There are planty of places to shrink the code.
For example, let's look at dhcp6c_script.c:


#include <sys/types.h>
#include <sys/socket.h>
#include <sys/queue.h>
#include <sys/wait.h>
#include <sys/stat.h>

#include <netinet/in.h>

#include <fcntl.h>
#include <unistd.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <syslog.h>
#include <errno.h>

#include "common.h"

common.h includes libbb.h, which in turn includes most of system includes.
You can remove almost all includes above this line.




        const char reason[] = "REASON=NBI";
...
        *curr = xstrdup(reason);

Why reason[] exists?


                        elen = strlen(client6_envp_list[i].name) + 1 + elens[i];
                        *curr = xzalloc(elen);
                        snprintf(*curr, elen, "%s=", client6_envp_list[i].name);
                        ^^^^^^^^^^^^
Can use sprintf: we know we won't overflow.

                        TAILQ_FOREACH(v, &optinfo->ad_list, link) {
                                if (v->dh6optype != client6_envp_list[i].type)
                                        continue;
                                /* since we count total length above, it is safely to use strcat() */
                                switch (v->lvtype) {
                                    case DHCP6_LISTVAL_VBUF:
                                            strcat(*curr, v->val_vbuf.dv_buf);
                                            strcat(*curr, " ");
strcat is inefficient. strcat(" ") doubly so:
can use *p++ = ' ' instead.
                                            break;
                                    case DHCP6_LISTVAL_ADDR6:
                                            sprint_nip6(a, (const uint8_t *)&v->val_addr6);
                                            strcat(*curr, a);
                                            strcat(*curr, " ");
                                            break;
                                    default:
                                            break;
                                }
                        }




static int safefile(const char *path)
{
        struct stat s;
        uid_t myuid = getuid();

        /* no setuid */
        if (myuid != geteuid()) {
                bb_info_msg("setuid'ed execution not allowed");
                return -1;
        }

        if (access(path, X_OK) || lstat(path, &s)) {
                return -1;
        }

        if (!S_ISREG(s.st_mode) && !S_ISLNK(s.st_mode)) {
                return -1;
        }

        /* the file must be owned by the running uid */
        if (s.st_uid != myuid) {
                bb_info_msg("script has invalid owner");
                return -1;
        }

        return 0;
}

int dhcp6_script(const char *scriptpath, char **envp)
{
        char *argv[2];

        /* if a script is not specified, do nothing */
        if (strlen(scriptpath) == 0)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Use if (!scriptpath[0]) ... instead

                return -2;

        if (safefile(scriptpath)) {
                bb_error_msg("script \"%s\" cannot be executed safely", scriptpath);
                return -2;
        }
Nuke safefile altogether, it's bloat. We never do such checks
in other applets.



dhcp6_script() is called exactly one, this way:

         dhcp6_script(ifp->scriptpath, fill_envp_client6(optinfo, ifp->ifname));

which is also the only callsite of fill_envp_client6.
Therefore, fill_envp_client6 can be made static to dhcp6c_script.c
and called *from within* dhcp6_script() (which, by the way,
is better be renamed to dhcp6_run_script()...)



Elsewhere, random observations:


        memset(&hints, 0, sizeof(hints));
        hints.ai_family = PF_INET6;
        hints.ai_socktype = SOCK_DGRAM;
        hints.ai_protocol = IPPROTO_UDP;
        error = getaddrinfo(DH6ADDR_ALLAGENT, DH6PORT_UPSTREAM, &hints, &res);
        if (error) {
                bb_error_msg_and_die("getaddrinfo: %s", gai_strerror(error));
        }
        memcpy(&G.sa6_allagent, res->ai_addr, res->ai_addrlen);
        freeaddrinfo(res);

???! This is a fixed IPv6 address, [ff02::1:2]:547, it can be just a constant struct,
right?



        argv += optind;
        argc -= optind;
        if (argc < 1) {
                bb_show_usage();
        }

        if (!(opt & OPT_FOREGROUND) && !(opt & OPT_i)) {
                bb_daemonize_or_rexec(DAEMON_CLOSE_EXTRA_FDS, argv);

BUG: would re-execute (on NOMMU) with wrong argv: argv += optind
is done too early.



Need documentation for /etc/dhcp6c.conf format.



Patch does not compile with uclibc:

networking/udhcp/lib.a(if6.o): In function `gethwid':
if6.c:(.text.gethwid+0xf): undefined reference to `getifaddrs'
if6.c:(.text.gethwid+0x9a): undefined reference to `freeifaddrs'
networking/udhcp/lib.a(if6.o): In function `if6init':
if6.c:(.text.if6init+0x88): undefined reference to `getifaddrs'
if6.c:(.text.if6init+0x10d): undefined reference to `freeifaddrs'
networking/udhcp/lib.a(common6.o): In function `duidstr':
common6.c:(.text.duidstr+0x9): undefined reference to `BUG_dhcp6c_globals_too_big'
collect2: ld returned 1 exit status
make: *** [busybox_unstripped] Error 1

-- 
vda