From 9e13d34c2b7edaca7a5b119bc6619b496ab9404a Mon Sep 17 00:00:00 2001 From: Lei Meng Date: Sat, 10 Oct 2026 00:38:08 -0700 Subject: [PATCH 01/11] test: add data-driven test suite for libcli (#1) A Test::Base harness drives cli_loop() over a socketpair under AddressSanitizer ("make test") and valgrind ("make test-valgrind"). Seven SKIP blocks document reproduced bugs as ready-to-enable regression tests. See t/README.md for the design, block format, fixtures, and the known-bug catalog. --- .gitignore | 3 + Makefile | 21 +- t/README.md | 176 +++++++++++++++ t/basic.t | 249 +++++++++++++++++++++ t/cli_driver.c | 511 +++++++++++++++++++++++++++++++++++++++++++ t/completion.t | 111 ++++++++++ t/editing.t | 92 ++++++++ t/filters.t | 116 ++++++++++ t/known_bugs.t | 30 +++ t/lib/Test/LibCLI.pm | 309 ++++++++++++++++++++++++++ t/valgrind.supp | 19 ++ 11 files changed, 1636 insertions(+), 1 deletion(-) create mode 100644 t/README.md create mode 100644 t/basic.t create mode 100644 t/cli_driver.c create mode 100644 t/completion.t create mode 100644 t/editing.t create mode 100644 t/filters.t create mode 100644 t/known_bugs.t create mode 100644 t/lib/Test/LibCLI.pm create mode 100644 t/valgrind.supp diff --git a/.gitignore b/.gitignore index b2e1526..c79a965 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,6 @@ *.so.* clitest compile_commands.json +t/cli_driver +*~ +t/cli_driver_plain diff --git a/Makefile b/Makefile index b5b050d..75bd696 100644 --- a/Makefile +++ b/Makefile @@ -57,11 +57,30 @@ libcli.o: libcli.h clitest: clitest.o $(LIB) $(CC) $(CPPFLAGS) $(CFLAGS) -o $@ $< -L. -lcli +# Test driver for the Perl suite in t/. libcli.c is compiled in directly so +# the sanitizers instrument the library too. +TEST_CFLAGS = -g -O1 -fno-omit-frame-pointer -fsanitize=address,undefined \ + -Wall -std=c99 -pedantic -Werror -Wextra -Wno-unused-parameter + +t/cli_driver: t/cli_driver.c libcli.c libcli.h + $(CC) $(CPPFLAGS) $(TEST_CFLAGS) -I. -o $@ t/cli_driver.c libcli.c $(LIBS) + +test: t/cli_driver + prove -r t/ + +# Non-sanitized driver for valgrind (ASan and valgrind cannot coexist). +t/cli_driver_plain: t/cli_driver.c libcli.c libcli.h + $(CC) $(CPPFLAGS) -g -O0 -I. -std=c99 -pedantic -Wall -Wextra \ + -Wno-unused-parameter -o $@ t/cli_driver.c libcli.c $(LIBS) + +test-valgrind: t/cli_driver_plain + LIBCLI_DRIVER=t/cli_driver_plain LIBCLI_VALGRIND=1 prove -r t/ + clitest.exe: clitest.c libcli.o $(CC) $(CPPFLAGS) $(CFLAGS) -o $@ $< libcli.o -lws2_32 clean: - rm -f *.o $(LIB)* $(LIB_STATIC) clitest libcli-$(MAJOR).$(MINOR).$(REVISION).tar.gz + rm -f *.o $(LIB)* $(LIB_STATIC) clitest t/cli_driver t/cli_driver_plain libcli-$(MAJOR).$(MINOR).$(REVISION).tar.gz install: $(TARGET_LIBS) install -d $(DESTDIR)$(PREFIX)/include $(DESTDIR)$(PREFIX)/lib diff --git a/t/README.md b/t/README.md new file mode 100644 index 0000000..5071c98 --- /dev/null +++ b/t/README.md @@ -0,0 +1,176 @@ +# libcli test suite + +Data-driven tests for libcli, in the style of +[Test::Nginx](https://metacpan.org/dist/Test-Nginx). Each test is a block +of text describing input to type into a running CLI session and the +output to expect back. The library is compiled directly into a small C +driver built with sanitizers, so memory and undefined-behaviour errors +turn into test failures. + +## Running + + make test # fast: AddressSanitizer + UndefinedBehaviorSanitizer + make test-valgrind # slower: valgrind/Memcheck (leaks, uninitialized reads) + +`make test` builds `t/cli_driver` with `-fsanitize=address,undefined` and +runs `prove -r t/`. `make test-valgrind` builds a separate non-sanitized +driver (`t/cli_driver_plain`, because ASan and valgrind cannot share a +binary) and runs the same blocks under Memcheck. + +Run one file verbosely: + + prove -v t/filters.t + +To run a single block while debugging, add `--- ONLY` to it (Test::Base +runs only that block); remove it before committing. + +Requires Perl with `Test::Base` (Debian/Ubuntu: `libtest-base-perl`). + +## Layout + + t/ + cli_driver.c the C driver: builds a CLI from a fixture, runs cli_loop() + lib/Test/LibCLI.pm the harness: forks the driver, feeds input, checks output + valgrind.supp baseline suppressions for known, pre-existing leaks + basic.t commands, auth, enable, config mode, history + editing.t line-editing keys (ctrl-a/e/b/f/k/w/u/z, backspace, insert) + filters.t include/exclude/count/begin/between/grep/egrep + completion.t tab and '?' completion, validators + known_bugs.t cross-cutting known-bug blocks (all SKIP) + +## How a block runs + +For each block the harness: + +1. creates a `socketpair` and forks the driver, with one socket end as the + driver's stdin/stdout (what `cli_loop()` reads and writes), and the + driver's stderr captured to a temp file; +2. types the block's `input` into the socket, then closes the write end; +3. reads all output until EOF, with an `alarm`-based timeout guard; +4. renders that byte stream the way a terminal would (see **Rendering**); +5. checks the block's assertions, plus three implicit ones that run on + **every** block: the driver must finish within the timeout, must not die + from a signal, and must not print a sanitizer or valgrind report. + +Each block is one `Test::More` subtest, so a failure names the block and +shows the full session output and stderr as diagnostics. + +## Block sections + +A block starts with `=== NAME` and contains `--- section` entries: + +| Section | Meaning | +|---------|---------| +| `--- fixture: NAME` | driver fixture to build (default `basic`) | +| `--- users` | one `name:password` per line; enables login | +| `--- enable_password: PW` | password for the `enable` command | +| `--- hostname: NAME` | hostname shown in the prompt (default `router`) | +| `--- telnet` | enable telnet protocol negotiation (presence only) | +| `--- input` | keystrokes; each newline is sent as Enter (`\r`) | +| `--- input eval` | Perl that returns the input string (for generated input) | +| `--- output` | exact rendered output must equal this | +| `--- output_like` | one regex per line; each must match | +| `--- output_unlike` | one regex per line; none may match | +| `--- exit_code: N` | driver exit code must be N (default 0) | +| `--- timeout: SECONDS` | per-block timeout (default 10; scaled under valgrind) | +| `--- SKIP` | Test::Base: exclude this block from the run (see below) | + +Regexes are matched in `/ms` mode, so `^` and `$` anchor to line +boundaries within the rendered output. + +### Input key markers + +Inside `--- input`, these stand for control bytes: + + arrow keys send ANSI sequences + any letter, e.g. clears the line + a literal "<" + +`--- input eval` is Perl; use it for input a literal block cannot express, +e.g. a near-buffer-length line or raw bytes: + + --- input eval + "show" . (" " x 4091) . "\n" + +## Rendering + +libcli's line editor redraws with backspaces and carriage returns, so the +raw bytes on the socket are not what a user sees. Before checking output +the harness replays the stream as a dumb terminal would: `\b` moves the +cursor left, characters overwrite, `\r` returns to column 0, `\a` (bell) +is dropped, and trailing spaces on each line are removed. Assertions +therefore describe the *visible* screen, not the redraw bytes. + +## The driver and fixtures + +`t/cli_driver.c` builds a CLI from a named fixture, then calls +`cli_loop()` on fd 0. Generic command callbacks print what they received +so tests can assert on how a line was parsed: + + RAN + OPTARG = + ARG[] + +Fixtures currently available (`--- fixture:`): + +| Fixture | Purpose | +|---------|---------| +| `basic` | subcommands, optargs, privilege, config mode, output for filters | +| `completion` | a single long child plus common-prefix children | +| `validate` | a command with a validated argument | +| `dupoptarg` | registers a duplicate optarg, then exercises the command | +| `buildmode` | a build-mode command whose optarg name collides with a reserved one | +| `unreg_child` | registers `show counters`, then unregisters the child | + +### Adding a fixture + +Add a `drv_fixture_*` function in `t/cli_driver.c`, list it in +`drv_fixtures[]`, and forward-declare it. Fixtures return `CLI_OK` or +`CLI_ERROR`. The driver also accepts `--user name:password`, `--enable +PW`, `--hostname NAME`, and `--telnet`, which the harness sets from the +corresponding block sections. + +## Known bugs: `--- SKIP` + +A block documenting a bug that is not yet fixed is marked with +Test::Base's `--- SKIP`, which removes it from `blocks()` so it does not +run and the suite stays green. Its title ends with `(KNOWN BUG +libcli.c:LINE)`. The block states the *correct* expected behaviour, so +fixing the bug is: delete the `--- SKIP` line (and the `(KNOWN BUG ...)` +note), and the block becomes a live regression test. + +Because `SKIP` blocks never run, fixing a bug will not automatically flag +that its block now passes — removing the `--- SKIP` line is a required +step of the fix. + +Current known-bug blocks: + +| Block | libcli.c | Symptom | +|-------|----------|---------| +| completion.t "duplicate optarg" | 2279 | `cli_free_optarg(NULL)` crash | +| completion.t "overflow near line limit" | 1472 | tab-completion heap overflow | +| completion.t "failed buildmode entry" | 1781 | NULL deref after freed buildmode | +| basic.t "password may contain '?'" | 1544 | `?` swallowed in password states | +| basic.t "password mid-line edit" | 1358 | cursor moved without compacting buffer | +| known_bugs.t "unregister child" | 476 | parent `children` dangles (use-after-free) | +| known_bugs.t "telnet subnegotiation" | 1251 | no SB…SE state machine; payload injected | + +## valgrind mode + +`make test-valgrind` runs every block under Memcheck. It catches two +things the ASan build does not: reads of uninitialized memory, and leaks +(the ASan build runs with `detect_leaks=0`). Each block's timeout is +scaled up automatically because Memcheck is ~20–30x slower. + +`t/valgrind.supp` baselines known pre-existing leaks so the gate still +fails on *new* ones. Each suppression names the bug it hides; remove the +suppression as part of the fix so the exercising test turns into live +leak coverage. It currently suppresses one leak: `cli_done()` does not +free `cli->enable_password` (allocated in `cli_allow_enable`, +libcli.c:270). + +## CI + +`make test` is suitable for every push (seconds). `make test-valgrind` +suits a nightly or pre-release job. Both need `libtest-base-perl`; +valgrind additionally needs the `valgrind` package. diff --git a/t/basic.t b/t/basic.t new file mode 100644 index 0000000..725c3ee --- /dev/null +++ b/t/basic.t @@ -0,0 +1,249 @@ +use lib 't/lib'; +use Test::LibCLI; +run_tests(); + +__DATA__ + +=== TEST 1: run a subcommand without authentication +--- input +show counters +--- output_like +^router> show counters$ +^RAN show counters$ + + + +=== TEST 2: abbreviated command words +--- input +sh co +--- output_like +^RAN show counters$ + + + +=== TEST 3: unknown command is rejected +--- input +bogus +--- output_like +^Invalid command "bogus"$ +--- output_unlike +^RAN + + + +=== TEST 4: required argument, optional flag and optional argument +--- input +set foo verbose count 3 +--- output_like +^RAN set$ +^OPTARG name=foo$ +^OPTARG verbose=verbose$ +^OPTARG count=3$ + + + +=== TEST 5: missing required argument +--- input +set +--- output_like +missing required argument 'name' for command 'set' +--- output_unlike +^RAN + + + +=== TEST 6: login with a valid user, password not echoed +--- users +bob:s3cret +--- input +bob +s3cret +show version +--- output_like +^Username: bob$ +Password: +^RAN show version$ +--- output_unlike +s3cret +Access denied + + + +=== TEST 7: login with a wrong password is denied +--- users +bob:s3cret +--- input +bob +wrong +--- output_like +Access denied +^Username:$ +--- output_unlike +^router> + + + +=== TEST 8: privileged command needs enable +--- input +reload +--- output_like +^Invalid command "reload"$ +--- output_unlike +^RAN reload + + + +=== TEST 9: enable with password, then run a privileged command +--- enable_password: letmein +--- input +enable +letmein +reload +--- output_like +^router# reload$ +^RAN reload$ +--- output_unlike +Access denied +letmein + + + +=== TEST 10: enable with a wrong password is denied +--- enable_password: letmein +--- input +enable +nope +reload +--- output_like +Access denied +^Invalid command "reload"$ + + + +=== TEST 11: config mode changes the prompt and the hostname +--- input +enable +configure terminal +hostname edge1 +exit +--- output_like +^router\(config\)# hostname edge1$ +^edge1\(config\)# exit$ +^edge1#$ + + + +=== TEST 12: include filter +--- input +show lines | include beta +--- output_like +^line 2: beta$ +--- output_unlike +alpha +gamma + + + +=== TEST 13: exclude filter +--- input +show lines | exclude beta +--- output_like +^line 1: alpha$ +^line 3: gamma$ +--- output_unlike +^line 2: beta$ + + + +=== TEST 14: count filter +--- input +show lines | count +--- output_like +^3$ +--- output_unlike +^line + + + +=== TEST 15: tab completes a unique subcommand +--- input +show cou +--- output_like +^router> show counters$ +^RAN show counters$ + + + +=== TEST 16: help lists subcommands +--- input +show ? +--- output_like +^ counters\s+Show counters$ +^ version\s+Show version$ + + + +=== TEST 17: ctrl-u discards the line +--- input +bogusshow version +--- output_like +^RAN show version$ +--- output_unlike +Invalid command + + + +=== TEST 18: up arrow recalls the previous command +--- input +show version + +--- output_like +RAN show version\n.*RAN show version + + + +=== TEST 19: history lists earlier commands +--- input +show version +show counters +history +--- output_like +^Command history:$ +^ 0\. show version$ +^ 1\. show counters$ + + + +=== TEST 20: backspace edits the line +--- input eval +"show versionX\x7f\n" +--- output_like +^RAN show version$ + + + +=== TEST 21: a password may contain '?' (KNOWN BUG libcli.c:1544) +--- SKIP +--- users +bob:ab? +--- input +bob +ab? +show version +--- output_like +^RAN show version$ +--- output_unlike +Access denied + + + +=== TEST 22: editing a password mid-line does not corrupt it (KNOWN BUG libcli.c:1358) +--- SKIP +--- users +bob:ac +--- input eval +"bob\rabc\x1b[D\x7f\rshow version\r" +--- output_like +^RAN show version$ +--- output_unlike +Access denied diff --git a/t/cli_driver.c b/t/cli_driver.c new file mode 100644 index 0000000..b7ce645 --- /dev/null +++ b/t/cli_driver.c @@ -0,0 +1,511 @@ +/* vim: set sw=4 tw=80 et: */ + +/* + * Test driver for the Perl test suite in t/. + * + * The driver builds a cli from a named fixture, then runs cli_loop() on + * fd 0, which the harness connects to one end of a socketpair. Command + * callbacks print what they received in a fixed format so tests can + * assert on how a line was parsed: + * + * RAN + * OPTARG = + * ARG[] + * + * Usage: cli_driver [--fixture name] [--user name:password]... + * [--enable password] [--hostname name] [--telnet] + */ + +#include "libcli.h" + +#include +#include +#include + + +#define DRV_MAX_USERS 16 + + +typedef int (*drv_fixture_pt)(struct cli_def *cli); + + +typedef struct { + const char *name; + drv_fixture_pt setup; +} drv_fixture_t; + + +static int drv_cmd_print(struct cli_def *cli, const char *command, + char *argv[], int argc); +static int drv_cmd_lines(struct cli_def *cli, const char *command, + char *argv[], int argc); +static int drv_cmd_hostname(struct cli_def *cli, const char *command, + char *argv[], int argc); +static int drv_fixture_basic(struct cli_def *cli); +static int drv_fixture_completion(struct cli_def *cli); +static int drv_fixture_validate(struct cli_def *cli); +static int drv_fixture_dupoptarg(struct cli_def *cli); +static int drv_fixture_buildmode(struct cli_def *cli); +static int drv_fixture_unreg_child(struct cli_def *cli); +static int drv_validate_color(struct cli_def *cli, const char *name, + const char *value); +static const drv_fixture_t *drv_find_fixture(const char *name); +static int drv_add_user(struct cli_def *cli, const char *spec); +static void drv_usage(const char *argv0); + + +static const drv_fixture_t drv_fixtures[] = { + { "basic", drv_fixture_basic }, + { "completion", drv_fixture_completion }, + { "validate", drv_fixture_validate }, + { "dupoptarg", drv_fixture_dupoptarg }, + { "buildmode", drv_fixture_buildmode }, + { "unreg_child", drv_fixture_unreg_child }, + { NULL, NULL } +}; + + +int +main(int argc, char *argv[]) +{ + int i, rc, telnet, nusers; + const char *fixture, *enable, *hostname; + struct cli_def *cli; + const drv_fixture_t *fx; + const char *users[DRV_MAX_USERS]; + + fixture = "basic"; + enable = NULL; + hostname = "router"; + telnet = 0; + nusers = 0; + + for (i = 1; i < argc; i++) { + if (strcmp(argv[i], "--telnet") == 0) { + telnet = 1; + + } else if (i + 1 < argc && strcmp(argv[i], "--fixture") == 0) { + fixture = argv[++i]; + + } else if (i + 1 < argc && strcmp(argv[i], "--enable") == 0) { + enable = argv[++i]; + + } else if (i + 1 < argc && strcmp(argv[i], "--hostname") == 0) { + hostname = argv[++i]; + + } else if (i + 1 < argc && strcmp(argv[i], "--user") == 0) { + if (nusers == DRV_MAX_USERS) { + fprintf(stderr, "cli_driver: too many --user options\n"); + return 2; + } + + users[nusers++] = argv[++i]; + + } else { + drv_usage(argv[0]); + return 2; + } + } + + fx = drv_find_fixture(fixture); + if (fx == NULL) { + fprintf(stderr, "cli_driver: unknown fixture \"%s\"\n", fixture); + return 2; + } + + cli = cli_init(); + if (cli == NULL) { + fprintf(stderr, "cli_driver: cli_init() failed\n"); + return 2; + } + + cli_telnet_protocol(cli, telnet); + cli_set_hostname(cli, hostname); + + for (i = 0; i < nusers; i++) { + if (drv_add_user(cli, users[i]) != 0) { + cli_done(cli); + return 2; + } + } + + if (enable != NULL) { + cli_allow_enable(cli, enable); + } + + if (fx->setup(cli) != CLI_OK) { + fprintf(stderr, "cli_driver: fixture \"%s\" setup failed\n", + fixture); + cli_done(cli); + return 2; + } + + rc = cli_loop(cli, 0); + cli_done(cli); + + return rc == CLI_OK ? 0 : 1; +} + + +static int +drv_cmd_print(struct cli_def *cli, const char *command, char *argv[], + int argc) +{ + int i; + struct cli_optarg_pair *optarg; + + cli_print(cli, "RAN %s", command); + + for (optarg = cli_get_all_found_optargs(cli); + optarg != NULL; + optarg = optarg->next) + { + cli_print(cli, "OPTARG %s=%s", optarg->name, optarg->value); + } + + for (i = 0; i < argc; i++) { + cli_print(cli, "ARG[%d] %s", i, argv[i]); + } + + return CLI_OK; +} + + +static int +drv_cmd_lines(struct cli_def *cli, const char *command, char *argv[], + int argc) +{ + cli_print(cli, "line 1: alpha"); + cli_print(cli, "line 2: beta"); + cli_print(cli, "line 3: gamma"); + + return CLI_OK; +} + + +static int +drv_cmd_hostname(struct cli_def *cli, const char *command, char *argv[], + int argc) +{ + char *name; + + name = cli_get_optarg_value(cli, "name", NULL); + cli_set_hostname(cli, name); + + return CLI_OK; +} + + +/* + * basic: a small command tree covering subcommands, optargs, privilege + * levels, config mode, and output for filters. + */ +static int +drv_fixture_basic(struct cli_def *cli) +{ + struct cli_command *c; + + c = cli_register_command(cli, NULL, "show", NULL, PRIVILEGE_UNPRIVILEGED, + MODE_EXEC, "Show information"); + if (c == NULL) { + return CLI_ERROR; + } + + if (cli_register_command(cli, c, "counters", drv_cmd_print, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Show counters") == NULL + || cli_register_command(cli, c, "version", drv_cmd_print, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Show version") == NULL + || cli_register_command(cli, c, "lines", drv_cmd_lines, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Print three lines") == NULL) + { + return CLI_ERROR; + } + + c = cli_register_command(cli, NULL, "set", drv_cmd_print, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Set a value"); + if (c == NULL) { + return CLI_ERROR; + } + + if (cli_register_optarg(c, "name", CLI_CMD_ARGUMENT, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Name to set", NULL, NULL, NULL) == NULL + || cli_register_optarg(c, "verbose", CLI_CMD_OPTIONAL_FLAG, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Be verbose", NULL, NULL, NULL) == NULL + || cli_register_optarg(c, "count", CLI_CMD_OPTIONAL_ARGUMENT, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Repeat count", NULL, NULL, NULL) == NULL) + { + return CLI_ERROR; + } + + if (cli_register_command(cli, NULL, "reload", drv_cmd_print, + PRIVILEGE_PRIVILEGED, MODE_EXEC, + "Reload the system") == NULL) + { + return CLI_ERROR; + } + + c = cli_register_command(cli, NULL, "hostname", drv_cmd_hostname, + PRIVILEGE_PRIVILEGED, MODE_CONFIG, + "Set the hostname"); + if (c == NULL) { + return CLI_ERROR; + } + + if (cli_register_optarg(c, "name", CLI_CMD_ARGUMENT, + PRIVILEGE_PRIVILEGED, MODE_CONFIG, + "New hostname", NULL, NULL, NULL) == NULL) + { + return CLI_ERROR; + } + + return CLI_OK; +} + + +/* + * completion: "neighbor" has a single child with a long name, so a Tab at + * the end of a near-full line yields one completion. "interface" has + * children sharing a prefix, to exercise partial completion and double + * Tab. + */ +static int +drv_fixture_completion(struct cli_def *cli) +{ + struct cli_command *c; + + c = cli_register_command(cli, NULL, "neighbor", NULL, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, "Neighbors"); + if (c == NULL) { + return CLI_ERROR; + } + + if (cli_register_command(cli, c, "advertisement", drv_cmd_print, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Advertisement") == NULL) + { + return CLI_ERROR; + } + + c = cli_register_command(cli, NULL, "interface", NULL, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, "Interface"); + if (c == NULL) { + return CLI_ERROR; + } + + if (cli_register_command(cli, c, "ethernet", drv_cmd_print, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Ethernet") == NULL + || cli_register_command(cli, c, "ethertype", drv_cmd_print, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Ethertype") == NULL) + { + return CLI_ERROR; + } + + return CLI_OK; +} + + +/* + * validate: a command with a validated optional argument, to exercise the + * validator success and failure paths. + */ +static int +drv_fixture_validate(struct cli_def *cli) +{ + struct cli_command *c; + + c = cli_register_command(cli, NULL, "paint", drv_cmd_print, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, "Paint it"); + if (c == NULL) { + return CLI_ERROR; + } + + if (cli_register_optarg(c, "color", CLI_CMD_ARGUMENT, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "A primary color", NULL, drv_validate_color, + NULL) == NULL) + { + return CLI_ERROR; + } + + return CLI_OK; +} + + +/* + * dupoptarg: registers "name" twice with the same privilege and mode. + * The duplicate registration must fail cleanly (return NULL) and leave a + * usable cli, which the following session then exercises. + */ +static int +drv_fixture_dupoptarg(struct cli_def *cli) +{ + struct cli_command *c; + + c = cli_register_command(cli, NULL, "set", drv_cmd_print, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, "Set"); + if (c == NULL) { + return CLI_ERROR; + } + + if (cli_register_optarg(c, "name", CLI_CMD_ARGUMENT, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Name", NULL, NULL, NULL) == NULL) + { + return CLI_ERROR; + } + + /* The duplicate; the return value is intentionally discarded. */ + cli_register_optarg(c, "name", CLI_CMD_ARGUMENT, PRIVILEGE_UNPRIVILEGED, + MODE_EXEC, "Name again", NULL, NULL, NULL); + + return CLI_OK; +} + + +/* + * buildmode: "build" has an optarg flagged CLI_CMD_ALLOW_BUILDMODE and a + * second optarg whose name, "show", collides with a reserved buildmode + * command. Entering buildmode (by typing "build go") therefore fails, + * and cli_loop reports the failure. A plain "ping" command follows so a + * test can confirm the cli survives the failed entry. + */ +static int +drv_fixture_buildmode(struct cli_def *cli) +{ + struct cli_command *c; + + c = cli_register_command(cli, NULL, "build", drv_cmd_print, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, "Build"); + if (c == NULL) { + return CLI_ERROR; + } + + if (cli_register_optarg(c, "go", + CLI_CMD_ALLOW_BUILDMODE | CLI_CMD_OPTIONAL_FLAG, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Enter build mode", NULL, NULL, NULL) == NULL + || cli_register_optarg(c, "show", CLI_CMD_OPTIONAL_FLAG, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Reserved name", NULL, NULL, NULL) == NULL) + { + return CLI_ERROR; + } + + if (cli_register_command(cli, NULL, "ping", drv_cmd_print, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Ping") == NULL) + { + return CLI_ERROR; + } + + return CLI_OK; +} + + +/* + * unreg_child: registers "show counters", then unregisters the child via + * cli_unregister_all(). cli_free_command does not clear the parent's + * children pointer, so "show" is left pointing at freed memory; the + * following session then walks into it. + */ +static int +drv_fixture_unreg_child(struct cli_def *cli) +{ + struct cli_command *show, *counters; + + show = cli_register_command(cli, NULL, "show", NULL, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, "Show"); + if (show == NULL) { + return CLI_ERROR; + } + + counters = cli_register_command(cli, show, "counters", drv_cmd_print, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Counters"); + if (counters == NULL) { + return CLI_ERROR; + } + + cli_unregister_all(cli, counters); + + return CLI_OK; +} + + +static int +drv_validate_color(struct cli_def *cli, const char *name, const char *value) +{ + if (value != NULL + && (strcmp(value, "red") == 0 || strcmp(value, "green") == 0 + || strcmp(value, "blue") == 0)) + { + return CLI_OK; + } + + return CLI_ERROR; +} + + +static const drv_fixture_t * +drv_find_fixture(const char *name) +{ + const drv_fixture_t *fx; + + for (fx = drv_fixtures; fx->name != NULL; fx++) { + if (strcmp(fx->name, name) == 0) { + return fx; + } + } + + return NULL; +} + + +/* spec is "name:password"; the password may itself contain ':' */ +static int +drv_add_user(struct cli_def *cli, const char *spec) +{ + char *name, *colon; + size_t len; + + colon = strchr(spec, ':'); + if (colon == NULL) { + fprintf(stderr, "cli_driver: --user wants name:password\n"); + return -1; + } + + len = (size_t) (colon - spec); + + name = malloc(len + 1); + if (name == NULL) { + fprintf(stderr, "cli_driver: out of memory\n"); + return -1; + } + + memcpy(name, spec, len); + name[len] = '\0'; + + cli_allow_user(cli, name, colon + 1); + free(name); + + return 0; +} + + +static void +drv_usage(const char *argv0) +{ + fprintf(stderr, + "usage: %s [--fixture name] [--user name:password]...\n" + " [--enable password] [--hostname name] [--telnet]\n", + argv0); +} diff --git a/t/completion.t b/t/completion.t new file mode 100644 index 0000000..d913c51 --- /dev/null +++ b/t/completion.t @@ -0,0 +1,111 @@ +use lib 't/lib'; +use Test::LibCLI; +run_tests(); + +__DATA__ + +=== TEST 1: a unique prefix completes to the full word +--- fixture: completion +--- input +neighadv +--- output_like +^router> neighbor advertisement$ +^RAN neighbor advertisement$ + + + +=== TEST 2: a common prefix is filled in, stopping where names differ +--- fixture: completion +--- input +interether +--- output_like +router> interface ether + + + +=== TEST 3: double Tab lists the ambiguous completions +--- fixture: completion +--- input +interface ether +--- output_like +ethernet +ethertype + + + +=== TEST 4: '?' lists subcommands with help text +--- fixture: completion +--- input +interface ? +--- output_like +^ ethernet\s+Ethernet$ +^ ethertype\s+Ethertype$ + + + +=== TEST 5: Tab on an empty command rings the bell, no crash +--- fixture: completion +--- input eval +"\n" +--- output_unlike +RAN + + + +=== TEST 6: validated argument accepts a valid value +--- fixture: validate +--- input +paint red +--- output_like +^RAN paint$ +^OPTARG color=red$ + + + +=== TEST 7: validated argument rejects an invalid value +--- fixture: validate +--- input +paint mauve +--- output_like +Problem parsing command setting color with value mauve +--- output_unlike +^OPTARG color=mauve$ + + + +=== TEST 8: '?' after a validated argument name shows its help +--- fixture: validate +--- input +paint ? +--- output_like +\s+A primary color + + + +=== TEST 9: registering a duplicate optarg leaves a usable cli (KNOWN BUG libcli.c:2279) +--- SKIP +--- fixture: dupoptarg +--- input +set hello +--- output_like +^RAN set$ +^OPTARG name=hello$ + + + +=== TEST 10: Tab completion near the line-length limit does not overflow (KNOWN BUG libcli.c:1472) +--- SKIP +--- fixture: completion +--- input eval +"neighbor" . (" " x 4080) . "\n" + + + +=== TEST 11: a failed buildmode entry does not crash the cli (KNOWN BUG libcli.c:1781) +--- SKIP +--- fixture: buildmode +--- input +build go +ping +--- output_like +^RAN ping$ diff --git a/t/editing.t b/t/editing.t new file mode 100644 index 0000000..ad8f81d --- /dev/null +++ b/t/editing.t @@ -0,0 +1,92 @@ +use lib 't/lib'; +use Test::LibCLI; +run_tests(); + +__DATA__ + +=== TEST 1: ctrl-a jumps to start, ctrl-e to end +--- input eval +"show countersX\n" +--- output_like +^Invalid command "Xshow"$ +--- output_unlike +^RAN show counters$ + + + +=== TEST 2: ctrl-b moves left so an inserted char lands mid-word +--- input eval +"shwo\n" +--- output_like +^router> show$ +^Incomplete command$ + + + +=== TEST 3: ctrl-k kills to end of line +--- input eval +"show counters junk\n" +--- output_like +counters + + + +=== TEST 4: ctrl-w deletes the previous word +--- input eval +"show counters junk\n" +--- output_like +^RAN show counters$ + + + +=== TEST 5: backspace at an empty line rings the bell, no crash +--- input eval +"show version\n" +--- output_like +^RAN show version$ + + + +=== TEST 6: ctrl-u then a fresh command +--- input eval +"nonsense hereshow version\n" +--- output_like +^RAN show version$ +--- output_unlike +nonsense + + + +=== TEST 7: insert a character in the middle of the line +--- input eval +"shw counterso\n" +--- output_like +^RAN show counters$ + + + +=== TEST 8: ctrl-z leaves config mode back to exec +--- input +enable +configure terminal + +show version +--- output_like +^router# show version$ +^RAN show version$ + + + +=== TEST 9: a very long line is truncated at the buffer limit, no crash +--- input eval +"show " . ("x" x 5000) . "\n" +--- output_like +Invalid command + + + +=== TEST 10: control characters do not crash the editor +--- input eval +"\x01\x02\x05\x06show version\n" +--- output_like +^RAN show version$ diff --git a/t/filters.t b/t/filters.t new file mode 100644 index 0000000..069ef8c --- /dev/null +++ b/t/filters.t @@ -0,0 +1,116 @@ +use lib 't/lib'; +use Test::LibCLI; +run_tests(); + +__DATA__ + +=== TEST 1: include keeps only matching lines +--- input +show lines | include beta +--- output_like +^line 2: beta$ +--- output_unlike +alpha +gamma + + + +=== TEST 2: exclude drops matching lines +--- input +show lines | exclude beta +--- output_like +^line 1: alpha$ +^line 3: gamma$ +--- output_unlike +^line 2: beta$ + + + +=== TEST 3: count reports the number of lines +--- input +show lines | count +--- output_like +^3$ + + + +=== TEST 4: begin starts at the first match +--- input +show lines | begin beta +--- output_like +^line 2: beta$ +^line 3: gamma$ +--- output_unlike +^line 1: alpha$ + + + +=== TEST 5: between bounds the output by two matches +--- input +show lines | between alpha gamma +--- output_like +^line 1: alpha$ +^line 2: beta$ +^line 3: gamma$ + + + +=== TEST 6: grep matches a regular expression +--- input +show lines | grep be.a +--- output_like +^line 2: beta$ +--- output_unlike +alpha +gamma + + + +=== TEST 7: grep -v inverts the match +--- input +show lines | grep -v beta +--- output_like +^line 1: alpha$ +^line 3: gamma$ +--- output_unlike +^line 2: beta$ + + + +=== TEST 8: grep -i matches case-insensitively +--- input +show lines | grep -i BETA +--- output_like +^line 2: beta$ + + + +=== TEST 9: egrep supports extended syntax +--- input +show lines | egrep "(alpha|gamma)" +--- output_like +^line 1: alpha$ +^line 3: gamma$ +--- output_unlike +^line 2: beta$ + + + +=== TEST 10: an invalid regex is reported, not crashed +--- input +show lines | egrep ( +--- output_like +Invalid pattern +--- output_unlike +^line 1: alpha$ + + + +=== TEST 11: filters chain together +--- input +show lines | exclude alpha | include gamma +--- output_like +^line 3: gamma$ +--- output_unlike +^line 1: alpha$ +^line 2: beta$ diff --git a/t/known_bugs.t b/t/known_bugs.t new file mode 100644 index 0000000..57d875a --- /dev/null +++ b/t/known_bugs.t @@ -0,0 +1,30 @@ +use lib 't/lib'; +use Test::LibCLI; + +# Known, unfixed bugs that do not belong to a single topical test file. +# Each block is marked SKIP so the suite stays green; drop the "--- SKIP" +# line (and the "(KNOWN BUG ...)" note) as part of the fix to turn the +# block into a live regression test. +run_tests(); + +__DATA__ + +=== TEST 1: unregistering a child command leaves the parent usable (KNOWN BUG libcli.c:476) +--- SKIP +--- fixture: unreg_child +--- input +show ? +--- output_unlike +counters + + + +=== TEST 2: telnet subnegotiation payload must not leak into the command line (KNOWN BUG libcli.c:1251) +--- SKIP +--- telnet: 1 +--- input eval +"\xff\xfa\x00ZZZ\xff\xf0show version\r" +--- output_like +^RAN show version$ +--- output_unlike +Invalid command "ZZZshow" diff --git a/t/lib/Test/LibCLI.pm b/t/lib/Test/LibCLI.pm new file mode 100644 index 0000000..5af0c3f --- /dev/null +++ b/t/lib/Test/LibCLI.pm @@ -0,0 +1,309 @@ +package Test::LibCLI; + +# Data-driven tests for libcli, in the style of Test::Nginx. +# +# Each block starts t/cli_driver on one end of a socketpair, types the +# block's input into cli_loop(), and checks the session output. Every +# block is one subtest, so a test file plans "tests => scalar blocks()". +# +# Sections: +# +# --- fixture: NAME driver fixture (default "basic") +# --- users one "name:password" per line +# --- enable_password: PW password for "enable" +# --- hostname: NAME hostname shown in the prompt (default "router") +# --- telnet enable telnet protocol negotiation (presence) +# --- input keystrokes; each newline is sent as Enter (\r) +# --- output exact rendered output (see render()) +# --- output_like one regex per line, each must match +# --- output_unlike one regex per line, none may match +# --- exit_code: N driver exit code (default 0) +# --- timeout: SECONDS default 10 +# +# Test::Base's own "--- SKIP" section excludes a block from blocks(), so a +# block documenting a known, unfixed bug is marked SKIP and does not run. +# +# Key markers in input: +# (any letter) and for a literal "<". Use "--- input eval" +# to generate input with Perl; newlines in the result are sent as Enter. +# +# Output assertions run against the session as a terminal would show it, +# with line-editor backspaces and redraws applied (see render()). +# +# Every block also fails if the driver dies on a signal, times out, or +# writes an AddressSanitizer or UndefinedBehaviorSanitizer report. + +use Test::Base -Base; + +use Errno qw(EAGAIN EINTR); +use File::Temp qw(tempfile); +use IO::Select; +use POSIX qw(WNOHANG); +use Socket; +use Time::HiRes qw(time); + +our @EXPORT = qw(run_tests); + +my $Driver = $ENV{LIBCLI_DRIVER} || 't/cli_driver'; + +$ENV{ASAN_OPTIONS} ||= 'detect_leaks=0'; +$ENV{UBSAN_OPTIONS} ||= 'print_stacktrace=1'; + +# With LIBCLI_VALGRIND set, each driver run is wrapped in Memcheck. Use +# a non-sanitized driver for this (ASan and valgrind cannot coexist), so +# "make test-valgrind" points LIBCLI_DRIVER at t/cli_driver_plain. This +# catches uninitialized-memory reads and leaks that ASan does not. +my @Valgrind = $ENV{LIBCLI_VALGRIND} + ? ('valgrind', '-q', '--error-exitcode=99', '--leak-check=full', + '--errors-for-leak-kinds=definite', '--track-origins=yes', + '--suppressions=t/valgrind.supp') + : (); + +# Memcheck is ~20-30x slower, so stretch each block's timeout under it. +my $TimeoutScale = @Valgrind ? 30 : 1; + +sub run_tests () { + if (!-x $Driver) { + Test::More::BAIL_OUT("$Driver not found; run \"make test\""); + } + + my @blocks = Test::Base::blocks(); + + # A file whose blocks are all marked SKIP has nothing to run; declare + # it skipped rather than planning zero tests (which is an error). + if (!@blocks) { + Test::More::plan(skip_all => 'all blocks are marked SKIP'); + return; + } + + Test::More::plan(tests => scalar @blocks); + + for my $block (@blocks) { + Test::More::subtest($block->name, sub { run_block($block) }); + } +} + +sub section ($$) { + my ($block, $name) = @_; + my $value = $block->$name; + + return undef if !defined $value; + $value =~ s/\n\z//; + + return $value; +} + +sub driver_args ($) { + my $block = shift; + my @args = ('--fixture', section($block, 'fixture') // 'basic'); + + my $users = section($block, 'users'); + if (defined $users) { + push @args, map { ('--user', $_) } grep { length } split /\n/, $users; + } + + my $enable = section($block, 'enable_password'); + push @args, '--enable', $enable if defined $enable; + + my $hostname = section($block, 'hostname'); + push @args, '--hostname', $hostname if defined $hostname; + + push @args, '--telnet' if defined section($block, 'telnet'); + + return @args; +} + +my %Keys = ( + TAB => "\t", + ESC => "\e", + DEL => "\x7f", + UP => "\e[A", + DOWN => "\e[B", + RIGHT => "\e[C", + LEFT => "\e[D", + LT => '<', +); + +sub encode_input ($) { + my $input = shift; + + return '' if !defined $input; + + $input =~ s/\n/\r/g; + $input =~ s//chr(ord($1) - 64)/ge; + $input =~ s/<(TAB|ESC|DEL|UP|DOWN|RIGHT|LEFT|LT)>/$Keys{$1}/g; + + return $input; +} + +# Run the driver, feed it $input, and return its output, exit status, +# stderr, and whether it timed out. +sub run_session ($$$) { + my ($args, $input, $timeout) = @_; + + socketpair(my $ours, my $theirs, AF_UNIX, SOCK_STREAM, PF_UNSPEC) + or die "socketpair: $!"; + + my ($errfh, $errfile) = tempfile(UNLINK => 1); + + my $pid = fork(); + die "fork: $!" if !defined $pid; + + if ($pid == 0) { + close $ours; + POSIX::dup2(fileno($theirs), 0) or POSIX::_exit(127); + POSIX::dup2(fileno($errfh), 1) or POSIX::_exit(127); + POSIX::dup2(fileno($errfh), 2) or POSIX::_exit(127); + { exec @Valgrind, $Driver, @$args } + print STDERR "exec $Driver: $!\n"; + POSIX::_exit(127); + } + + close $theirs; + $ours->blocking(0); + + my $output = ''; + my $deadline = time + $timeout; + my $timed_out = 0; + my $writing = length $input > 0; + + shutdown($ours, 1) if !$writing; + + for (;;) { + my $left = $deadline - time; + if ($left <= 0) { + $timed_out = 1; + last; + } + + my $rsel = IO::Select->new($ours); + my $wsel = $writing ? IO::Select->new($ours) : undef; + my ($r, $w) = IO::Select->select($rsel, $wsel, undef, $left); + + if ($w && @$w) { + my $n = syswrite($ours, $input); + if (defined $n) { + substr($input, 0, $n) = ''; + + } elsif ($! != EAGAIN && $! != EINTR) { + # The driver went away before reading all input. + $input = ''; + } + + if (!length $input) { + shutdown($ours, 1); + $writing = 0; + } + } + + if ($r && @$r) { + my $n = sysread($ours, my $buf, 65536); + next if !defined $n && ($! == EAGAIN || $! == EINTR); + last if !$n; + $output .= $buf; + } + } + + close $ours; + + if ($timed_out) { + kill 'KILL', $pid; + } + + waitpid($pid, 0); + my $status = $?; + + seek $errfh, 0, 0; + my $stderr = do { local $/; <$errfh> } // ''; + close $errfh; + + return (render($output), $status, $stderr, $timed_out); +} + +# Render the byte stream the way a simple terminal would show it, so tests +# assert on what the user sees rather than on line-editor redraw bytes: +# \b moves left, characters overwrite, \r returns to column 0, \a is +# dropped, and trailing spaces on each line are removed. +sub render ($) { + my $raw = shift; + my @lines = (''); + my $col = 0; + + for my $ch (split //, $raw) { + if ($ch eq "\n") { + push @lines, ''; + $col = 0; + + } elsif ($ch eq "\r") { + $col = 0; + + } elsif ($ch eq "\b") { + $col-- if $col > 0; + + } elsif ($ch ne "\a") { + my $line = \$lines[-1]; + $$line .= ' ' x ($col - length $$line) if $col > length $$line; + substr($$line, $col, 1) = $ch; + $col++; + } + } + + s/ +\z// for @lines; + + return join "\n", @lines; +} + +sub run_block ($) { + my $block = shift; + + my @args = driver_args($block); + my $input = encode_input(scalar $block->input); + my $timeout = (section($block, 'timeout') // 10) * $TimeoutScale; + + my ($output, $status, $stderr, $timed_out) + = run_session(\@args, $input, $timeout); + + my $diag = "driver: $Driver @args\n--- output\n$output" + . (length $stderr ? "\n--- stderr\n$stderr" : ''); + + Test::More::ok(!$timed_out, "finished within ${timeout}s") + or Test::More::diag($diag); + + Test::More::is($status & 127, 0, 'driver not killed by a signal') + or Test::More::diag($diag); + + Test::More::unlike($stderr, + qr/ERROR: AddressSanitizer|runtime error:|ERROR: LeakSanitizer| + Invalid\ (?:read|write)|uninitialised| + Mismatched\ free|definitely\ lost/x, + 'no sanitizer or valgrind reports') or Test::More::diag($diag); + + Test::More::is($status >> 8, section($block, 'exit_code') // 0, + 'driver exit code') or Test::More::diag($diag); + + my $expected = section($block, 'output'); + if (defined $expected) { + (my $got = $output) =~ s/\s+\z//; + Test::More::is($got, $expected, 'output') + or Test::More::diag($diag); + } + + for my $re (regex_lines(section($block, 'output_like'))) { + Test::More::like($output, qr/$re/ms, "output matches /$re/") + or Test::More::diag($diag); + } + + for my $re (regex_lines(section($block, 'output_unlike'))) { + Test::More::unlike($output, qr/$re/ms, "output lacks /$re/") + or Test::More::diag($diag); + } +} + +sub regex_lines ($) { + my $text = shift; + + return () if !defined $text; + return grep { length } split /\n/, $text; +} + +1; diff --git a/t/valgrind.supp b/t/valgrind.supp new file mode 100644 index 0000000..d9c363f --- /dev/null +++ b/t/valgrind.supp @@ -0,0 +1,19 @@ +# Valgrind (Memcheck) suppressions for "make test-valgrind". +# +# This file baselines KNOWN, pre-existing leaks so the valgrind gate can +# still catch NEW ones. Each entry names the bug it hides; remove the +# entry as part of the fix so the exercising test turns into live leak +# coverage. + +# KNOWN BUG: cli_done() does not free cli->enable_password, which +# cli_allow_enable() strdup's at libcli.c:270. Leaked on every cli that +# sets a static enable password. +{ + cli_done-leaks-enable_password + Memcheck:Leak + match-leak-kinds: definite + fun:malloc + fun:strdup + fun:cli_allow_enable + fun:main +} From 0f9544d20a658a31869ecf70a38ffc613f2df528 Mon Sep 17 00:00:00 2001 From: Lei Meng Date: Sat, 10 Oct 2026 10:09:12 -0700 Subject: [PATCH 02/11] Simplify Makefile: static lib by default, Linux only (#2) * Simplify Makefile: static lib by default, Linux only - Build libcli.a by default; the shared library is opt-in via DYNAMIC_LIB=1 - Always build clitest, linked against the static library (drop TESTS) - Drop the uname/Darwin handling; Linux is the only supported platform - Remove the uninstall, rpm, rpmprep, clitest.exe and test-valgrind targets - Add .PHONY for the remaining non-file targets --- Makefile | 80 ++++++++++---------------------------------- t/README.md | 19 +++++++---- t/lib/Test/LibCLI.pm | 4 +-- t/valgrind.supp | 2 +- 4 files changed, 32 insertions(+), 73 deletions(-) diff --git a/Makefile b/Makefile index 75bd696..26ce306 100644 --- a/Makefile +++ b/Makefile @@ -1,11 +1,6 @@ -# Build dynamic library by default -DYNAMIC_LIB ?= 1 -# Build static library by default -STATIC_LIB ?= 1 -# Run tests by default -TESTS ?= 1 - -UNAME = $(shell sh -c 'uname -s 2>/dev/null || echo not') +# Static library is always built; set DYNAMIC_LIB=1 to also build the shared one +DYNAMIC_LIB ?= 0 + DESTDIR = PREFIX = /usr/local @@ -21,24 +16,18 @@ ARFLAGS = rcs DEBUG = -g OPTIM = -O3 override CFLAGS += $(DEBUG) $(OPTIM) -Wall -std=c99 -pedantic -Wformat-security -Wno-format-zero-length -Werror -Wwrite-strings -Wformat -fdiagnostics-show-option -Wextra -Wsign-compare -Wcast-align -Wno-unused-parameter -override LDFLAGS += -shared -override LIBPATH += -L. - -ifeq ($(UNAME),Darwin) -override LDFLAGS += -Wl,-install_name,$(LIB).$(MAJOR).$(MINOR) -else -override LDFLAGS += -Wl,-soname,$(LIB).$(MAJOR).$(MINOR) +override LDFLAGS += -shared -Wl,-soname,$(LIB).$(MAJOR).$(MINOR) LIBS = -lcrypt -endif +TARGET_LIBS = $(LIB_STATIC) ifeq (1,$(DYNAMIC_LIB)) TARGET_LIBS += $(LIB) endif -ifeq (1,$(STATIC_LIB)) -TARGET_LIBS += $(LIB_STATIC) -endif -all: $(TARGET_LIBS) $(if $(filter 1,$(TESTS)),clitest) +all: $(TARGET_LIBS) clitest + +$(LIB_STATIC): libcli.o + $(AR) $(ARFLAGS) $@ $^ $(LIB): libcli.o $(CC) -o $(LIB).$(MAJOR).$(MINOR).$(REVISION) $^ $(LDFLAGS) $(LIBS) @@ -46,16 +35,13 @@ $(LIB): libcli.o ln -s $(LIB).$(MAJOR).$(MINOR).$(REVISION) $(LIB).$(MAJOR).$(MINOR) ln -s $(LIB).$(MAJOR).$(MINOR) $(LIB) -$(LIB_STATIC): libcli.o - $(AR) $(ARFLAGS) $@ $^ - %.o: %.c $(CC) $(CPPFLAGS) $(CFLAGS) -fPIC -o $@ -c $< libcli.o: libcli.h -clitest: clitest.o $(LIB) - $(CC) $(CPPFLAGS) $(CFLAGS) -o $@ $< -L. -lcli +clitest: clitest.o $(LIB_STATIC) + $(CC) $(CFLAGS) -o $@ $^ $(LIBS) # Test driver for the Perl suite in t/. libcli.c is compiled in directly so # the sanitizers instrument the library too. @@ -68,53 +54,21 @@ t/cli_driver: t/cli_driver.c libcli.c libcli.h test: t/cli_driver prove -r t/ -# Non-sanitized driver for valgrind (ASan and valgrind cannot coexist). -t/cli_driver_plain: t/cli_driver.c libcli.c libcli.h - $(CC) $(CPPFLAGS) -g -O0 -I. -std=c99 -pedantic -Wall -Wextra \ - -Wno-unused-parameter -o $@ t/cli_driver.c libcli.c $(LIBS) - -test-valgrind: t/cli_driver_plain - LIBCLI_DRIVER=t/cli_driver_plain LIBCLI_VALGRIND=1 prove -r t/ - -clitest.exe: clitest.c libcli.o - $(CC) $(CPPFLAGS) $(CFLAGS) -o $@ $< libcli.o -lws2_32 +lint: + clang-tidy -quiet -warnings-as-errors *.c *.h clean: - rm -f *.o $(LIB)* $(LIB_STATIC) clitest t/cli_driver t/cli_driver_plain libcli-$(MAJOR).$(MINOR).$(REVISION).tar.gz + rm -f *.o $(LIB)* $(LIB_STATIC) clitest t/cli_driver install: $(TARGET_LIBS) install -d $(DESTDIR)$(PREFIX)/include $(DESTDIR)$(PREFIX)/lib install -m 0644 libcli.h $(DESTDIR)$(PREFIX)/include - ifeq (1,$(STATIC_LIB)) install -m 0644 $(LIB_STATIC) $(DESTDIR)$(PREFIX)/lib - endif - ifeq (1,$(DYNAMIC_LIB)) +ifeq (1,$(DYNAMIC_LIB)) install -m 0755 $(LIB).$(MAJOR).$(MINOR).$(REVISION) $(DESTDIR)$(PREFIX)/lib cd $(DESTDIR)$(PREFIX)/lib && \ ln -fs $(LIB).$(MAJOR).$(MINOR).$(REVISION) $(LIB).$(MAJOR).$(MINOR) && \ ln -fs $(LIB).$(MAJOR).$(MINOR) $(LIB) - endif - -rpmprep: - rm -rf libcli-$(MAJOR).$(MINOR).$(REVISION) - mkdir libcli-$(MAJOR).$(MINOR).$(REVISION) - cp -R libcli.c libcli.h libcli.spec clitest.c Makefile COPYING README.md doc libcli-$(MAJOR).$(MINOR).$(REVISION) - tar zcvf libcli-$(MAJOR).$(MINOR).$(REVISION).tar.gz --exclude CVS --exclude *.tar.gz libcli-$(MAJOR).$(MINOR).$(REVISION) - rm -rf libcli-$(MAJOR).$(MINOR).$(REVISION) - -rpm: rpmprep - rpmbuild -ta libcli-$(MAJOR).$(MINOR).$(REVISION).tar.gz --define "debug_package %{nil}" --clean - -lint: - clang-tidy -quiet -warnings-as-errors *.c *.h +endif -uninstall: - rm -f $(DESTDIR)$(PREFIX)/include/libcli.h - ifeq (1,$(STATIC_LIB)) - rm -f $(DESTDIR)$(PREFIX)/lib/$(LIB_STATIC) - endif - ifeq (1,$(DYNAMIC_LIB)) - rm -f $(DESTDIR)$(PREFIX)/lib/$(LIB).$(MAJOR).$(MINOR).$(REVISION) - rm -f $(DESTDIR)$(PREFIX)/lib/$(LIB).$(MAJOR).$(MINOR) - rm -f $(DESTDIR)$(PREFIX)/lib/$(LIB) - endif +.PHONY: all test lint clean install diff --git a/t/README.md b/t/README.md index 5071c98..1c29f8b 100644 --- a/t/README.md +++ b/t/README.md @@ -9,13 +9,11 @@ turn into test failures. ## Running - make test # fast: AddressSanitizer + UndefinedBehaviorSanitizer - make test-valgrind # slower: valgrind/Memcheck (leaks, uninitialized reads) + make test # AddressSanitizer + UndefinedBehaviorSanitizer `make test` builds `t/cli_driver` with `-fsanitize=address,undefined` and -runs `prove -r t/`. `make test-valgrind` builds a separate non-sanitized -driver (`t/cli_driver_plain`, because ASan and valgrind cannot share a -binary) and runs the same blocks under Memcheck. +runs `prove -r t/`. The same blocks can also be run under valgrind; see +[valgrind mode](#valgrind-mode). Run one file verbosely: @@ -157,7 +155,14 @@ Current known-bug blocks: ## valgrind mode -`make test-valgrind` runs every block under Memcheck. It catches two +Set `LIBCLI_VALGRIND=1` to run every block under Memcheck. ASan and +valgrind cannot share a binary, so build a non-sanitized driver first and +point `LIBCLI_DRIVER` at it: + + gcc -g -O0 -I. -std=c99 -o t/cli_driver_plain t/cli_driver.c libcli.c -lcrypt + LIBCLI_DRIVER=t/cli_driver_plain LIBCLI_VALGRIND=1 prove -r t/ + +Memcheck catches two things the ASan build does not: reads of uninitialized memory, and leaks (the ASan build runs with `detect_leaks=0`). Each block's timeout is scaled up automatically because Memcheck is ~20–30x slower. @@ -171,6 +176,6 @@ libcli.c:270). ## CI -`make test` is suitable for every push (seconds). `make test-valgrind` +`make test` is suitable for every push (seconds). The valgrind run suits a nightly or pre-release job. Both need `libtest-base-perl`; valgrind additionally needs the `valgrind` package. diff --git a/t/lib/Test/LibCLI.pm b/t/lib/Test/LibCLI.pm index 5af0c3f..f081411 100644 --- a/t/lib/Test/LibCLI.pm +++ b/t/lib/Test/LibCLI.pm @@ -50,8 +50,8 @@ $ENV{ASAN_OPTIONS} ||= 'detect_leaks=0'; $ENV{UBSAN_OPTIONS} ||= 'print_stacktrace=1'; # With LIBCLI_VALGRIND set, each driver run is wrapped in Memcheck. Use -# a non-sanitized driver for this (ASan and valgrind cannot coexist), so -# "make test-valgrind" points LIBCLI_DRIVER at t/cli_driver_plain. This +# a non-sanitized driver for this (ASan and valgrind cannot coexist) by +# pointing LIBCLI_DRIVER at it; see "valgrind mode" in t/README.md. This # catches uninitialized-memory reads and leaks that ASan does not. my @Valgrind = $ENV{LIBCLI_VALGRIND} ? ('valgrind', '-q', '--error-exitcode=99', '--leak-check=full', diff --git a/t/valgrind.supp b/t/valgrind.supp index d9c363f..4d6bc20 100644 --- a/t/valgrind.supp +++ b/t/valgrind.supp @@ -1,4 +1,4 @@ -# Valgrind (Memcheck) suppressions for "make test-valgrind". +# Valgrind (Memcheck) suppressions for the valgrind test run (see t/README.md). # # This file baselines KNOWN, pre-existing leaks so the valgrind gate can # still catch NEW ones. Each entry names the bug it hides; remove the From e61f0a7518780267649a249a309d7de69ca36a2e Mon Sep 17 00:00:00 2001 From: Lei Meng Date: Sat, 10 Oct 2026 10:30:09 -0700 Subject: [PATCH 03/11] Add USE_ASAN option for the test driver, build it plain by default (#3) * Add USE_ASAN option for the test driver, build it plain by default t/cli_driver is now built without sanitizers unless USE_ASAN is set, so the same driver works under valgrind. --- Makefile | 10 +++++++--- t/README.md | 27 ++++++++++++++------------- t/lib/Test/LibCLI.pm | 6 +++--- 3 files changed, 24 insertions(+), 19 deletions(-) diff --git a/Makefile b/Makefile index 26ce306..4961577 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,7 @@ # Static library is always built; set DYNAMIC_LIB=1 to also build the shared one DYNAMIC_LIB ?= 0 +# Set USE_ASAN=1 to build t/cli_driver with AddressSanitizer + UBSan +USE_ASAN ?= DESTDIR = PREFIX = /usr/local @@ -44,9 +46,11 @@ clitest: clitest.o $(LIB_STATIC) $(CC) $(CFLAGS) -o $@ $^ $(LIBS) # Test driver for the Perl suite in t/. libcli.c is compiled in directly so -# the sanitizers instrument the library too. -TEST_CFLAGS = -g -O1 -fno-omit-frame-pointer -fsanitize=address,undefined \ - -Wall -std=c99 -pedantic -Werror -Wextra -Wno-unused-parameter +# that, with USE_ASAN, the sanitizers instrument the library too. +TEST_CFLAGS = -g -O0 -Wall -std=c99 -pedantic -Werror -Wextra -Wno-unused-parameter +ifneq ($(USE_ASAN),) +TEST_CFLAGS += -fno-omit-frame-pointer -fsanitize=address,undefined +endif t/cli_driver: t/cli_driver.c libcli.c libcli.h $(CC) $(CPPFLAGS) $(TEST_CFLAGS) -I. -o $@ t/cli_driver.c libcli.c $(LIBS) diff --git a/t/README.md b/t/README.md index 1c29f8b..5cd12a9 100644 --- a/t/README.md +++ b/t/README.md @@ -4,16 +4,19 @@ Data-driven tests for libcli, in the style of [Test::Nginx](https://metacpan.org/dist/Test-Nginx). Each test is a block of text describing input to type into a running CLI session and the output to expect back. The library is compiled directly into a small C -driver built with sanitizers, so memory and undefined-behaviour errors -turn into test failures. +driver; built with sanitizers (`USE_ASAN=1`) or run under valgrind, +memory and undefined-behaviour errors turn into test failures. ## Running - make test # AddressSanitizer + UndefinedBehaviorSanitizer + make test # plain driver + USE_ASAN=1 make test # AddressSanitizer + UndefinedBehaviorSanitizer -`make test` builds `t/cli_driver` with `-fsanitize=address,undefined` and -runs `prove -r t/`. The same blocks can also be run under valgrind; see -[valgrind mode](#valgrind-mode). +`make test` builds `t/cli_driver` and runs `prove -r t/`. With +`USE_ASAN=1` the driver is built with `-fsanitize=address,undefined`. +The driver is not rebuilt when only `USE_ASAN` changes, so `rm +t/cli_driver` (or `make clean`) when switching. The same blocks can also +be run under valgrind; see [valgrind mode](#valgrind-mode). Run one file verbosely: @@ -156,14 +159,12 @@ Current known-bug blocks: ## valgrind mode Set `LIBCLI_VALGRIND=1` to run every block under Memcheck. ASan and -valgrind cannot share a binary, so build a non-sanitized driver first and -point `LIBCLI_DRIVER` at it: +valgrind cannot share a binary, so use the default (non-ASan) driver: - gcc -g -O0 -I. -std=c99 -o t/cli_driver_plain t/cli_driver.c libcli.c -lcrypt - LIBCLI_DRIVER=t/cli_driver_plain LIBCLI_VALGRIND=1 prove -r t/ + make t/cli_driver + LIBCLI_VALGRIND=1 prove -r t/ -Memcheck catches two -things the ASan build does not: reads of uninitialized memory, and leaks +Memcheck catches two things the ASan build does not: reads of uninitialized memory, and leaks (the ASan build runs with `detect_leaks=0`). Each block's timeout is scaled up automatically because Memcheck is ~20–30x slower. @@ -176,6 +177,6 @@ libcli.c:270). ## CI -`make test` is suitable for every push (seconds). The valgrind run +`USE_ASAN=1 make test` is suitable for every push (seconds). The valgrind run suits a nightly or pre-release job. Both need `libtest-base-perl`; valgrind additionally needs the `valgrind` package. diff --git a/t/lib/Test/LibCLI.pm b/t/lib/Test/LibCLI.pm index f081411..8a21720 100644 --- a/t/lib/Test/LibCLI.pm +++ b/t/lib/Test/LibCLI.pm @@ -49,9 +49,9 @@ my $Driver = $ENV{LIBCLI_DRIVER} || 't/cli_driver'; $ENV{ASAN_OPTIONS} ||= 'detect_leaks=0'; $ENV{UBSAN_OPTIONS} ||= 'print_stacktrace=1'; -# With LIBCLI_VALGRIND set, each driver run is wrapped in Memcheck. Use -# a non-sanitized driver for this (ASan and valgrind cannot coexist) by -# pointing LIBCLI_DRIVER at it; see "valgrind mode" in t/README.md. This +# With LIBCLI_VALGRIND set, each driver run is wrapped in Memcheck. The +# driver must be built without USE_ASAN (ASan and valgrind cannot +# coexist); see "valgrind mode" in t/README.md. This # catches uninitialized-memory reads and leaks that ASan does not. my @Valgrind = $ENV{LIBCLI_VALGRIND} ? ('valgrind', '-q', '--error-exitcode=99', '--leak-check=full', From d4a1c04bb6af4c19bd3ea0976b5446ecb170a0ec Mon Sep 17 00:00:00 2001 From: lynch Date: Sat, 10 Oct 2026 10:35:22 -0700 Subject: [PATCH 04/11] Build t/cli_driver by default --- Makefile | 2 +- t/README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 4961577..d16322c 100644 --- a/Makefile +++ b/Makefile @@ -26,7 +26,7 @@ ifeq (1,$(DYNAMIC_LIB)) TARGET_LIBS += $(LIB) endif -all: $(TARGET_LIBS) clitest +all: $(TARGET_LIBS) clitest t/cli_driver $(LIB_STATIC): libcli.o $(AR) $(ARFLAGS) $@ $^ diff --git a/t/README.md b/t/README.md index 5cd12a9..4f74e77 100644 --- a/t/README.md +++ b/t/README.md @@ -161,7 +161,7 @@ Current known-bug blocks: Set `LIBCLI_VALGRIND=1` to run every block under Memcheck. ASan and valgrind cannot share a binary, so use the default (non-ASan) driver: - make t/cli_driver + make LIBCLI_VALGRIND=1 prove -r t/ Memcheck catches two things the ASan build does not: reads of uninitialized memory, and leaks From 76da395999b0e9c69cfbf501b13ce6c57eca4fdd Mon Sep 17 00:00:00 2001 From: lynch Date: Sat, 10 Oct 2026 10:55:29 -0700 Subject: [PATCH 05/11] Fix crash when registering a duplicate optarg cli_register_optarg() jumps to its cleanup path with optarg still NULL when the name already exists, and cli_free_optarg() then dereferenced it. Make cli_free_optarg() NULL-safe and enable the regression test. Co-Authored-By: Claude Opus 5.5 --- libcli.c | 1 + t/README.md | 1 - t/completion.t | 3 +-- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/libcli.c b/libcli.c index 88c23d8..87695e5 100644 --- a/libcli.c +++ b/libcli.c @@ -2221,6 +2221,7 @@ static void cli_optarg_build_shortest(struct cli_optarg *optarg) { } void cli_free_optarg(struct cli_optarg *optarg) { + if (!optarg) return; free_z(optarg->help); free_z(optarg->name); free_z(optarg); diff --git a/t/README.md b/t/README.md index 4f74e77..93924af 100644 --- a/t/README.md +++ b/t/README.md @@ -148,7 +148,6 @@ Current known-bug blocks: | Block | libcli.c | Symptom | |-------|----------|---------| -| completion.t "duplicate optarg" | 2279 | `cli_free_optarg(NULL)` crash | | completion.t "overflow near line limit" | 1472 | tab-completion heap overflow | | completion.t "failed buildmode entry" | 1781 | NULL deref after freed buildmode | | basic.t "password may contain '?'" | 1544 | `?` swallowed in password states | diff --git a/t/completion.t b/t/completion.t index d913c51..01fde1d 100644 --- a/t/completion.t +++ b/t/completion.t @@ -82,8 +82,7 @@ paint ? -=== TEST 9: registering a duplicate optarg leaves a usable cli (KNOWN BUG libcli.c:2279) ---- SKIP +=== TEST 9: registering a duplicate optarg leaves a usable cli --- fixture: dupoptarg --- input set hello From d9ce48dc0e632bea1eceeacd314ba974bb7481fd Mon Sep 17 00:00:00 2001 From: lynch Date: Sat, 10 Oct 2026 10:55:32 -0700 Subject: [PATCH 06/11] Fix NULL dereference after a failed buildmode entry cli_int_enter_buildmode() frees cli->buildmode on every error path, but cli_loop() then printed cli->buildmode->cname. Report the failure using the entered command line instead, drop the redundant free, and enable the regression test with a check on the failure message. Co-Authored-By: Claude Opus 5.5 --- libcli.c | 5 ++--- t/README.md | 1 - t/completion.t | 4 ++-- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/libcli.c b/libcli.c index 87695e5..2997bbe 100644 --- a/libcli.c +++ b/libcli.c @@ -1777,9 +1777,8 @@ int cli_loop(struct cli_def *cli, int sockfd) { rc = cli_run_command(cli, cmd); switch (rc) { case CLI_BUILDMODE_ERROR: - // Unable to enter buildmode successfully - cli_print(cli, "Failure entering build mode for '%s'", cli->buildmode->cname); - cli_int_free_buildmode(cli); + // Unable to enter buildmode successfully; cli_int_enter_buildmode() has already freed cli->buildmode + cli_print(cli, "Failure entering build mode for '%s'", cmd); continue; case CLI_BUILDMODE_CANCEL: // Called if user enters 'cancel' diff --git a/t/README.md b/t/README.md index 93924af..68b82a1 100644 --- a/t/README.md +++ b/t/README.md @@ -149,7 +149,6 @@ Current known-bug blocks: | Block | libcli.c | Symptom | |-------|----------|---------| | completion.t "overflow near line limit" | 1472 | tab-completion heap overflow | -| completion.t "failed buildmode entry" | 1781 | NULL deref after freed buildmode | | basic.t "password may contain '?'" | 1544 | `?` swallowed in password states | | basic.t "password mid-line edit" | 1358 | cursor moved without compacting buffer | | known_bugs.t "unregister child" | 476 | parent `children` dangles (use-after-free) | diff --git a/t/completion.t b/t/completion.t index 01fde1d..0a3e058 100644 --- a/t/completion.t +++ b/t/completion.t @@ -100,11 +100,11 @@ set hello -=== TEST 11: a failed buildmode entry does not crash the cli (KNOWN BUG libcli.c:1781) ---- SKIP +=== TEST 11: a failed buildmode entry does not crash the cli --- fixture: buildmode --- input build go ping --- output_like +^Failure entering build mode for 'build go'$ ^RAN ping$ From 5621eeb4cb7b0249734cc6b991f2b47a3bc3905c Mon Sep 17 00:00:00 2001 From: lynch Date: Sat, 10 Oct 2026 11:07:54 -0700 Subject: [PATCH 07/11] Move the duplicate optarg test to known_bugs.t The block exercises optarg registration, not completion, so move it next to the other cross-cutting bug blocks and renumber completion.t. known_bugs.t now holds fixed (live) blocks as well as unfixed SKIP ones. Co-Authored-By: Claude Opus 5.5 --- t/README.md | 2 +- t/completion.t | 14 ++------------ t/known_bugs.t | 18 ++++++++++++++---- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/t/README.md b/t/README.md index 68b82a1..3850418 100644 --- a/t/README.md +++ b/t/README.md @@ -37,7 +37,7 @@ Requires Perl with `Test::Base` (Debian/Ubuntu: `libtest-base-perl`). editing.t line-editing keys (ctrl-a/e/b/f/k/w/u/z, backspace, insert) filters.t include/exclude/count/begin/between/grep/egrep completion.t tab and '?' completion, validators - known_bugs.t cross-cutting known-bug blocks (all SKIP) + known_bugs.t cross-cutting bug blocks (fixed ones live, unfixed SKIP) ## How a block runs diff --git a/t/completion.t b/t/completion.t index 0a3e058..bcdb542 100644 --- a/t/completion.t +++ b/t/completion.t @@ -82,17 +82,7 @@ paint ? -=== TEST 9: registering a duplicate optarg leaves a usable cli ---- fixture: dupoptarg ---- input -set hello ---- output_like -^RAN set$ -^OPTARG name=hello$ - - - -=== TEST 10: Tab completion near the line-length limit does not overflow (KNOWN BUG libcli.c:1472) +=== TEST 9: Tab completion near the line-length limit does not overflow (KNOWN BUG libcli.c:1472) --- SKIP --- fixture: completion --- input eval @@ -100,7 +90,7 @@ set hello -=== TEST 11: a failed buildmode entry does not crash the cli +=== TEST 10: a failed buildmode entry does not crash the cli --- fixture: buildmode --- input build go diff --git a/t/known_bugs.t b/t/known_bugs.t index 57d875a..e7956a5 100644 --- a/t/known_bugs.t +++ b/t/known_bugs.t @@ -1,10 +1,10 @@ use lib 't/lib'; use Test::LibCLI; -# Known, unfixed bugs that do not belong to a single topical test file. -# Each block is marked SKIP so the suite stays green; drop the "--- SKIP" -# line (and the "(KNOWN BUG ...)" note) as part of the fix to turn the -# block into a live regression test. +# Bugs that do not belong to a single topical test file. Unfixed ones +# are marked SKIP so the suite stays green; drop the "--- SKIP" line (and +# the "(KNOWN BUG ...)" note) as part of the fix to turn the block into a +# live regression test. run_tests(); __DATA__ @@ -28,3 +28,13 @@ counters ^RAN show version$ --- output_unlike Invalid command "ZZZshow" + + + +=== TEST 3: registering a duplicate optarg leaves a usable cli +--- fixture: dupoptarg +--- input +set hello +--- output_like +^RAN set$ +^OPTARG name=hello$ From 3676104d325fdfaf68bc336ae8050a4573af6d97 Mon Sep 17 00:00:00 2001 From: lynch Date: Sat, 10 Oct 2026 11:12:11 -0700 Subject: [PATCH 08/11] Fix heap overflow in Tab completion near the line-length limit Both completion branches erased the current word and copied the completion into the CLI_MAX_LINE_LENGTH buffer without a length check. Find the word start first (new cli_int_word_start() helper), ring the bell instead of completing when the result would not fit with its NUL terminator, and always terminate the buffer. Enable the regression test and add a boundary test showing that a completion which exactly fits still completes. Co-Authored-By: Claude Opus 5.5 --- libcli.c | 48 ++++++++++++++++++++++++++++++------------------ t/README.md | 1 - t/completion.t | 18 ++++++++++++++---- 3 files changed, 44 insertions(+), 23 deletions(-) diff --git a/libcli.c b/libcli.c index 2997bbe..f5c72a2 100644 --- a/libcli.c +++ b/libcli.c @@ -995,6 +995,12 @@ void cli_get_completions(struct cli_def *cli, const char *command, char lastchar cli_int_free_pipeline(pipeline); } +// Return the index where the word ending at position l starts +static int cli_int_word_start(const char *cmd, int l, int comma_separated) { + while (l > 0 && cmd[l - 1] != ' ' && cmd[l - 1] != '|' && !(comma_separated && cmd[l - 1] == ',')) l--; + return l; +} + static void cli_clear_line(int sockfd, char *cmd, int l, int cursor) { // Use cmd as our buffer, and overwrite contents as needed. // Backspace to beginning @@ -1465,18 +1471,24 @@ int cli_loop(struct cli_def *cli, int sockfd) { } else if (comphelp.num_entries == 1) { // Single completion - show *unless* the optional/required 'prefix' is present if (comphelp.entries[0][0] != '[' && comphelp.entries[0][0] != '<') { - for (; l > 0; l--, cursor--) { - if (cmd[l - 1] == ' ' || cmd[l - 1] == '|' || (comphelp.comma_separated && cmd[l - 1] == ',')) break; - _write(sockfd, "\b", 1); + int start = cli_int_word_start(cmd, l, comphelp.comma_separated); + int len = strlen(comphelp.entries[0]); + + // The completed word and its trailing space must fit, leaving room for the terminating NUL + if (start + len + 1 >= CLI_MAX_LINE_LENGTH) { + _write(sockfd, "\a", 1); + } else { + for (; l > start; l--, cursor--) _write(sockfd, "\b", 1); + memcpy(cmd + l, comphelp.entries[0], len); + l += len; + cmd[l++] = ' '; + cmd[l] = 0; + cursor = l; + _write(sockfd, comphelp.entries[0], len); + _write(sockfd, " ", 1); + // And now forget the tab, since we just found a single match + lastchar = '\0'; } - strcpy((cmd + l), comphelp.entries[0]); - l += strlen(comphelp.entries[0]); - cmd[l++] = ' '; - cursor = l; - _write(sockfd, comphelp.entries[0], strlen(comphelp.entries[0])); - _write(sockfd, " ", 1); - // And now forget the tab, since we just found a single match - lastchar = '\0'; } else { // Yes, we had a match, but it wasn't required - remember the tab in case the user double tabs.... lastchar = CTRL('I'); @@ -1518,14 +1530,14 @@ int cli_loop(struct cli_def *cli, int sockfd) { k = j; } - // Try to show minimum match string if we have a non-zero k and the first letter of the last word is not '['. - if (k && comphelp.entries[comphelp.num_entries - 1][0] != '[') { - for (; l > 0; l--, cursor--) { - if (cmd[l - 1] == ' ' || cmd[l - 1] == '|' || (comphelp.comma_separated && cmd[l - 1] == ',')) break; - _write(sockfd, "\b", 1); - } - strncpy(cmd + l, tptr, k); + // Try to show minimum match string if we have a non-zero k and the first letter of the last word is not '[', + // and the result fits in the buffer with room for the terminating NUL. + int start = cli_int_word_start(cmd, l, comphelp.comma_separated); + if (k && comphelp.entries[comphelp.num_entries - 1][0] != '[' && start + k < CLI_MAX_LINE_LENGTH) { + for (; l > start; l--, cursor--) _write(sockfd, "\b", 1); + memcpy(cmd + l, tptr, k); l += k; + cmd[l] = 0; cursor = l; _write(sockfd, tptr, k); diff --git a/t/README.md b/t/README.md index 3850418..6c8eeec 100644 --- a/t/README.md +++ b/t/README.md @@ -148,7 +148,6 @@ Current known-bug blocks: | Block | libcli.c | Symptom | |-------|----------|---------| -| completion.t "overflow near line limit" | 1472 | tab-completion heap overflow | | basic.t "password may contain '?'" | 1544 | `?` swallowed in password states | | basic.t "password mid-line edit" | 1358 | cursor moved without compacting buffer | | known_bugs.t "unregister child" | 476 | parent `children` dangles (use-after-free) | diff --git a/t/completion.t b/t/completion.t index bcdb542..68b5d23 100644 --- a/t/completion.t +++ b/t/completion.t @@ -82,15 +82,25 @@ paint ? -=== TEST 9: Tab completion near the line-length limit does not overflow (KNOWN BUG libcli.c:1472) ---- SKIP +=== TEST 9: Tab completion past the line-length limit rings the bell, no overflow --- fixture: completion --- input eval -"neighbor" . (" " x 4080) . "\n" +"neighbor" . (" " x 4074) . "\n" +--- output_unlike +^RAN neighbor advertisement$ + + + +=== TEST 10: Tab completion that exactly fits the line-length limit still completes +--- fixture: completion +--- input eval +"neighbor" . (" " x 4073) . "\n" +--- output_like +^RAN neighbor advertisement$ -=== TEST 10: a failed buildmode entry does not crash the cli +=== TEST 11: a failed buildmode entry does not crash the cli --- fixture: buildmode --- input build go From 8e6c64f3d544acb63e1625340f7d82aa7f82cbb6 Mon Sep 17 00:00:00 2001 From: lynch Date: Sat, 10 Oct 2026 11:14:03 -0700 Subject: [PATCH 09/11] Fix use-after-free after unregistering a first child command cli_free_command() updated the previous/next siblings and cli->commands but not parent->children, so freeing a parent's first child left the parent pointing at freed memory. Move parent->children on to the next sibling. Enable the regression test and also check that the parent still resolves after its child is gone. Co-Authored-By: Claude Opus 5.5 --- libcli.c | 3 +++ t/README.md | 1 - t/known_bugs.t | 6 ++++-- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/libcli.c b/libcli.c index f5c72a2..12b15fc 100644 --- a/libcli.c +++ b/libcli.c @@ -480,6 +480,9 @@ static void cli_free_command(struct cli_def *cli, struct cli_command *cmd) { cmd->next->previous = NULL; } } else { + if (cmd->parent && cmd->parent->children == cmd) { + cmd->parent->children = cmd->next; + } if (cmd->previous) { cmd->previous->next = cmd->next; } diff --git a/t/README.md b/t/README.md index 6c8eeec..6628666 100644 --- a/t/README.md +++ b/t/README.md @@ -150,7 +150,6 @@ Current known-bug blocks: |-------|----------|---------| | basic.t "password may contain '?'" | 1544 | `?` swallowed in password states | | basic.t "password mid-line edit" | 1358 | cursor moved without compacting buffer | -| known_bugs.t "unregister child" | 476 | parent `children` dangles (use-after-free) | | known_bugs.t "telnet subnegotiation" | 1251 | no SB…SE state machine; payload injected | ## valgrind mode diff --git a/t/known_bugs.t b/t/known_bugs.t index e7956a5..6bcd192 100644 --- a/t/known_bugs.t +++ b/t/known_bugs.t @@ -9,11 +9,13 @@ run_tests(); __DATA__ -=== TEST 1: unregistering a child command leaves the parent usable (KNOWN BUG libcli.c:476) ---- SKIP +=== TEST 1: unregistering a child command leaves the parent usable --- fixture: unreg_child --- input show ? +show +--- output_like +^No callback for "show"$ --- output_unlike counters From e47f23ef86c31c37133dad887cd8392d74549685 Mon Sep 17 00:00:00 2001 From: lynch Date: Sat, 10 Oct 2026 11:16:56 -0700 Subject: [PATCH 10/11] Fix leak of the enable password in cli_done() cli_allow_enable() strdup's cli->enable_password but cli_done() never freed it. Free it, and drop the valgrind suppression that hid the leak so the valgrind run now covers it. Co-Authored-By: Claude Opus 5.5 --- libcli.c | 1 + t/README.md | 5 ++--- t/valgrind.supp | 15 ++------------- 3 files changed, 5 insertions(+), 16 deletions(-) diff --git a/libcli.c b/libcli.c index 12b15fc..730941d 100644 --- a/libcli.c +++ b/libcli.c @@ -745,6 +745,7 @@ int cli_done(struct cli_def *cli) { free_z(cli->banner); free_z(cli->promptchar); free_z(cli->hostname); + free_z(cli->enable_password); free_z(cli->buffer); free_z(cli); diff --git a/t/README.md b/t/README.md index 6628666..82b40ad 100644 --- a/t/README.md +++ b/t/README.md @@ -167,9 +167,8 @@ scaled up automatically because Memcheck is ~20–30x slower. `t/valgrind.supp` baselines known pre-existing leaks so the gate still fails on *new* ones. Each suppression names the bug it hides; remove the suppression as part of the fix so the exercising test turns into live -leak coverage. It currently suppresses one leak: `cli_done()` does not -free `cli->enable_password` (allocated in `cli_allow_enable`, -libcli.c:270). +leak coverage. There are currently no known leaks, so the file holds no +suppressions. ## CI diff --git a/t/valgrind.supp b/t/valgrind.supp index 4d6bc20..2941bca 100644 --- a/t/valgrind.supp +++ b/t/valgrind.supp @@ -4,16 +4,5 @@ # still catch NEW ones. Each entry names the bug it hides; remove the # entry as part of the fix so the exercising test turns into live leak # coverage. - -# KNOWN BUG: cli_done() does not free cli->enable_password, which -# cli_allow_enable() strdup's at libcli.c:270. Leaked on every cli that -# sets a static enable password. -{ - cli_done-leaks-enable_password - Memcheck:Leak - match-leak-kinds: definite - fun:malloc - fun:strdup - fun:cli_allow_enable - fun:main -} +# +# There are currently no known leaks. From bedb38ccc6d1d366528359fbf807e1b014707dcf Mon Sep 17 00:00:00 2001 From: lynch Date: Sun, 11 Oct 2026 10:01:10 -0700 Subject: [PATCH 11/11] Add cli-aware auth/enable callbacks and cli_read_password() The auth and enable callbacks get only the typed strings, so a threaded server cannot tell which session is logging in, and cannot end a session after repeated failures. Add: - cli_set_auth_callback_ctx() and cli_set_enable_callback_ctx(): the callback also gets the cli_def. It returns CLI_OK to accept, CLI_QUIT to end the session, anything else to deny. The line after the unechoed password is ended first, so its messages start on their own line. - cli_read_password(): reads a line without echo from inside a command callback, e.g. for "username bob secret". The line stays out of the history; ctrl-c cancels; telnet options are skipped; the idle timeout applies. - Typed usernames and passwords, and the enable password, are wiped before they are freed. A failed strdup() in the login states now leaves cli_loop() through its cleanup instead of returning early. New t/auth.t covers both callbacks and cli_read_password() with the new auth_ctx fixture. Co-Authored-By: Claude Opus 5.5 --- README.md | 14 ++++ libcli.c | 173 ++++++++++++++++++++++++++++++++++++++++--- libcli.h | 52 +++++++++++++ t/README.md | 2 + t/auth.t | 197 +++++++++++++++++++++++++++++++++++++++++++++++++ t/cli_driver.c | 135 +++++++++++++++++++++++++++++++++ 6 files changed, 561 insertions(+), 12 deletions(-) create mode 100644 t/auth.t diff --git a/README.md b/README.md index 3dde870..96e12f3 100644 --- a/README.md +++ b/README.md @@ -79,6 +79,20 @@ return `CLI_OK` if the user is to have access and `CLI_ERROR` if they are not. The library itself will take care of prompting the user for credentials. +In a threaded server, where the callback needs to know which session it is +checking, use `cli_set_auth_callback_ctx(cli, callback)` instead. Its callback +also gets the `cli_def`, so it can reach `cli_get_context()` and print with +`cli_print()`. It returns `CLI_OK` to accept, `CLI_QUIT` to end the session +(e.g. after too many failures), or anything else to deny. +`cli_set_enable_callback_ctx(cli, callback)` does the same for `enable`. +The buffers holding typed passwords are wiped after use. + +A command can ask for a password itself, e.g. `username bob secret`, with +`cli_read_password(cli, prompt, buf, size)`. The line is not echoed and never +enters the history; ctrl-c cancels it. It returns `CLI_OK`, `CLI_ERROR` +(cancelled or too long), or `CLI_QUIT` (connection closed or idle timeout), +which the command should return. + Commands are built using a tree-like structure. You define commands with the `cli_register_command(parent, command, callback, privilege, mode, help)` function. diff --git a/libcli.c b/libcli.c index 730941d..a2249ef 100644 --- a/libcli.c +++ b/libcli.c @@ -134,6 +134,23 @@ struct cli_filter_cmds { } \ } while (0) +// memset through a volatile pointer, so the compiler cannot drop it before a free() +static void *(*const volatile cli_wipe_memset)(void *, int, size_t) = memset; + +static void cli_wipe(void *p, size_t n) { + if (p) cli_wipe_memset(p, 0, n); +} + +// Wipe a string, then free and zero it +#define free_wipe(p) \ + do { \ + if (p) { \ + cli_wipe((p), strlen(p)); \ + free(p); \ + (p) = 0; \ + } \ + } while (0) + // Forward defines of *INTERNAL* library function as static here static int cli_search_flags_validator(struct cli_def *cli, const char *word, const char *value); static int cli_match_filter_init(struct cli_def *cli, int argc, char **argv, struct cli_filter *filt); @@ -237,6 +254,15 @@ void cli_set_enable_callback(struct cli_def *cli, int (*enable_callback)(const c cli->enable_callback = enable_callback; } +void cli_set_auth_callback_ctx(struct cli_def *cli, + int (*auth_callback)(struct cli_def *cli, const char *username, const char *password)) { + cli->auth_callback_ctx = auth_callback; +} + +void cli_set_enable_callback_ctx(struct cli_def *cli, int (*enable_callback)(struct cli_def *cli, const char *password)) { + cli->enable_callback_ctx = enable_callback; +} + void cli_allow_user(struct cli_def *cli, const char *username, const char *password) { struct unp *u, *n; if (!(n = malloc(sizeof(struct unp)))) { @@ -532,7 +558,7 @@ int cli_show_help(struct cli_def *cli, struct cli_command *c) { int cli_enable(struct cli_def *cli, UNUSED(const char *command), UNUSED(char *argv[]), UNUSED(int argc)) { if (cli->privilege == PRIVILEGE_PRIVILEGED) return CLI_OK; - if (!cli->enable_password && !cli->enable_callback) { + if (!cli->enable_password && !cli->enable_callback && !cli->enable_callback_ctx) { // No password required, set privilege immediately. cli_set_privilege(cli, PRIVILEGE_PRIVILEGED); cli_set_configmode(cli, MODE_EXEC, NULL); @@ -745,7 +771,7 @@ int cli_done(struct cli_def *cli) { free_z(cli->banner); free_z(cli->promptchar); free_z(cli->hostname); - free_z(cli->enable_password); + free_wipe(cli->enable_password); free_z(cli->buffer); free_z(cli); @@ -1131,7 +1157,7 @@ int cli_loop(struct cli_def *cli, int sockfd) { cli_set_configmode(cli, MODE_EXEC, NULL); // No auth required? - if (!cli->users && !cli->auth_callback) cli->state = STATE_NORMAL; + if (!cli->users && !cli->auth_callback && !cli->auth_callback_ctx) cli->state = STATE_NORMAL; while (1) { signed int in_history = 0; @@ -1732,19 +1758,35 @@ int cli_loop(struct cli_def *cli, int sockfd) { // Require login free_z(username); - if (!(username = strdup(cmd))) return 0; + if (!(username = strdup(cmd))) break; cli->state = STATE_PASSWORD; cli->showprompt = 1; } else if (cli->state == STATE_PASSWORD) { // Require password - int allowed = 0; + int allowed = 0, quit = 0; - free_z(password); - if (!(password = strdup(cmd))) return 0; + free_wipe(password); + if (!(password = strdup(cmd))) break; + cli_wipe(cmd, l); if (cli->auth_callback) { if (cli->auth_callback(username, password) == CLI_OK) allowed++; } + if (!allowed && cli->auth_callback_ctx) { + // The Enter after a password is not echoed; end the line so the callback's messages start on their own + _write(sockfd, "\r\n", 2); + switch (cli->auth_callback_ctx(cli, username, password)) { + case CLI_OK: + allowed++; + break; + case CLI_QUIT: + quit++; + break; + } + } + + if (quit) break; + if (!allowed) { struct unp *u; for (u = cli->users; u; u = u->next) { @@ -1755,19 +1797,19 @@ int cli_loop(struct cli_def *cli, int sockfd) { } } + free_wipe(password); if (allowed) { cli_error(cli, " "); cli->state = STATE_NORMAL; } else { cli_error(cli, "\n\nAccess denied"); - free_z(username); - free_z(password); + free_wipe(username); cli->state = STATE_LOGIN; } cli->showprompt = 1; } else if (cli->state == STATE_ENABLE_PASSWORD) { - int allowed = 0; + int allowed = 0, quit = 0; if (cli->enable_password) { // Check stored static enable password if (pass_matches(cli->enable_password, cmd)) allowed++; @@ -1778,6 +1820,21 @@ int cli_loop(struct cli_def *cli, int sockfd) { if (cli->enable_callback(cmd)) allowed++; } + if (!allowed && cli->enable_callback_ctx) { + _write(sockfd, "\r\n", 2); + switch (cli->enable_callback_ctx(cli, cmd)) { + case CLI_OK: + allowed++; + break; + case CLI_QUIT: + quit++; + break; + } + } + + cli_wipe(cmd, l); + if (quit) break; + if (allowed) { cli->state = STATE_ENABLE; cli_set_privilege(cli, PRIVILEGE_PRIVILEGED); @@ -1824,8 +1881,9 @@ int cli_loop(struct cli_def *cli, int sockfd) { } cli_free_history(cli); - free_z(username); - free_z(password); + free_wipe(username); + free_wipe(password); + cli_wipe(cmd, CLI_MAX_LINE_LENGTH); free_z(cmd); fclose(cli->client); @@ -1833,6 +1891,97 @@ int cli_loop(struct cli_def *cli, int sockfd) { return CLI_OK; } +int cli_read_password(struct cli_def *cli, const char *prompt, char *buf, size_t size) { + int sockfd, telnet_option = 0, too_long = 0, rc = CLI_QUIT; + size_t l = 0; + unsigned char c; + struct timeval tm; + + if (!cli->client || !buf || size < 1) return CLI_ERROR; + sockfd = fileno(cli->client); + buf[0] = 0; + if (prompt) _write(sockfd, prompt, strlen(prompt)); + + while (1) { + int sr; + ssize_t n; + + tm.tv_sec = 1; + tm.tv_usec = 0; + if ((sr = cli_socket_wait(sockfd, &tm)) < 0) { + if (errno == EINTR) continue; + break; + } + + if (sr == 0) { + if (cli->idle_timeout && time(NULL) - cli->last_action >= cli->idle_timeout) break; + continue; + } + + if ((n = read(sockfd, &c, 1)) < 0) { + if (errno == EINTR) continue; + break; + } + + if (n == 0) break; + if (cli->idle_timeout) time(&cli->last_action); + + // Telnet: IAC, then a WILL/WONT/DO/DONT and its option byte; IAC IAC is a literal 255 + if (telnet_option) { + if (telnet_option == 255 && c >= 251 && c <= 254) { + telnet_option = c; + continue; + } + + telnet_option = 0; + if (c != 255) continue; + } else if (c == 255) { + telnet_option = 255; + continue; + } + + // A line ends with \r; the \n or \0 after it, left over from the command line too, is dropped + if (c == 0 || c == '\n') continue; + + if (c == '\r') { + rc = too_long ? CLI_ERROR : CLI_OK; + break; + } + + if (c == CTRL('C')) { + rc = CLI_ERROR; + break; + } + + if (c == CTRL('H') || c == 0x7f) { + if (l > 0) buf[--l] = 0; + continue; + } + + if (c == CTRL('U')) { + cli_wipe(buf, l); + l = 0; + continue; + } + + if (c < ' ') continue; + + if (l + 1 >= size) { + // Too long: read on to the end of the line, then fail + too_long = 1; + continue; + } + + buf[l++] = c; + buf[l] = 0; + } + + _write(sockfd, "\r\n", 2); + if (rc != CLI_OK) cli_wipe(buf, size); + + return rc; +} + int cli_file(struct cli_def *cli, FILE *fh, int privilege, int mode) { int oldpriv = cli_set_privilege(cli, privilege); int oldmode = cli_set_configmode(cli, mode, NULL); diff --git a/libcli.h b/libcli.h index d5673fc..471527e 100644 --- a/libcli.h +++ b/libcli.h @@ -91,6 +91,8 @@ struct cli_def { int disallow_buildmode; struct cli_pipeline *pipeline; struct cli_buildmode *buildmode; + int (*auth_callback_ctx)(struct cli_def *cli, const char *username, const char *password); + int (*enable_callback_ctx)(struct cli_def *cli, const char *password); }; struct cli_filter { @@ -336,6 +338,56 @@ void cli_set_auth_callback(struct cli_def *cli, int (*auth_callback)(const char */ void cli_set_enable_callback(struct cli_def *cli, int (*enable_callback)(const char *)); +/** + * @brief like cli_set_auth_callback(), but the callback also gets the cli, + * so it can reach cli_get_context() and print with cli_print() + * + * @note the callback returns CLI_OK to accept the login, CLI_QUIT to end + * the session (e.g. after too many failures), or anything else to + * deny it and prompt for the username again; the buffers holding + * the username and password are wiped after the call, so the + * callback must copy what it wants to keep + * + * @param cli target cli object + * @param[in] auth_callback username/password checker, or NULL to remove it + */ +void cli_set_auth_callback_ctx(struct cli_def *cli, + int (*auth_callback)(struct cli_def *cli, const char *username, const char *password)); + +/** + * @brief like cli_set_enable_callback(), but the callback also gets the + * cli + * + * @note the callback returns CLI_OK to enter privileged mode, CLI_QUIT to + * end the session, or anything else to deny it; unlike the + * callback of cli_set_enable_callback(), CLI_OK (0) means success + * + * @param cli target cli object + * @param[in] enable_callback enable password checker, or NULL to remove it + */ +void cli_set_enable_callback_ctx(struct cli_def *cli, int (*enable_callback)(struct cli_def *cli, const char *password)); + +/** + * @brief reads a line without echoing it, from inside a command callback + * running under cli_loop(); for asking a password, e.g. "username + * bob secret"; the line never enters the history + * + * @note backspace deletes a character, ctrl-u clears the line, ctrl-c + * cancels; telnet option negotiation is skipped; the idle timeout + * of cli_set_idle_timeout() applies, without its callback + * + * @param cli target cli object + * @param[in] prompt text written first, e.g. "Password: " + * @param[out] buf receives the line, NUL-terminated + * @param[in] size size of buf, at least 1 + * + * @return CLI_OK with the line in buf; CLI_ERROR if cancelled with ctrl-c, + * too long for buf, or not under cli_loop(); CLI_QUIT if the + * connection closed or timed out, and the command should return + * CLI_QUIT; buf is wiped on any error + */ +int cli_read_password(struct cli_def *cli, const char *prompt, char *buf, size_t size); + /** * @brief function to add a username/password pair; this is a dynamic * version of using cli_set_auth_callback diff --git a/t/README.md b/t/README.md index 82b40ad..df13450 100644 --- a/t/README.md +++ b/t/README.md @@ -34,6 +34,7 @@ Requires Perl with `Test::Base` (Debian/Ubuntu: `libtest-base-perl`). lib/Test/LibCLI.pm the harness: forks the driver, feeds input, checks output valgrind.supp baseline suppressions for known, pre-existing leaks basic.t commands, auth, enable, config mode, history + auth.t cli-aware auth/enable callbacks, cli_read_password() editing.t line-editing keys (ctrl-a/e/b/f/k/w/u/z, backspace, insert) filters.t include/exclude/count/begin/between/grep/egrep completion.t tab and '?' completion, validators @@ -122,6 +123,7 @@ Fixtures currently available (`--- fixture:`): | `dupoptarg` | registers a duplicate optarg, then exercises the command | | `buildmode` | a build-mode command whose optarg name collides with a reserved one | | `unreg_child` | registers `show counters`, then unregisters the child | +| `auth_ctx` | login and enable through the cli-aware callbacks; `secret` and `short` read hidden input | ### Adding a fixture diff --git a/t/auth.t b/t/auth.t new file mode 100644 index 0000000..b13daaa --- /dev/null +++ b/t/auth.t @@ -0,0 +1,197 @@ +use lib 't/lib'; +use Test::LibCLI; +run_tests(); + +__DATA__ + +=== TEST 1: cli-aware login callback accepts, password not echoed +--- fixture: auth_ctx +--- input +alice +pw1 +reload +--- output_like +^Username: alice$ +^AUTH alice$ +^Invalid command "reload"$ +--- output_unlike +pw1 +Access denied + + + +=== TEST 2: denied login prompts for the username again +--- fixture: auth_ctx +--- input +alice +wrong +alice +pw1 +--- output_like +Access denied +^AUTH alice$ +^router> +--- output_unlike +wrong + + + +=== TEST 3: CLI_QUIT from the login callback ends the session +--- fixture: auth_ctx +--- input +bob +x +bob +y +bob +z +alice +pw1 +--- output_like +TOO MANY FAILURES +--- output_unlike +AUTH +^router> + + + +=== TEST 4: cli-aware enable callback accepts +--- fixture: auth_ctx +--- input +alice +pw1 +enable +en +reload +--- output_like +^ENABLE OK$ +^router# reload$ +^RAN reload$ +--- output_unlike +^en$ +Access denied + + + +=== TEST 5: cli-aware enable callback denies +--- fixture: auth_ctx +--- input +alice +pw1 +enable +nope +reload +--- output_like +Access denied +^router> reload$ +^Invalid command "reload"$ +--- output_unlike +nope + + + +=== TEST 6: CLI_QUIT from the enable callback ends the session +--- fixture: auth_ctx +--- input +alice +pw1 +enable +bye +reload +--- output_like +^ENABLE QUIT$ +--- output_unlike +RAN reload +Invalid command + + + +=== TEST 7: cli_read_password reads two hidden lines, kept out of history +--- fixture: auth_ctx +--- input +alice +pw1 +secret +abc +xyz +history +--- output_like +^New password:\s*$ +^Confirm password:\s*$ +^RC1 0$ +^RC2 0$ +^GOT \[ABC\] \[XYZ\]$ +--- output_unlike +abc +xyz + + + +=== TEST 8: cli_read_password handles backspace and ctrl-u +--- fixture: auth_ctx +--- input +alice +pw1 +secret +abc +zzq +--- output_like +^GOT \[AC\] \[Q\]$ + + + +=== TEST 9: ctrl-c cancels cli_read_password, the session goes on +--- fixture: auth_ctx +--- input +alice +pw1 +secret +ab +reload +--- output_like +^RC1 -1$ +^Invalid command "reload"$ +--- output_unlike +RC2 +GOT + + + +=== TEST 10: a line too long for the buffer fails, and is read to its end +--- fixture: auth_ctx +--- input +alice +pw1 +short +abcdef +short +abc +reload +--- output_like +^RC -1 LEN 0$ +^RC 0 LEN 3$ +^Invalid command "reload"$ +--- output_unlike +abc +def + + + +=== TEST 11: EOF while reading a password returns CLI_QUIT +--- fixture: auth_ctx +--- input eval +"alice\npw1\nsecret\nab" +--- output_like +^RC1 -2$ +--- output_unlike +GOT + + + +=== TEST 12: telnet options inside a password are skipped +--- fixture: auth_ctx +--- telnet +--- input eval +"alice\npw1\nsecret\na\xff\xfd\x03b\nc\xff\xfb\x01\n" +--- output_like +^GOT \[AB\] \[C\]$ diff --git a/t/cli_driver.c b/t/cli_driver.c index b7ce645..0fd43ac 100644 --- a/t/cli_driver.c +++ b/t/cli_driver.c @@ -18,6 +18,7 @@ #include "libcli.h" +#include #include #include #include @@ -47,6 +48,14 @@ static int drv_fixture_validate(struct cli_def *cli); static int drv_fixture_dupoptarg(struct cli_def *cli); static int drv_fixture_buildmode(struct cli_def *cli); static int drv_fixture_unreg_child(struct cli_def *cli); +static int drv_fixture_auth_ctx(struct cli_def *cli); +static int drv_auth_ctx(struct cli_def *cli, const char *username, + const char *password); +static int drv_enable_ctx(struct cli_def *cli, const char *password); +static int drv_cmd_secret(struct cli_def *cli, const char *command, + char *argv[], int argc); +static int drv_cmd_short(struct cli_def *cli, const char *command, + char *argv[], int argc); static int drv_validate_color(struct cli_def *cli, const char *name, const char *value); static const drv_fixture_t *drv_find_fixture(const char *name); @@ -61,6 +70,7 @@ static const drv_fixture_t drv_fixtures[] = { { "dupoptarg", drv_fixture_dupoptarg }, { "buildmode", drv_fixture_buildmode }, { "unreg_child", drv_fixture_unreg_child }, + { "auth_ctx", drv_fixture_auth_ctx }, { NULL, NULL } }; @@ -441,6 +451,131 @@ drv_fixture_unreg_child(struct cli_def *cli) } +/* + * auth_ctx: login and enable go through the cli-aware callbacks, and two + * commands read hidden input with cli_read_password(). + * + * login alice/pw1 is accepted; the third failure ends the session + * enable "en" is accepted, "bye" ends the session + * secret reads a password and its confirmation, prints both + * upper-cased (so an echo of the typed text would show) + * short reads into a 4-byte buffer + */ +static int drv_auth_failures; + + +static int +drv_fixture_auth_ctx(struct cli_def *cli) +{ + cli_set_auth_callback_ctx(cli, drv_auth_ctx); + cli_set_enable_callback_ctx(cli, drv_enable_ctx); + cli_set_context(cli, &drv_auth_failures); + + if (cli_register_command(cli, NULL, "secret", drv_cmd_secret, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Read a password twice") == NULL + || cli_register_command(cli, NULL, "short", drv_cmd_short, + PRIVILEGE_UNPRIVILEGED, MODE_EXEC, + "Read into a small buffer") == NULL + || cli_register_command(cli, NULL, "reload", drv_cmd_print, + PRIVILEGE_PRIVILEGED, MODE_EXEC, + "Reload the system") == NULL) + { + return CLI_ERROR; + } + + return CLI_OK; +} + + +static int +drv_auth_ctx(struct cli_def *cli, const char *username, const char *password) +{ + int *failures; + + failures = cli_get_context(cli); + + if (strcmp(username, "alice") == 0 && strcmp(password, "pw1") == 0) { + cli_print(cli, "AUTH %s", username); + return CLI_OK; + } + + if (++*failures >= 3) { + cli_print(cli, "TOO MANY FAILURES"); + return CLI_QUIT; + } + + return CLI_ERROR; +} + + +static int +drv_enable_ctx(struct cli_def *cli, const char *password) +{ + if (strcmp(password, "en") == 0) { + cli_print(cli, "ENABLE OK"); + return CLI_OK; + } + + if (strcmp(password, "bye") == 0) { + cli_print(cli, "ENABLE QUIT"); + return CLI_QUIT; + } + + return CLI_ERROR; +} + + +static int +drv_cmd_secret(struct cli_def *cli, const char *command, char *argv[], + int argc) +{ + int rc1, rc2; + char *p; + char a[64], b[64]; + + rc1 = cli_read_password(cli, "New password: ", a, sizeof(a)); + cli_print(cli, "RC1 %d", rc1); + + if (rc1 != CLI_OK) { + return rc1 == CLI_QUIT ? CLI_QUIT : CLI_OK; + } + + rc2 = cli_read_password(cli, "Confirm password: ", b, sizeof(b)); + cli_print(cli, "RC2 %d", rc2); + + if (rc2 != CLI_OK) { + return rc2 == CLI_QUIT ? CLI_QUIT : CLI_OK; + } + + for (p = a; *p; p++) { + *p = toupper((unsigned char) *p); + } + + for (p = b; *p; p++) { + *p = toupper((unsigned char) *p); + } + + cli_print(cli, "GOT [%s] [%s]", a, b); + + return CLI_OK; +} + + +static int +drv_cmd_short(struct cli_def *cli, const char *command, char *argv[], + int argc) +{ + int rc; + char buf[4]; + + rc = cli_read_password(cli, "Password: ", buf, sizeof(buf)); + cli_print(cli, "RC %d LEN %d", rc, (int) strlen(buf)); + + return CLI_OK; +} + + static int drv_validate_color(struct cli_def *cli, const char *name, const char *value) {