Add libroute and basic librarification of route utility

[ahsanb]

Summary

This work is done as a part of Google Summer of Code 2020.

This change adds the libroute library, which intends to provide APIs for management of routes. This also includes the illustration of the API by its basic usage by the route utility. There are tests for the route utility in /src/sbin/route/tests. The tests currently tests the following basic functionalities.

1. Addition of route
2. Change of route
3. Deletion of route

These tests are our current method for the validation of library functionalities. We plan to add more tests to have a more rigorous testing. The planned tests include tests for Flushing the routes and also for the various other arguments accepted by the route utility.

[kp]

lib/libroute/Makefile.depend

3

This is an autogenerated file and should not be part of the diff.

lib/libroute/libroute.c

47

This will need an error check as well.

48

As will this.

56

Is it worth checking if the fib actually exists here?

69

Error checking here.

106

Line length.

lib/libroute/libroute.h

70

Is this function intended to be used by users of lib route, or is this internal?

If it's the former it needs a lib route prefix, if it's the latter it shouldn't be in our header file (and it should be static in the .c file).

[thj]

Please make sure to check for failure after memory allocation. You need to add the paths to report these errors and the socket errors that kp@ pointed out too.
Other than these there are some minor nits, reading style will probably flag these up.

lib/libroute/Makefile

13

I think you should remove these three lines. If not the "become" should be "becomes", I think this was just copied from libifconfigs Makefile

lib/libroute/libroute.c

41

I prefer taking the sizeof the struct.

43

I think you need to report ENOMEM when the allocation fails

60

extra space between * and str

64

You need to check for memory allocation failure here and report back when it does.

80

You need to check for memory allocation failure here and report back when it does.

142

I am not sure what this comment means. I could read it as a pending TODO, if it is a TODO you should note that. This comment appears each time in the file, maybe we could try rewording.

style(9) says:
"/* Most single-line comments look like this. */"

164

We tend to group all initialisers at the start of the function. In this case you should just return the error directly from the library.

This applies a few times in this file, please fix all instances

165

sbin/route/route.c

789

I think there is an extra newline here

1023

this line is very long, but it can be sensibly broken up.

1029

This might be a little long too, but I can't tell from phabricator.

[ahsanb]

Add error handling for memory allocations, socket creation etc. Fix line lengths.

[kp]

I've applied the patch on a CURRENT checkout, and run into this build error:

--- all_subdir_rescue ---
ld: error: undefined symbol: libroute_open
>>> referenced by _$$hide$$ route.lo route.c
>>>               route.lo:(_$$hide$$ route.lo newroute)

ld: error: undefined symbol: libroute_setfib
>>> referenced by _$$hide$$ route.lo route.c
>>>               route.lo:(_$$hide$$ route.lo newroute)

ld: error: undefined symbol: libroute_modify
>>> referenced by _$$hide$$ route.lo route.c
>>>               route.lo:(_$$hide$$ route.lo newroute)
cc: error: linker command failed with exit code 1 (use -v to see invocation)
*** [rescue] Error code 1

I assume we're missing a bit of build glue for the rescue system.

There's a recent commit that made ifconfig start to use libifconfig, and it had this bit for rescue:

diff --git a/rescue/rescue/Makefile b/rescue/rescue/Makefile
index 9681ebaddfa..66aa7f188ec 100644
--- a/rescue/rescue/Makefile
+++ b/rescue/rescue/Makefile
@@ -224,6 +224,9 @@ CRUNCH_ALIAS_chown= chgrp
 ##################################################################
 CRUNCH_LIBS+= -lm

+CRUNCH_LIBS+=          ${OBJTOP}/lib/libifconfig/libifconfig.a
+CRUNCH_BUILDOPTS+=     CRUNCH_CFLAGS+=-I${OBJTOP}/lib/libifconfig
+
 .if ${MK_ISCSI} != "no"
 CRUNCH_PROGS_usr.bin+= iscsictl
 CRUNCH_PROGS_usr.sbin+=        iscsid

[kp]

lib/libroute/libroute.c

47

Memory leak.

47

All returns should be return (NULL); (i.e. have () around the value).

49

Memory leak.

67

Space between if and (

135

I'd return either '-1', or errno.

Either way we'll want a way for the user to figure out what the error was.

I'd propose a int libroute_errno(struct rt_handle *h); function for that, much like what libifconfig does.

[ahsanb]

Fix memory leaks
Add method to get errorno

[kp]

In D26078#580351, @ahsanb wrote:

Fix memory leaks
Add method to get errorno

Thanks. Buildworld now passes, but with the new libroute-using route installed I see this during startup:

Starting Network: vtnet1.
vtnet1: flags=8922<BROADCAST,PROMISC,SIMPLEX,MULTICAST> metric 0 mtu 1500
        options=80028<VLAN_MTU,JUMBO_MTU,LINKSTATE>
        ether 58:9c:fc:04:73:33
        media: Ethernet 10Gbase-T <full-duplex>
        status: active
        nd6 options=21<PERFORMNUD,AUTO_LINKLOCAL>
add host 127.0.0.1: gateway lo0 fib 0: route already in table
add host 127.0.0.1: gateway lo0 fib 1: route already in table
add host 127.0.0.1: gateway lo0 fib 2: route already in table
add net default: gateway 172.16.2.1 fib 0: Invalid argument
add host ::1: gateway lo0 fib 0: route already in table
add host ::1: gateway lo0 fib 1: route already in table
add host ::1: gateway lo0 fib 2: route already in table
add net fe80::: gateway ::1 fib 0: Invalid argument
add net fe80::: gateway ::1 fib 1: Invalid argument
add net fe80::: gateway ::1 fib 2: Invalid argument
add net ff02::: gateway ::1 fib 0: Invalid argument
add net ff02::: gateway ::1 fib 1: Invalid argument
add net ff02::: gateway ::1 fib 2: Invalid argument
add net ::ffff:0.0.0.0: gateway ::1 fib 0: Invalid argument
add net ::ffff:0.0.0.0: gateway ::1 fib 1: Invalid argument
add net ::ffff:0.0.0.0: gateway ::1 fib 2: Invalid argument
add net ::0.0.0.0: gateway ::1 fib 0: Invalid argument
add net ::0.0.0.0: gateway ::1 fib 1: Invalid argument
add net ::0.0.0.0: gateway ::1 fib 2: Invalid argument

and I can't SSH into the machine.

When I try to add it manually:

sudo route add default 172.16.2.1
add net default: gateway 172.16.2.1 fib 0: Invalid argument

I think this is the relevant bit of truss output:

socket(PF_ROUTE,SOCK_RAW,0)                      = 4 (0x4)
setsockopt(4,SOL_SOCKET,SO_SETFIB,0x800a0b000,4) = 0 (0x0)
setsockopt(4,SOL_SOCKET,SO_SETFIB,0x800a0b000,4) = 0 (0x0)
write(4,"\M-8\0\^E\^A\0\0\0\0\^C\b\0\0\^C"...,184) ERR#22 'Invalid argument'

[thj]

lib/libroute/libroute.c

53

This test should be against -1

56

space missing between closing bracket and the brace

84

This test should be against -1

107

brackets on the return

130

brackets on the return

143

I think this allows a buffer overflow, you need to verify the destination can hold the address or use the address len from the sockaddr you control.

A sa_in with sa_len set to 200000 would break this.

151

I think rlen should probably be called result, as it is used to store the result from the write call later. With this name you can use it for the result of the read call too.

l should be called either len or maybe msglen. More than a single charactar really helps people comprehend your intent.

175

with my above suggested change this would now read:

result = read(...)

240

star should be with the variable name like the line below.

When you have a line like:
oO oo = O0
you have a name problem. maybe chagne rtmsg_t to just msg or maybe something even longer like routemsg.

lib/libroute/libroute.h

61

the star here should be bound to the function name. This is the below for str_to_sockaddr and str_to_sockaddr6

70

I think this should be called str_to_sockaddr4

[ahsanb]

Add man page

[ahsanb]

Fix manpage by running igor on it
Fix style changes