#27325: Rework NETINFO wire format handling to rely on trunnel #370

rl1987 · 2018-09-22T17:17:25Z

https://trac.torproject.org/projects/tor/ticket/27325

coveralls · 2018-09-22T17:35:36Z

Pull Request Test Coverage Report for Build 2610

0 of 65 (0.0%) changed or added relevant lines in 2 files are covered.
11953 unchanged lines in 53 files lost coverage.
Overall coverage increased (+0.08%) to 62.009%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/core/or/connection_or.c	0	30	0.0%
src/core/or/channeltls.c	0	35	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/core/or/or.h	1	88.89%
src/core/or/channelpadding.c	5	97.27%
src/app/config/confparse.c	5	80.36%
src/lib/tls/tortls.c	6	96.55%
src/lib/container/order.h	6	0.0%
src/lib/evloop/procmon.c	7	76.74%
src/core/or/status.c	7	86.87%
src/feature/nodelist/torcert.c	14	95.22%
src/core/proto/proto_socks.c	14	92.74%
src/feature/control/fmt_serverstatus.c	18	0.0%

Totals
Change from base Build 2312:	0.08%
Covered Lines:	44089
Relevant Lines:	71101

💛 - Coveralls

dgoulet-tor · 2018-10-23T15:12:16Z

src/trunnel/netinfo.trunnel

+const NETINFO_ADDR_TYPE_IPV6 = 6;
+
+struct netinfo_addr {
+  u8 addr_type IN [NETINFO_ADDR_TYPE_IPV4, NETINFO_ADDR_TYPE_IPV6];


Hmmm is it possible this is missing 3 values by any chance (looking at tor-spec.txt):

"Type" is one of: 0x00 -- Hostname 0x04 -- IPv4 address 0x06 -- IPv6 address 0xF0 -- Error, transient 0xF1 -- Error, nontransient

Seems also that the are mapped from RESOLVED_TYPE_* values. We should put a strong comment to keep them insync.

The old code did not use any address type values other 4 and 6. I suppose they cannot realistically appear in a valid NETINFO cell during handshake?

Added a comment in 9cd116b.

Code doesn't use it but doesn't mean the protocol doesn't allow it.

This specific "netinfo addr" structure actually should be generalized (and probably put outside the netinfo context) because it is also a structure that is used in the RELAY_RESOLVED cell so all type need to be there.

It just happens that the NETINFO cell will only care about two of those values.

dgoulet-tor · 2018-10-23T15:23:37Z

src/core/or/channeltls.c


+  netinfo_addr_t *my_addr = netinfo_cell_get_other_addr(netinfo_cell);


Can we use the const getter here you think?

Yes. Did so in b0a8cf0.

dgoulet-tor · 2018-10-23T15:26:47Z

src/core/or/channeltls.c

  if (my_addr_type == RESOLVED_TYPE_IPV4 && my_addr_len == 4) {
-    tor_addr_from_ipv4n(&my_apparent_addr, get_uint32(my_addr_ptr));
+    uint32_t ipv4 = netinfo_addr_get_addr_ipv4(my_addr);


I believe the ipv4 will be network order here so I would try to reflect that in the name of the variable because we'll get confused really fast :S

Actually, why do we need to do that if we have the address already in my_apparent_addr ? Can't we simply compare that with me->addr ?

Trunnel does change between endianness automatically - see netinfo_addr_encode() and netinfo_addr_parse_into().

Simplified the code to rely on my_apparent_addr in 92ada4d.

dgoulet-tor · 2018-10-23T15:31:37Z

src/core/or/channeltls.c

-  n_other_addrs = (uint8_t) *cp++;
-  while (n_other_addrs && cp < end-2) {
+  n_other_addrs = netinfo_cell_get_n_my_addrs(netinfo_cell);
+  for (size_t i = 0; i < n_other_addrs; i++) {


size_t i --> uint8_t i. I say we match the type with our top limit n_other_addrs.

Fixed in 1ae0ab8.

dgoulet-tor · 2018-10-23T15:36:45Z

src/core/or/connection_or.c

+  netinfo_addr_t *netinfo_addr = netinfo_addr_new();
+
+  if (addr_family == AF_INET) {
+    netinfo_addr_set_addr_type(netinfo_addr, NETINFO_ADDR_TYPE_IPV4);


When decoding, we used RESOLVED_TYPE_..., here the NETINFO_... are used. Whichever we use, we should simply be consistent and stick to use one.

In b195438, I fixed parsing code to use macros from netinfo.trunnel.

dgoulet-tor · 2018-10-23T15:38:32Z

src/core/or/connection_or.c

+    uint8_t *ipv6_buf = netinfo_addr_getarray_addr_ipv6(netinfo_addr);
+    // XXX: this looks sketchy
+    memcpy(ipv6_buf, tor_addr->addr.in6_addr.s6_addr, 16);
+    // XXX: can we avoid memcpy here?


The HS subsystem does it like this:

const uint8_t *in6_addr = tor_addr_to_in6_addr8(&ap.addr); uint8_t *ipv6_array = link_specifier_getarray_un_ipv6_addr(ls); memcpy(ipv6_array, in6_addr, addr_len);

You can probably ignore addr_len and instead use 16.

Removed the comments in 216880f and used the the helper function in 6867bea.

dgoulet-tor · 2018-10-23T15:40:55Z

src/core/or/connection_or.c

  /* Timestamp, if we're a relay. */
  if (public_server_mode(get_options()) || ! conn->is_outgoing)
-    set_uint32(cell.payload, htonl((uint32_t)now));
+    netinfo_cell_set_timestamp(netinfo_cell, (uint32_t)now);


You sure you can remove the htonl() ? Trunnel won't magically switch it :S... Spec specifies:

The timestamp is a big-endian unsigned integer number of seconds since the Unix epoch.

Yes, as trunnel changes the endianness for us:

597 /* Encode u32 timestamp */ 598 trunnel_assert(written <= avail); 599 if (avail - written < 4) 600 goto truncated; 601 trunnel_set_uint32(ptr, trunnel_htonl(obj->timestamp)); 602 written += 4; ptr += 4;

dgoulet-tor · 2018-10-23T15:43:36Z

src/core/or/connection_or.c

+                    errmsg);
+    r = -1;
+    goto cleanup;
+  }


I think it is OK to remove this check because the encoding function does call netinfo_cell_check() and -1 is returned if the check fails. -2 is the allocation failed.

But we do lose the error message if we rely on _encode() calling _check().

While calling _check() in here is not strictly necessary, it may prove useful in case struct gets populated incorrectly. I would say let's leave it here just to be safe and make debugging easier.

dgoulet-tor · 2018-10-23T15:45:21Z

src/core/or/connection_or.c

@@ -2434,8 +2461,7 @@ connection_or_send_netinfo,(or_connection_t *conn))
  cell_t cell;
  time_t now = time(NULL);
  const routerinfo_t *me;
-  int len;
-  uint8_t *out;
+  int r = 0;


A trick that I like is set this to -1 so then every goto will have the right error value set. Then if you reach the cleanup: label, just before you set r = 0. This way, any code addition later, we avoid the error of someone forgetting to set r within the error path.

Did so in e9b6b8a.

rl1987 added 4 commits September 22, 2018 19:52

Add trunnel spec and impl for NETINFO cells

79951a0

Use trunnel for NETINFO cell parsing

ff5d493

Generate NETINFO cell using trunnel

d6e52ce

Add changes file

e15ca9b

dgoulet-tor reviewed Oct 23, 2018

View reviewed changes

rl1987 added 8 commits October 26, 2018 11:30

Add a comment about address type field to netinfo.trunnel

9cd116b

fixup! Use trunnel for NETINFO cell parsing

b0a8cf0

fixup! Use trunnel for NETINFO cell parsing

92ada4d

fixup! Use trunnel for NETINFO cell parsing

1ae0ab8

fixup! Use trunnel for NETINFO cell parsing

b195438

fixup! Generate NETINFO cell using trunnel

216880f

fixup! Generate NETINFO cell using trunnel

6867bea

fixup! Generate NETINFO cell using trunnel

e9b6b8a

ghost closed this May 25, 2021

ghost deleted the branch torproject:master May 25, 2021 12:55

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#27325: Rework NETINFO wire format handling to rely on trunnel #370

#27325: Rework NETINFO wire format handling to rely on trunnel #370

rl1987 commented Sep 22, 2018

coveralls commented Sep 22, 2018 •

edited

Loading

dgoulet-tor Oct 23, 2018

dgoulet-tor Oct 23, 2018

rl1987 Oct 26, 2018

dgoulet-tor Oct 29, 2018

dgoulet-tor Oct 23, 2018

rl1987 Oct 26, 2018

dgoulet-tor Oct 23, 2018

dgoulet-tor Oct 23, 2018

rl1987 Oct 26, 2018

dgoulet-tor Oct 23, 2018

rl1987 Oct 26, 2018

dgoulet-tor Oct 23, 2018

rl1987 Oct 26, 2018

dgoulet-tor Oct 23, 2018

rl1987 Oct 26, 2018

dgoulet-tor Oct 23, 2018

rl1987 Oct 26, 2018

dgoulet-tor Oct 29, 2018

dgoulet-tor Oct 23, 2018

rl1987 Oct 26, 2018

dgoulet-tor Oct 29, 2018

dgoulet-tor Oct 23, 2018

rl1987 Oct 26, 2018


		netinfo_addr_t *my_addr = netinfo_cell_get_other_addr(netinfo_cell);

#27325: Rework NETINFO wire format handling to rely on trunnel #370

#27325: Rework NETINFO wire format handling to rely on trunnel #370

Conversation

rl1987 commented Sep 22, 2018

coveralls commented Sep 22, 2018 • edited Loading

Pull Request Test Coverage Report for Build 2610

💛 - Coveralls

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 22, 2018 •

edited

Loading