the command 'ifconfig epair2a link 10.1.2.200/28' has a somewhat
surprising result:
% ifconfig epair2a link 10.1.2.200/28
% ifconfig epair2a
epair2a: flags=1008842<BROADCAST,RUNNING,SIMPLEX,MULTICAST,LOWER_UP> metric 0 mtu 1500
options=8<VLAN_MTU>
ether 10:01:02:20:00:28
hwaddr 02:25:24:d4:38:0a
this is because link_addr(3) considers any unrecognised character, such
as '.' or '/', to be a delimiter in the Ethernet address. usually this
wouldn't be a significant problem, because people should just not do
this.
however, consider a system which is IPv6-only, i.e., the kernel supports
INET6 but not INET. in this case ifconfig(8) defaults to the 'link'
address family instead of the 'inet' address family. then suppose the
user has an existing ifconfig_xxx entry in /etc/rc.conf:
ifconfig_hn0="10.1.2.200/28"
because ifconfig defaults to 'link', link_addr will parse the IP address
as a link address, and ifconfig will change the Ethernet address of the
interface, possibly breaking networking.
to fix this, change link_addr() to be able to indicate an error by
returning int instead of void. return an error if the input is invalid
for any reason, such as:
* using delimiters other than '-', ':' or '.'
* using more than one different delimiter (as in the original example)
* overflowing the output buffer
the choice of [:.-] for delimiter was made to support the "native"
link_addr format (dots) as well as Ethernet addresses (dashes or
colons).
for compatibility with existing callers, we make some attempt to
replicate link_addr()'s existing behaviour if the caller ignores the
error return, but this might break some edge cases.
bump __FreeBSD_version so link_addr() consumers can detect the change.Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 63609 Build 60493: arc lint + arc unit
Event Timeline
Comment Actions
i am not 100% sure about this one because i suspect link_addr/sockaddr_dl may be used in ways i'm not aware of. however, it does fix the original issue that i ran into, which was extremely confusing.
Comment Actions
in this case ifconfig(8) defaults to the 'link' address family instead of the 'inet' address family.
I wonder if that's worth changing too. My gut reaction here is that for an INET6-only build we ought to be defaulting to inet6, rather than to link. Or possibly defaulting to nothing at all, because defaulting to 'inet' has been causing headaches for years. We've not wanted to change it because there are any number of rc.conf files out there that rely on that, but that's less likely to be the case for the INET6-only build (especially since relying on it changing the link address is much more obviously the wrong thing to do).
| lib/libc/net/linkaddr.c | ||
|---|---|---|
| 72 | Should we even be accepting '.' as a MAC address delimiter? | |
| 75 | style(9): return (-1); | |
| sys/sys/param.h | ||
| 76 | I don't know if this is significant enough to bump the version, but on the other hand, numbers are cheap and we buy them in bulk. It may be worth noting this in RELNOTES though. (There too it's probably better to err on the side of including edge cases. The people who write the release notes can still decide not to mention it later.) | |
| lib/libc/net/linkaddr.c | ||
|---|---|---|
| 72 | i believe this original code was intended to accept MAC addresses in the form '0011.2233.4455', in which case, yes. | |
| sys/sys/param.h | ||
| 76 | i did this so code could use __FreeBSD_version to check whether it could check for a return value from link_addr(). in practice i don't know how useful that is, but... | |
Comment Actions
so... i'm not opposed to changing that. actually, i think that makes a lot of sense. however, i think this change to link_addr() also makes sense regardless of that.
Comment Actions
I would suggest also accepting dashes as separators. I've seen plenty of software that displays Ethernet addresses with dashes instead of colons, which raises the possibility that someone might be using them here as well, but they don't show up in IPv4 or IPv6 addresses, so there's minimal risk of confusion.
| sys/sys/param.h | ||
|---|---|---|
| 76 |
Definitely. Lexi, when you get that far, please include Relnotes: yes in the commit footer. | |
Comment Actions
if we're going to support ':', '-' and '.', i wonder if it makes sense to change this so only one delimiter is accepted, so 00-11-22-33-44-55 is fine, but 00-11-22:33:44:55 is rejected.
or, we could be even more strict and simply define a list of supported formats:
- 0000.0000.0000
- 00:00:00:00:00:00
- 00-00-00-00-00-00
theses are the only three ways i've ever seen an Ethernet address written, and while i'm sure others exist, i doubt they are in significant use among FreeBSD users.
Comment Actions
To estimate usage of link_addr(3) in external applications you can create a branch where link_addr(3) is removed from libc (the world still needs to be compilable, so you'd need to add a copy of link_addr() to ifconfig and others), then request an exp-run from the ports team. That will highlight all the ports that use this API. It could be a very small list actually.
Comment Actions
Wow. I’m surprised that this kind of bug exists in this libcall..
Would it make sense to define ERRORs to include errno’s of some kind, e.g., EINVAL?
Comment Actions
i did consider this and decided not to set errno since input is either correct or wrong, but on reflection, there's no downside to setting errno and it might help in the future, so i will do this when i update this diff.
Comment Actions
new version of link_addr().
this is essentially a rewrite of the existing code. it's a bit longer, but
that's mainly because it now has comments and error handling; i think it's
cognitively less complicated overall.
i decided on accepting -, : and . for delimiter, but limiting this to a
single delimiter. i'm open to being more or less strict here if there's
consensus on that, but i think this is a reasonable compromise between fixing
the original problem, and not inhibiting valid uses of link_addr() such as
non-Ethernet addresses.
this is rebased against the recent if_dl.h/link_addr_test changes, and i've
included both positive and negative tests for the new functionality.
Comment Actions
update __FreeBSD_version. this got lost during the rebase.
(this is so people can detect the new version of link_addr in third party code.)
Comment Actions
There seem to be very few uses of link_addr() in the tree, it would be great if we could add error checking to them (in a separate review) now that it can return an error.
| lib/libc/net/linkaddr.c | ||
|---|---|---|
| 55 | this should be last | |
| 111 | What if addr is just a colon-separated MAC address? Unfortunately ae10, cc10 and dc10 are both valid interface names and valid hex strings... | |
| 116 | I would check for a second digit first, _then_ check for a delimiter or NUL. | |
| 117 | ||
| 147 | ||
| 148 | ||
| 158 | ||
| lib/libc/net/linkaddr.c | ||
|---|---|---|
| 111 | then you have to include the leading colon: link_addr(":ae:22:33:44:55:66"). it's intended to be used with . as the delimiter, so i think using link_addr("ae.22.33.44.55.66") would also work, but it's better to just put the colon there. yes, this is a rather odd API. | |
| lib/libc/net/linkaddr.c | ||
|---|---|---|
| 111 | to be clear, there should be no functional change here, you already had to include the colon and ifconfig for example does this itself. so it's not like we're now forcing people to add a colon in front of all their Ethernet addresses. | |
| lib/libc/net/linkaddr.c | ||
|---|---|---|
| 116 | i've reorganised this in a way that's hopefully better. | |
| lib/libc/net/linkaddr.c | ||
|---|---|---|
| 116 | Yes, but you missed the point of doing so. You are now repeating *cp++ = digit; in all cases except the error case. You could just to that unconditionally, then check for a delimiter or NUL, then you're done. In the error case, the next loop iteration will not find a digit and error out. | |
| lib/libc/net/linkaddr.c | ||
|---|---|---|
| 116 | I should add: it means you _sometimes_ store a bogus byte before returning an error, but you save a lot of code, and you've already documented that the array contents are indeterminate in case of error. | |
| lib/libc/net/linkaddr.c | ||
|---|---|---|
| 116 | ...you don't even need to check for a delimiter, because you consume delimiters at the top of the loop. Just check for NUL. And you can do that by rewriting the loop as do { ... } while (*addr != '\0') (or even while (*addr != '\0') { ... }). | |
| lib/libc/net/linkaddr.c | ||
|---|---|---|
| 116 |
no, we do, because we need to remove one delimiter here without incrementing cp, while the delimiter check at the top of the loop adds *cp++ = 0 for each delimiter it finds, to handle addresses like 1::3:4:5:6. i understand your point but i find doing *cp++ = ... then undoing it (or leaving garbage) a bit ugly, i'm thinking about a better way to do this. | |