uptime mod - fix structhack and sanitize struct data types.

[michaelortmann]

Found by: wakco
Patch by: michaelortmann
Fixes: #377 and #664

One-line summary:
Fix structhack with sane code, sanitize struct data types and replace rand() with random()

Additional description (if needed):
This patch requires merged #660!
Side effect of new code is better network usage, due to proper ending of the send network packet with one \0 instead of useless additional \0s.
Datatypes within struct are result of
uint32_t htonl(uint32_t hostlong);
(see man htonl) and the server understands 32 and 64 bit.
So the struct was sanitized and uses fixed datatype uint32_t now.
While at it, i replaced rand() with random(), so eggdrop will only uses one random function everywhere.
Some comments were cleaned up regarding line length.
tab -> space
gethostbyname() -> getaddrinfo() to prepare for ipv6 ;) and modernize network code
static functions
tcpdump test below...

Test cases demonstrating functionality (if applicable):

eggdrop.conf:

  loadmodule uptime
  set nick "fix377"

i modified the code to test this:
uptime.c:
before:
static int update_interval = 720;
after:
static int update_interval = 1;

uptime.c:
before:

      len = sendto(uptimesock, (void *) mem, len, 0, (struct sockaddr *) &sai,   
                       sizeof(sai));

after:

      FILE *fil;
      fil = fopen("/home/michael/eggdrop/<before|after>.dat", "wb");
      fwrite(mem, 1, len, fil);
      fclose(fil);

hexdump -C before.dat

00000000  00 00 00 00 00 00 6b 88  00 00 00 02 00 00 00 00  |......k.........|
00000010  00 00 00 02 00 00 00 00  5b 71 b4 6a 00 00 00 00  |........[q.j....|
00000020  5b 71 b4 6b 00 00 00 00  5b 71 b5 0c 00 00 00 00  |[q.k....[q......|
00000030  5b 6c 1f 32 00 00 00 00  66 69 78 33 37 37 20 31  |[l.2....fix377 1|
00000040  32 37 2e 30 2e 30 2e 31  20 76 31 2e 38 2e 33 2b  |27.0.0.1 v1.8.3+|
00000050  61 72 63 68 64 65 74 65  63 74 00 00 00 00 00 00  |archdetect......|
00000060

hexdump -C after.dat

00000000  00 00 00 00 00 00 02 55  00 00 00 02 00 00 00 01  |.......U........|
00000010  5b 71 c9 78 5b 71 c9 79  5b 71 ca 13 5b 6c 1f 32  |[q.x[q.y[q..[l.2|
00000020  66 69 78 33 37 37 20 31  32 37 2e 30 2e 30 2e 31  |fix377 127.0.0.1|
00000030  20 76 31 2e 38 2e 33 2b  61 72 63 68 64 65 74 65  | v1.8.3+archdete|
00000040  63 74 00                                          |ct.|
00000043

20181014 tcpdump of the first sent packets:

$ tcpdump -vvvnX -s0 dst port 9969 --dont-verify-checksums
tcpdump: listening on enp31s0, link-type EN10MB (Ethernet), capture size 262144 bytes
10:11:47.690494 IP (tos 0x0, ttl 64, id 29078, offset 0, flags [DF], proto UDP (17), length 94)
    192.168.0.3.54501 > 38.109.218.218.9969: UDP, length 66
	0x0000:  4500 005e 7196 4000 4011 0706 c0a8 0003  E..^q.@.@.......
	0x0010:  266d dada d4e5 26f1 004a c24e 0000 0000  &m....&..J.N....
	0x0020:  0000 7091 0000 0002 0000 0001 5bc2 94ee  ..p.........[...
	0x0030:  5bc2 94ef 5bc2 fa43 5bb9 2507 6669 7833  [...[..C[.%.fix3
	0x0040:  3737 2031 3237 2e30 2e30 2e31 2076 312e  77.127.0.0.1.v1.
	0x0050:  382e 332b 6472 6167 6f6e 666c 7900       8.3+dragonfly.
14:03:47.616624 IP (tos 0x0, ttl 64, id 61114, offset 0, flags [DF], proto UDP (17), length 94)
    192.168.0.3.54501 > 38.109.218.218.9969: UDP, length 66
	0x0000:  4500 005e eeba 4000 4011 89e1 c0a8 0003  E..^..@.@.......
	0x0010:  266d dada d4e5 26f1 004a c24e 0000 0000  &m....&..J.N....
	0x0020:  0000 7091 0000 0002 0000 0002 5bc2 94ee  ..p.........[...
	0x0030:  5bc2 94ef 5bc3 30a3 5bb9 2507 6669 7833  [...[.0.[.%.fix3
	0x0040:  3737 2031 3237 2e30 2e30 2e31 2076 312e  77.127.0.0.1.v1.
	0x0050:  382e 332b 6472 6167 6f6e 666c 7900       8.3+dragonfly.
16:25:21.577218 IP (tos 0x0, ttl 64, id 16405, offset 0, flags [DF], proto UDP (17), length 94)
    192.168.0.3.54501 > 38.109.218.218.9969: UDP, length 66
	0x0000:  4500 005e 4015 4000 4011 3887 c0a8 0003  E..^@.@[email protected].....
	0x0010:  266d dada d4e5 26f1 004a c24e 0000 0000  &m....&..J.N....
	0x0020:  0000 7091 0000 0002 0000 0003 5bc2 94ee  ..p.........[...
	0x0030:  5bc2 94ef 5bc3 51d1 5bb9 2507 6669 7833  [...[.Q.[.%.fix3
	0x0040:  3737 2031 3237 2e30 2e30 2e31 2076 312e  77.127.0.0.1.v1.
	0x0050:  382e 332b 6472 6167 6f6e 666c 7900       8.3+dragonfly.

[thommey]

want to test this with the server

[michaelortmann]

@thommey: can i do anything to help with the test?

[vanosg]

Can you run it on a bot for a few days? We'll tell you if the server explodes :)

[thommey]

the memory allocation for flexible array members are apparently subject to some uncertainty regarding offsetof() vs. sizeof(). sizeof can include padding bytes after the last non-vla member, while offsetof does not. so, an actual non-vla member can require a smaller size than sizeof(struct x)+strlen(vla) calculates.

I mostly switched to omitting the member and just sending the packed struct as header and then the rest. with attribute((packed)), which we should be using anyway, this problem might go away on its own. it wouldn't actually be a problem to send a packet too long, the last element is a string and \0 terminated. so we can most likely just leave it like you have it now

see https://stackoverflow.com/questions/44745677/flexible-array-members-can-lead-to-undefined-behavior and similar for discussion

[michaelortmann]

Uncertainty regarding offsetof vs. sizeof can be treated by using them correctly, so whatever stackoverflow sais:

offsetof <= sizeof regarding any struct is obviously true, because offsetof is only equal to sizeof, if the last element is of ZERO size, and there is uncertainty about the size of an vla.

I enhanced our code to use offsetof instead of sizeof.

By "I mostly switched" you mean, you unstruckhacked a structure into two parts? One can always do that. Its a question of style preference.

Yes, packing should be used for network packets. But only for network packets. It hurts performance. There are other use cases, embedded systems, memory limits, but that would be off topic now.

I was already looking at padding/packing before, because whenever a struct is shared, via network or file or whatever, we have a portability issue. The code worked, so from that alone one can tell that there can't be much padding/packing. It would break the network. Ok, its not absolutely true. It broke for 32 vs. 64 bit because of uncertainty about integer size and some padding bytes at the very end of the struct. That's what the old workarounds were about. Now my patch fixes this. It removes any uncertainty about member sizes of the struct, and also about padding/packing, because all members are same certain size each.

Normally you would have a hard requirement for pack here, as you wrote, but in this case, we are clear without it now.

[michaelortmann]

Testbot is running since 3 days, see:
https://www.eggheads.org/irc/uptime_lookup/9fc6e170ef2b9c835f1140a32b7a52a8

The tcpdump of the first packets is shown above, so i don't expect any surprize.

If you don't need this testbot any longer, let me know pls, i could use system resources elsewhere, ty!

Should we bump this version number?
module_register(MODULE_NAME, uptime_table, 1, 4);

[thommey]

if we clean this up, I'd like a packed structure and using the unused "regnr" to identify "new version" packets, maybe simply make it a version field. it should be 0 for all eggdrops, not sure about energymechs, would need to be evaluated (easiest in the actual server probably)

[thommey]

also needs a server update then, but at least we have defined sizes then

[michaelortmann]

Note to my future self: Maybe we should not do the nmalloc / memcpy(mem, upPack) nor use flexible array member here, but simply set PackUp char string to the maximum size possible, which is not much, probably below 512 bytes. Its a good tradeoff for the simplification in sourcecode that we get out of this plus the nmalloc/memcopy was not for free also.