unix: fix wrong MAC of uv_interface_address #1375

XadillaX · 2017-06-10T12:46:28Z

fix a wrong if in uv_interface_address about MAC.

if (ent->ifa_addr->sa_family != AF_LINK)

against

if (ent->ifa_addr->sa_family == AF_LINK)

Fixes: nodejs/node#13581
Refs: https://github.com/libuv/libuv/blob/v1.9.1/src/unix/freebsd.c#L418-L420
Refs: https://github.com/libuv/libuv/blob/v1.9.1/src/unix/darwin.c#L303-L305
Refs: ...

XadillaX · 2017-06-11T15:51:52Z

Anyone to review this PR?

/cc @cjihrig

santigimeno · 2017-06-12T13:00:59Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/289/

XadillaX · 2017-06-12T13:09:15Z

@santigimeno it seems failed?

bnoordhuis · 2017-06-12T13:11:34Z

This won't fix #829 and nodejs/node#13581 because that was about EUI-64 MAC addresses and it almost certainly can't be the fix for nodejs/node#5629 because the file you changed is only used on the BSDs, not Linux.

That aside, the fix looks okay but but you should be able to avoid duplicating uv__ifaddr_exclude() with some more effort, e.g. by passing a flag that says whether or not to include AF_LINK addresses.

XadillaX · 2017-06-12T13:21:52Z

For nodejs/node#13581, I think the wrong if returns the wrong result. Because Node.js 4.x is right which uses the right if.

This PR won't fix other 2 issues, but I think it may fix nodejs/node#13581.

bnoordhuis · 2017-06-12T13:26:40Z

The point stands though that bsd-ifaddrs.c is not used on Linux.

santigimeno · 2017-06-12T13:27:53Z

One more time: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/290/

XadillaX · 2017-06-12T13:29:45Z

sorry, I will update code later to fix linux.

XadillaX · 2017-06-13T14:27:31Z

@bnoordhuis I've updated the code.

bnoordhuis

A bit too much code duplication to my liking. Here is what you can do. Change:

static int uv__ifaddr_exclude(struct ifaddrs *ent) {

To:

static int uv__ifaddr_exclude(struct ifaddrs *ent, int skip_pf_packet) {

And update:

if (ent->ifa_addr->sa_family == PF_PACKET) return 1;

To:

if (ent->ifa_addr->sa_family == PF_PACKET) return skip_pf_packet;

And then update the call sites accordingly. Bonus points if you use a descriptive enum instead of a plain int.

XadillaX · 2017-06-14T14:33:32Z

@bnoordhuis But in bsd-ifaddr they are different.

#elif defined(__NetBSD__) || defined(__OpenBSD__)
  if (ent->ifa_addr->sa_family != PF_INET)
    return 1;
#endif

against

ent->ifa_addr->sa_family != AF_LINK

cjihrig · 2017-06-14T17:50:39Z

src/unix/bsd-ifaddrs.c

@@ -31,7 +31,12 @@
 #include <net/if_dl.h>
 #endif

-static int uv__ifaddr_exclude(struct ifaddrs *ent) {
+typedef enum {
+    UV__EXCLUDE_IFPHYS = 0,


Two space indentation please.

XadillaX · 2017-06-15T16:00:53Z

@bnoordhuis I've updated the code.

bnoordhuis · 2017-06-16T09:46:30Z

src/unix/bsd-ifaddrs.c

+typedef enum {
+  UV__EXCLUDE_IFPHYS = 0,
+  UV__EXCLUDE_IFADDR = 1
+} uv__exclude_type;


Not wrong but you don't need to explicitly assign values. As well, I wouldn't use a typedef, it obscures the type. You use it in more than one file so you can move it to src/unix/internal.h to avoid the duplication.

bnoordhuis · 2017-06-16T09:47:42Z

src/unix/bsd-ifaddrs.c

@@ -42,12 +47,13 @@ static int uv__ifaddr_exclude(struct ifaddrs *ent) {
   * devices.  We're not interested in this information.
   */
  if (ent->ifa_addr->sa_family == AF_LINK)
-    return 1;
+    return !!(int)type;


Just return type; (and consider renaming it to exclude_type.)

bnoordhuis · 2017-06-16T09:50:40Z

src/unix/bsd-ifaddrs.c

 #elif defined(__NetBSD__) || defined(__OpenBSD__)
-  if (ent->ifa_addr->sa_family != PF_INET)
+  if (type == UV__EXCLUDE_IFADDR && ent->ifa_addr->sa_family != PF_INET)
    return 1;


You can drop the check and just return !exclude_type; or return exclude_type == UV__EXCLUDE_IFADDR;.

bnoordhuis · 2017-06-16T09:50:43Z

src/unix/bsd-ifaddrs.c

    return 1;
+  return type == UV__EXCLUDE_IFADDR ? 0 : (ent->ifa_addr->sa_family != AF_LINK);


Libuv house style generally doesn't use ternaries.

XadillaX · 2017-06-16T14:05:42Z

@bnoordhuis Updated. 👌

XadillaX · 2017-06-18T10:30:00Z

/ping @bnoordhuis

bnoordhuis

Would it be possible to add some checks to e.g. test/test-platform-output.c?

bnoordhuis · 2017-06-19T11:01:41Z

src/unix/bsd-ifaddrs.c

  if (!((ent->ifa_flags & IFF_UP) && (ent->ifa_flags & IFF_RUNNING)))
    return 1;
  if (ent->ifa_addr == NULL)
    return 1;
+  /*
+   * Just see whether sa_family equals to AF_LINK or not if exclude_type is
+   * UV__EXCLUDE_IFPHYS


General style nit: please capitalize and punctuate comments.

bnoordhuis · 2017-06-19T11:14:47Z

src/unix/bsd-ifaddrs.c

+   * UV__EXCLUDE_IFPHYS
+   */
+  if (exclude_type == UV__EXCLUDE_IFPHYS)
+    return (ent->ifa_addr->sa_family != AF_LINK);
 #if defined(__APPLE__) || defined(__FreeBSD__) || defined(__DragonFly__)
  /*
   * On BSD getifaddrs returns information related to the raw underlying


Since you're assuming that AF_LINK is defined everywhere (and I believe it is), you could drop the #ifdef and reduce this to:

return (exclude_type == UV__EXCLUDE_IFPHYS) ^ (ent->ifa_addr->sa_family == AF_LINK);

I think?

What about the NetBSD/OpenBSD code path? Should it take exclude_type into account? Or just moved a few lines up, before the return?

If UV__EXCLUDE_IFPHYS, then only returns (ent->ifa_addr->sa_family != AF_LINK) on any system.

Otherwise, it depends on system. If it's OSX / FreeBSD / DragonFly then it depends on AF_LINK, or it depends on PF_INET.

bnoordhuis · 2017-06-19T11:15:20Z

src/unix/linux-core.c

-    return 1;
-  return 0;
+    return exclude_type; 
+  return !exclude_type;


https://github.com/libuv/libuv/blob/v1.9.1/src/unix/linux-core.c#L901-L912 against https://github.com/libuv/libuv/blob/v1.9.1/src/unix/linux-core.c#L935-L939

if exclude_type equals UV__EXCLUDE_IFPHYS then return ent->ifa_addr->sa_family == PF_PACKET, otherwise return ent->ifa_addr->sa_family != PF_PACKET.

XadillaX · 2017-06-19T11:37:00Z

I'll add some test cases then.

XadillaX · 2017-06-21T16:49:16Z

/ping @bnoordhuis

bnoordhuis

LGTM with comments and a question.

bnoordhuis · 2017-06-26T09:03:26Z

test/test-platform-output.c

    printf("  name: %s\n", interfaces[i].name);
    printf("  internal: %d\n", interfaces[i].is_internal);
    printf("  physical address: ");
-    printf("%02x:%02x:%02x:%02x:%02x:%02x\n",
+    sprintf(ether, "%02x:%02x:%02x:%02x:%02x:%02x",


Can you use snprintf() here?

bnoordhuis · 2017-06-26T09:05:02Z

test/test-platform-output.c

           (unsigned char)interfaces[i].phys_addr[0],
           (unsigned char)interfaces[i].phys_addr[1],
           (unsigned char)interfaces[i].phys_addr[2],
           (unsigned char)interfaces[i].phys_addr[3],
           (unsigned char)interfaces[i].phys_addr[4],
           (unsigned char)interfaces[i].phys_addr[5]);
+    printf("%s\n", ether);
+
+    if (strcmp(ether, "00:00:00:00:00:00") != 0)


Why check if it's all zeroes?

bnoordhuis · 2017-06-26T09:05:36Z

test/test-platform-output.c


    if (interfaces[i].address.address4.sin_family == AF_INET) {
      uv_ip4_name(&interfaces[i].address.address4, buffer, sizeof(buffer));
    } else if (interfaces[i].address.address4.sin_family == AF_INET6) {
      uv_ip6_name(&interfaces[i].address.address6, buffer, sizeof(buffer));
    }

-    printf("  address: %s\n", buffer);
+    printf("  address: %s\n", buffer); 


Whitespace change.

CurlyMoo · 2017-06-27T21:54:50Z

Tested and confirmed working.

bnoordhuis · 2017-06-30T23:07:34Z

Let's see how the CI fares: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/311/

EDIT: Still needs some work. Lots of red.

XadillaX · 2017-07-01T07:41:59Z

Anyone has some better test solution?

XadillaX · 2017-07-06T06:21:46Z

@bnoordhuis To be honest, I've no better idea than spawning a ifconfig. But the problem is, not every OS has the same behavior of spawning that.

santigimeno · 2017-07-06T07:52:00Z

@XadillaX using ip address should work on Linux. I modified your test locally and executed that command instead of ifconfig and the tests passed on my Linux box.

XadillaX · 2017-07-07T02:45:08Z

@santigimeno All kinds of Linux distributions?

By another way, ip address won't work under OSX. And I'm not sure if it's working under BSD, etc.

mattbrun · 2017-07-07T16:02:00Z

AFAIK ip is repalcing ifconfig and the other network utils - such as route, etc. - in Linux.
So for example ip address show enp3s0f1 in my case gives

2: enp3s0f1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP group default qlen 1000
    link/ether 70:4d:7b:3c:16:b0 brd ff:ff:ff:ff:ff:ff
    inet 192.168.1.116/24 brd 192.168.1.255 scope global dynamic enp3s0f1
       valid_lft 2774sec preferred_lft 2774sec
    inet6 fe80::4100:1991:751a:d1aa/64 scope link
       valid_lft forever preferred_lft forever

For now in Linux i've solved the issue shelling synchronously the command cat /sys/class/net/enp3s0f1/address, which returns exactly 70:4d:7b:3c:16:b0 without the need of parsing the output reported above.

bnoordhuis · 2017-07-07T16:05:01Z

@libuv/collaborators How about we land this without the test? I don't think it's important enough to agonize endlessly over an unreliable test.

XadillaX · 2017-07-07T16:07:14Z

+1 with @bnoordhuis

santigimeno · 2017-07-07T16:22:02Z

How about we land this without the test? I don't think it's important enough to agonize endlessly over an unreliable test.

👍

WRT the test, apart from the issue that making it work for every platform may not be easy, I think there's a problem in that the args element in the uv_process_options_t hasn't been properly initialized. By doing something like this the tests pass on most Linuxes and FreeBSD:

diff --git a/test/test-platform-output.c b/test/test-platform-output.c
index 5b94212..770a831 100644
--- a/test/test-platform-output.c
+++ b/test/test-platform-output.c
@@ -70,6 +70,7 @@ TEST_IMPL(platform_output) {
   int i;
   int r;
   int err;
+  char* args[2];
 
   err = uv_get_process_title(buffer, sizeof(buffer));
   ASSERT(err == 0);
@@ -145,9 +146,11 @@ TEST_IMPL(platform_output) {
 #ifdef _WIN32
   options.file = "ipconfig";
 #else
-  options.file = "ifconfig";
+  options.file = "/sbin/ifconfig";
 #endif
-  options.args = 0;
+  args[0] = "/sbin/ifconfig";
+  args[1] = NULL;
+  options.args = args;
   options.flags = 0;
   options.exit_cb = exit_cb;
   r = uv_spawn(uv_default_loop(), &process, &options);

CurlyMoo · 2017-07-07T18:17:39Z

Why don't we create a tap device with a self-defined hardware address?
http://backreference.org/2010/03/26/tuntap-interface-tutorial/
https://github.com/LaKabane/libtuntap

That last source code should work cross-platform (although not tested yet).

@libuv/collaborators How about we land this without the test? I don't think it's important enough to agonize endlessly over an unreliable test.

It is important for my goals to have proper mac address resolving, because i'm writing to raw sockets. Using the wrong MAC address disconnects the computer from the network my programs are running on (also how i discovered this bug myself). So, my goal is to use libuv as a shared library and therefor i need certain minimal guarantees.

If we are allowed to create tap devices on the test systems, then this could work well, and i can create a test script for it. What platforms should be supported with linux-core.c?

tap0 10.0.1.1 255.255.0.0 aa:bb:cc:dd:ee:ff

old wrong code:

tap0 10.0.1.1 255.255.0.0 00:00:00:00:a8:bb

saghul · 2017-07-08T08:45:01Z

src/unix/internal.h

@@ -145,6 +145,12 @@ enum {
  UV_LOOP_BLOCK_SIGPROF = 1
 };

+/* flags of excluding ifaddr */
+enum {


maybe use a typedef?

@saghul I used to use typedef. But I removed it later referred by #1375 (comment).

saghul · 2017-07-08T08:45:58Z

How about we land this without the test? I don't think it's important enough to agonize endlessly over an unreliable test.

Agreed.

XadillaX · 2017-07-09T13:10:05Z

/ping @bnoordhuis I've removed the test.

CurlyMoo · 2017-07-09T13:38:03Z

src/unix/linux-core.c

@@ -847,8 +847,8 @@ static int uv__ifaddr_exclude(struct ifaddrs *ent) {
   * devices. We're not interested in this information yet.
   */
  if (ent->ifa_addr->sa_family == PF_PACKET)
-    return 1;
-  return 0;
+    return exclude_type; 


There is a bogus trailing space here.

XadillaX · 2017-07-12T05:33:02Z

/ping @bnoordhuis

CurlyMoo · 2017-07-13T16:43:25Z

A general question about this function. Why do we care if a network device is running or up? Shouldn't we just be able to discover all available network devices?

 if (!((ent->ifa_flags & IFF_UP) && (ent->ifa_flags & IFF_RUNNING)))

I would say that we just make these flags parameters of the interface struct so the user can decide how to deal with inactive and down network devices. Or is this a cross-platform thing?

bnoordhuis · 2017-07-13T21:13:51Z

CI: https://ci.nodejs.org/job/libuv-test-commit/345/

CurlyMoo · 2017-07-16T11:37:51Z

@XadillaX the tests all succeeded (the failing ones weren't due to these changes). Can you finish your PR so it can be merged like squashing and a good commit message.

XadillaX · 2017-07-16T11:42:19Z

@CurlyMoo I will rebase it after going back home, now i'm on a train.

fix a wrong `if` in `uv_interface_address` about MAC. Fixes: nodejs/node#13581

XadillaX · 2017-07-17T02:59:25Z

/ping @CurlyMoo, /cc @bnoordhuis I've rebased the code.

bnoordhuis · 2017-07-17T09:18:08Z

https://ci.nodejs.org/job/libuv-test-commit/347/

fix a wrong `if` in `uv_interface_address` about MAC. Fixes: nodejs/node#13581 PR-URL: #1375 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

bnoordhuis · 2017-07-17T11:47:36Z

Landed in f1e0fc4, thanks @XadillaX.

cwalther · 2017-08-16T07:05:17Z

It seems like this should also go into master, not just v1.x? (I haven’t tested it, but I was just reading the master code and thinking “how on earth is this supposed to work?”)

santigimeno · 2017-08-16T07:34:16Z

No worries, we merge v1.x changes into master periodically, so this change will end up in master too.

refack mentioned this pull request Jun 10, 2017

os.networkInterfaces() returns wrong result nodejs/node#5629

Closed

santigimeno approved these changes Jun 12, 2017

View reviewed changes

bnoordhuis requested changes Jun 14, 2017

View reviewed changes

XadillaX force-pushed the fix/uv_interface_addresses_bug branch from c204801 to edd7e40 Compare June 14, 2017 14:41

cjihrig reviewed Jun 14, 2017

View reviewed changes

bnoordhuis requested changes Jun 16, 2017

View reviewed changes

XadillaX force-pushed the fix/uv_interface_addresses_bug branch from 0e194c7 to 360accf Compare June 16, 2017 14:03

bnoordhuis requested changes Jun 19, 2017

View reviewed changes

XadillaX mentioned this pull request Jun 20, 2017

os.networkInterfaces() returns wrong MAC addresses nodejs/node#13581

Closed

bnoordhuis mentioned this pull request Jun 25, 2017

[1.12.0] [linux] uv_interface_addresses failed to get right physical address #1384

Closed

bnoordhuis approved these changes Jun 26, 2017

View reviewed changes

santigimeno mentioned this pull request Jun 27, 2017

unix: factor out reusable BSD ifaddrs implementation #1240

Closed

saghul reviewed Jul 8, 2017

View reviewed changes

CurlyMoo mentioned this pull request Jul 9, 2017

Test for interface mac, ip, and subnet discovery #1412

Closed

CurlyMoo reviewed Jul 9, 2017

View reviewed changes

CurlyMoo approved these changes Jul 16, 2017

View reviewed changes

unix: fix wrong MAC of uv_interface_address

f34d7e2

fix a wrong `if` in `uv_interface_address` about MAC. Fixes: nodejs/node#13581

XadillaX force-pushed the fix/uv_interface_addresses_bug branch from d78164b to f34d7e2 Compare July 17, 2017 02:59

bnoordhuis closed this Jul 17, 2017

XadillaX mentioned this pull request Apr 9, 2019

os.networkInterfaces() returns wrong mac addresses again! nodejs/node#27139

Closed

RaisinTen mentioned this pull request Nov 2, 2020

unix: replace ternary with if...else #3032

Closed

		return 1;
		return type == UV__EXCLUDE_IFADDR ? 0 : (ent->ifa_addr->sa_family != AF_LINK);

unix: fix wrong MAC of uv_interface_address #1375

unix: fix wrong MAC of uv_interface_address #1375

Conversation

XadillaX commented Jun 10, 2017 • edited Loading

XadillaX commented Jun 11, 2017 • edited Loading

santigimeno commented Jun 12, 2017

XadillaX commented Jun 12, 2017

bnoordhuis commented Jun 12, 2017

XadillaX commented Jun 12, 2017 • edited Loading

bnoordhuis commented Jun 12, 2017

santigimeno commented Jun 12, 2017

XadillaX commented Jun 12, 2017

XadillaX commented Jun 13, 2017

bnoordhuis left a comment • edited Loading

Choose a reason for hiding this comment

XadillaX commented Jun 14, 2017

Choose a reason for hiding this comment

XadillaX commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX commented Jun 16, 2017 • edited Loading

XadillaX commented Jun 18, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

XadillaX commented Jun 19, 2017

XadillaX commented Jun 21, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CurlyMoo commented Jun 27, 2017

bnoordhuis commented Jun 30, 2017 • edited Loading

XadillaX commented Jul 1, 2017

XadillaX commented Jul 6, 2017

santigimeno commented Jul 6, 2017

XadillaX commented Jul 7, 2017

mattbrun commented Jul 7, 2017 • edited Loading

bnoordhuis commented Jul 7, 2017

XadillaX commented Jul 7, 2017

santigimeno commented Jul 7, 2017

CurlyMoo commented Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

XadillaX Jul 9, 2017 • edited Loading

Choose a reason for hiding this comment

saghul commented Jul 8, 2017

XadillaX commented Jul 9, 2017

Choose a reason for hiding this comment

XadillaX commented Jul 12, 2017

CurlyMoo commented Jul 13, 2017

bnoordhuis commented Jul 13, 2017

CurlyMoo commented Jul 16, 2017

XadillaX commented Jul 16, 2017

XadillaX commented Jul 17, 2017

bnoordhuis commented Jul 17, 2017

bnoordhuis commented Jul 17, 2017

cwalther commented Aug 16, 2017

santigimeno commented Aug 16, 2017

XadillaX commented Jun 10, 2017 •

edited

Loading

XadillaX commented Jun 11, 2017 •

edited

Loading

XadillaX commented Jun 12, 2017 •

edited

Loading

bnoordhuis left a comment •

edited

Loading

XadillaX commented Jun 16, 2017 •

edited

Loading

XadillaX Jun 19, 2017 •

edited

Loading

XadillaX Jun 19, 2017 •

edited

Loading

bnoordhuis commented Jun 30, 2017 •

edited

Loading

mattbrun commented Jul 7, 2017 •

edited

Loading

CurlyMoo commented Jul 7, 2017 •

edited

Loading

XadillaX Jul 9, 2017 •

edited

Loading