Created attachment 7311 patch against panel-plugin/wormulon/openbsd.c In panel-plugin/wormulon/openbsd.c the code assumes that (struct sockaddr_dl *)->sdl_data will be nul terminated. If that were ever the case, it no longer is so. The result is that string comparison with (user provided) interface name fails; no interface is founs, so there is no activity displayed. Attached patch does: 1) change a buffer size as a literal constant to macro IFNAMSIZ. 2) Check that (struct sockaddr_dl *)->sdl_nlen > 0 (as OpenBSD netstat does). 3) Reorder lines so that proper copy is made before comparison, and that comparison uses the copy. With these changes, it works. Patch made against version 1.3.1 source. (The version field on this form does not have that entry, so 'unspecified' is selected.)
Created attachment 7314 Improved patch This patch replaces the first (not applied over); improves the code in both functions in openbsd.c.
I havent looked at the code yet again, but for me 1.3.1 works perfectly fine on OpenBSD (be it with em0 or ral0/iwn0 as interface names), so you'll have to give me more details for the failure you're seeing. Is it when listing interfaces ?
Ah, interesting. This is device specific bahavior. As you say, em(4) works (it does provide nul terminated sockaddr_dl.sdl_data), but re(4) does not work (not nul terminated sockaddr_dl.sdl_data). My (only) OpenBSD workstation is ... % uname -a OpenBSD benson.example.org 6.1 GENERIC.MP#21 amd64 ... with all syspatch applied. Obviously, with re(4). I just converted the wormulon/openbsd.c file into a test program. I'll attach it. It has both original and fixed versions of get_stat(). Testing on this workstation with re(4), and my gateway with em(4), I can confirm that the original code fails to find the interface with re, but succeeds with em. (Note lo0 is OK.) The fixed code works with both. Whether or not there is a bug in re(4), I would argue that my patch is more correct. It makes re(4) work, and maybe there are others that don't nul terminate sockaddr_dl.sdl_data. An improvement in principle is that a buffer on the stack, and the copy into it, is removed. Also, note that OpenBSD's netstat code does not rely on sockaddr_dl.sdl_data being nul terminated. -Ed
oki, you've given me enough arguments :) yes this is probably device-specific, i'll have to dig my netbook with re(4) to try to reproduce the actual issue - but yes your patch is probably correct, i'll just have to find a bit of time to properly test it :)
Created attachment 7330 test program derived from openbsd.c -- original and fixed code tested If attachment is saved as find-if.c, then: % make find-if % ./find-if lo0 re0 Found: 'lo0' -- doing get_stat_fixed() FOUND 'lo0' -- FIXED STATS OK Found: 'lo0' -- doing get_stat_orig() FOUND 'lo0' -- ORIG STATS OK Found: 're0' -- doing get_stat_fixed() FOUND 're0' -- FIXED STATS OK Found: 're0' -- doing get_stat_orig() Note the all caps lines -- "ORIG STATS OK" is missing for re0. Elsewhere: % ./find-if lo0 em0 em1 em2 Found: 'lo0' -- doing get_stat_fixed() FOUND 'lo0' -- FIXED STATS OK Found: 'lo0' -- doing get_stat_orig() FOUND 'lo0' -- ORIG STATS OK Found: 'em0' -- doing get_stat_fixed() FOUND 'em0' -- FIXED STATS OK Found: 'em0' -- doing get_stat_orig() FOUND 'em0' -- ORIG STATS OK Found: 'em1' -- doing get_stat_fixed() FOUND 'em1' -- FIXED STATS OK Found: 'em1' -- doing get_stat_orig() FOUND 'em1' -- ORIG STATS OK Not found: 'em2' All good for em(4) (em2 is not up).
(In reply to Landry Breuil from comment #4) > oki, you've given me enough arguments :) yes this is probably > device-specific, i'll have to dig my netbook with re(4) to try to reproduce > the actual issue - but yes your patch is probably correct, i'll just have to > find a bit of time to properly test it :) Argh. I meant comment 3 as the reply to you. Please see that. Also comment 5 was meant to be associated with atachment "test program derived from openbsd.c -- original and fixed code tested". Pardon the mess.
(In reply to Landry Breuil from comment #4) > oki, you've given me enough arguments :) yes this is probably > device-specific, i'll have to dig my netbook with re(4) to try to reproduce > the actual issue - but yes your patch is probably correct, i'll just have to > find a bit of time to properly test it :) Thank you. BTW, your reply came so fast I almost missed it!
Ok, so i have an nfe(4) here that seems affected with your testcase: $/tmp/a.out axe0 nfe0 Found: 'axe0' -- doing get_stat_fixed() FOUND 'axe0' -- FIXED STATS OK Found: 'axe0' -- doing get_stat_orig() FOUND 'axe0' -- ORIG STATS OK Found: 'nfe0' -- doing get_stat_fixed() FOUND 'nfe0' -- FIXED STATS OK Found: 'nfe0' -- doing get_stat_orig()
(In reply to Landry Breuil from comment #8) > Ok, so i have an nfe(4) here that seems affected with your testcase: > > $/tmp/a.out axe0 nfe0 > > Found: 'axe0' -- doing get_stat_fixed() > FOUND 'axe0' -- FIXED STATS OK > Found: 'axe0' -- doing get_stat_orig() > FOUND 'axe0' -- ORIG STATS OK > Found: 'nfe0' -- doing get_stat_fixed() > FOUND 'nfe0' -- FIXED STATS OK > Found: 'nfe0' -- doing get_stat_orig() Yes, and also this from /usr/include/net/if_dl.h: struct sockaddr_dl { [snip] u_char sdl_nlen; /* interface name length, no trailing 0 reqd. */ [snip] }; #define LLADDR(s) ((caddr_t)((s)->sdl_data + (s)->sdl_nlen)) suggesting nul termination of the name can't be relied on.
Created attachment 7345 simpler (?) fix So, i finally had time to properly look into this bug, as it puzzled me - there's indeed a problem on the kernel side where sysctl() doesnt null-terminate some strings but for specific drivers. This is a separate issue that has to be dealt with. But in the end, sdl_nlen should be used - it is correctly used in fact in checkinterface() (here: https://git.xfce.org/panel-plugins/xfce4-netload-plugin/tree/panel-plugin/wormulon/openbsd.c#n80) , but *not* in get_stats() when the interface name check is done *before* uselessly copying it to s (in https://git.xfce.org/panel-plugins/xfce4-netload-plugin/tree/panel-plugin/wormulon/openbsd.c#n152). So yes, your second patch works and fixes the issue, but there's a simpler solution to me: only fix get_stats, and compare the right strings at the right moment - which is what your first patch did more or less. Whether checking for nlen > 0 is needed is separate (and cargo-culting code from netstat isnt a reason :) - using IFNAMSIZ also makes sense.
Landry Breuil referenced this bugreport in commit 5829523b18fd83fa75daf6dc010c978251d3a9ac Fix stats on some openbsd drivers (bug #13853) https://git.xfce.org/panel-plugins/xfce4-netload-plugin/commit?id=5829523b18fd83fa75daf6dc010c978251d3a9ac
Commited the simpler fix, thanks for you detailed bugreport ;)
Aaaand fixed