addrwatch: Various fixes

[jefferyto]

Maintainer: @oskar456
Compile tested: armvirt-32, 2021-06-17 snapshot sdk
Run tested: armvirt-32 (qemu), 2021-06-17 snapshot

Description:
Makefile changes include:

• Remove USE_UCLIBC, as uclibc is no longer supported

• Package output modules

• Move main binary (back) to /usr/sbin, as it is system administration related and requires superuser privileges

New patches:

• 003-add-space-for-null-byte.patch - from fln/addrwatch@374cfd2

• 004-more-specific-library-linking.patch - from fln/addrwatch@27b57d9

• 005-use-c99-format-macro-constants.patch - from Use C99 format macro constants for timestamp and vlan_tag fln/addrwatch#28

Init script changes include:

• Change from explicit disable to explicit enable, so that the service is disabled by default and on first install

• Set config option default values to default values of the main binary

• Fix command-line option names and format (from https://forum.openwrt.org/t/cant-start-addrwatch-service/60499/3)

• Always use the --quiet command-line option, as the procd instance is not configured to capture stdout/stderr

• Change the syslog config option to start the syslog output module

Signed-off-by: Jeffery To jeffery.to@gmail.com

[jefferyto]

There are still some issues with this package. When I enable both file and syslog output, I see (what looks like) correct output in the log file:

1624004401 eth1 0 52:54:00:12:34:57 10.0.2.15 ARP_REQ
1624004401 eth1 0 52:55:0a:00:02:02 10.0.2.2 ARP_REP
1624004433 eth1 0 52:54:00:12:34:57 10.0.2.15 ARP_REQ
1624004433 eth1 0 52:55:0a:00:02:02 10.0.2.2 ARP_REP

But in syslog I see this:

Fri Jun 18 08:20:01 2021 daemon.info addrwatch: 1624004401 (null) 3069243648 (null) 52:54:00:12:34:57 10.0.2.15
Fri Jun 18 08:20:01 2021 daemon.info addrwatch: 1624004401 (null) 3069243704 (null) 52:55:0a:00:02:02 10.0.2.2
Fri Jun 18 08:20:33 2021 daemon.info addrwatch: 1624004433 (null) 3069243760 (null) 52:54:00:12:34:57 10.0.2.15
Fri Jun 18 08:20:33 2021 daemon.info addrwatch: 1624004433 (null) 3069243816 (null) 52:55:0a:00:02:02 10.0.2.2

Some of the values (interface name, mac address) are missing (replaced with (null)) and the vlan tag value (3069243648, 3069243704, etc.) appears to be incorrect.

Also, if I run addrwatch_stdout at the command line (while addrwatch is running), it will segfault at the first update.

@neheb would you know what is going on / how to fix these? I thought I should ask you before opening an issue upstream 😂

[oskar456]

Great work, @jefferyto, thank you!
Do you want to become a maintainer of this package? Feel free to replace me.

[neheb]

I think upstream is the best place to do that. The issue is a null pointer passed to printf. Would need to look at the code to see what’s going on. My guess is upstream only tests glibc.

[jefferyto]

@oskar456 thanks but I already have too much on my plate to maintain another package (plus I don't actually use addrwatch lol)

@neheb I was hoping you could help track down the issue since you are much more familiar with C than I am 😂 I will open an issue upstream a little later

[neheb]

I think the file is saved using: https://github.com/fln/addrwatch/blob/master/src/output_flatfile.c#L25 and https://github.com/fln/addrwatch/blob/master/src/addrwatch_syslog.c#L21 for syslog.

https://github.com/fln/addrwatch/blob/master/src/output_shm.c#L57 is where timestamp is initialized.

Sounds like a mess honestly.

[jefferyto]

Thanks @neheb I have opened some issues upstream (fln/addrwatch#25, fln/addrwatch#26). I should note that I only tried running addrwatch_mysql to see the help message, I didn't actually set up mariadb and test if the logging works.

[t-w]

(just letting you know in case you'd like to fix it before addrwatch upstream is fixed - probably in some next release...).

I needed to have this working and I've made a patch t-w@3574dcc fixing the problems: fln/addrwatch#25 and fln/addrwatch#26 (using the branch https://github.com/jefferyto/openwrt-packages/tree/addrwatch-fixes).

I've build manually the OpenWRT addrwatch packages and installed it on my router - logging to the stdout and to syslog works fine after patching.

I have just noticed reading this thread that the abovementioned output_flatfile.c may need the same simple fix in string format for the timestamp. I haven't used the 'flatfile' output, though...

[neheb]

Sounds like it should be using PRIu64 as it differs between 32 and 64-bit.

[jefferyto]

The appropriate way to print a uint64_t would be to use the PRIu64 format macro.

But I am more concerned that in this line:

	e->timestamp = p->pcap_header->ts.tv_sec;

the time_t value tv_sec (pcap_header is a pcap_pkthdr struct, ts is a timeval struct, tv_sec is a time_t) is being assigned to timestamp which is a uint64_t.

[t-w]

Just 5 more cents:

• AFAIK, unsigned long long is guaranteed to be at least 64bit. %llu must support unsigned long long, so it also supports at least 64 bit int as well. There is no reason why it won't work on a standard compliant system (some special embedded stuff might need special treatment, anyway, also might not have the PRIu64...). Anyway, either solves the problem for concerned OSs (Linuxes, BSD etc.).

• In C, an int can be safely assigned to a longer int, so e->timestamp = p->pcap_header->ts.tv_sec; is safe. The right side (time_t) is 4-byte long on some ARMs (RaspberryPi) and 8 bytes on x86_64, so it can be assigned to a uint64_t. However, there is indeed a difference on sign (tv_sec is signed as you noticed). But I do not think it is time to worry, now - currently the time_t from the Epoch is:

$ date +%s
1624717216

while the time for 32-bit (4 byte) INT_MAX is:

$ date -u --date @2147483647
Tue 19 Jan 2038 03:14:07 AM UTC

So it is the OS's problem anyway and by that time the time_t will have to become bigger (or funny things will happen...). And, maybe I am not an optimist on this, but most likely we do not have to worry about the sign bit of 64bit time_t ;-)

[jefferyto]

@t-w It seems you are making a lot of assumptions. Unless upstream accepts your patch, I don't foresee it being included here in its current form.

[t-w]

@jefferyto I have it patched, built and working on my OpenWRT router - that's hardly an assumption... If what I wrote above is not enough for you to try to test it, then I am wasting my time. Other users will just wait longer, I guess...

Sorry to have bothered you.

[jefferyto]

@t-w OpenWrt runs on more than arm and x86-64. Both @neheb and I have said PRIu64 is the right way to do it, if you don't want to listen that's fine.

[neheb]

Without PRIu64 or casting the value, -Wformat will complain.

As an aside, musl 1.2 brings 64-bit time_t to 32-bit platforms.

[neheb]

Is this ready to go?

[jefferyto]

Give me a few days, I'll try to find some time to work on a patch for the printf issues.

[jefferyto]

v2:

• added 005-use-c99-format-macro-constants.patch (from Use C99 format macro constants for timestamp and vlan_tag fln/addrwatch#28)
• updated source for 004-more-specific-library-linking.patch (PR was merged)