On 2024/12/26 18:31, Lucas Gabriel Vuotto wrote: > On Thu, Dec 26, 2024 at 10:46:10AM -0500, William Rusnack wrote: > > >Synopsis: The ordering of the iked flags -d and -n erroneously changes > > >the debug level. > > >Category: bin > > >Description: > > I've found an issue with iked's command line flag processing where the > > order of > > the -d and -n flags affects the resulting debug level. This appears to > > be a bug > > since flag ordering shouldn't change the program's behavior. > > > > Problem: > > - `iked -d -n` results in debug level 1 > > - `iked -n -d` results in debug level 2 > > > > The issue occurs because the -n flag unconditionally sets debug=1 > > rather than > > preserving any existing debug level: > > > > ```c > > case 'd': > > debug++; > > break; > > case 'n': > > debug = 1; /* Overwrites any previous debug > > setting */ > > opts |= IKED_OPT_NOACTION; > > break; > > ``` > > > > Impact: > > This can lead to unexpected behavior where debug messages may or may > > not appear > > depending solely on flag order. Users expecting to see debug output > > when using > > both flags may not see it if they happen to put the flags in the wrong > > order. > > > > >How-To-Repeat: > > 1. Run: iked -d -n -v -v > > 2. Note debug level and visible output > > 3. Run: iked -n -d -v -v > > 4. Note debug level and visible output > > 5. Observe that debug levels differ between the two commands > > > > >Fix: > > Make -n preserve existing debug level: > > ```c > > case 'n': > > if (!debug) debug = 1; /* Only set if not already set */ > > opts |= IKED_OPT_NOACTION; > > break; > > ``` > > A test configuration file showing the issue is welcome; I can't > reproduce it with my local config: > > $ doas iked -d -n -v -v 2>&1 | sha256 > ab2e93095df893c0b4bd266f19c37072ac3529a9f9e6a8b48d1792e53a5a5624 > $ doas iked -n -d -v -v 2>&1 | sha256 > ab2e93095df893c0b4bd266f19c37072ac3529a9f9e6a8b48d1792e53a5a5624 > > There is a very small window where it can have some effect tho, bc debug > is passed to log_init, which initialize log.c:verbose. Maybe something > like below is more correct: before the getopt switch, there is a > log_init(1, LOG_DAEMON). None of the things between the removed log_init > and the > > log_init(debug, LOG_DAEMON); > log_setverbose(verbose); > > call a log_debug, which is the only place that can affect the output if > debug > 1.
there's a little bit in parse.y that behaves differently with debug > 1 but I'm not sure how to trigger it. the (minor) bug here seems to be handling debug>1 at all, rather than something fiddly about order of -d and -n. the flag is really "do not daemonize and use stderr instead of syslog" rather than something about setting debug level.
