nrf_cli_help_print breaks when length of optname + optname_short approaches the line width

[Lumdev]

There are two issues in one here. The easy fix is the check for overflow in format_offset_string_print()
if (length <= p_cli->p_ctx->vt100_ctx.cons.terminal_wid - terminal_offset) 

This line doesn't recognize when the terminal_offset is greater than the terminal_wid. To check this the comparison should be reversed to 

if !(length >= p_cli->p_ctx->vt100_ctx.cons.terminal_wid - terminal_offset) 

Second is the handling of wrapping, see below for examples of how the wrapping breaks down and ultimately fails as the length of optname + optname_short approachs 75 (terminal width - padding chars)

nrf_cli_help_print inputs:

nrf_cli_getopt_option_t help[1] = {{"optname..........20","short............20", "helptext.........20"}};
nrf_cli_help_print(p_cli, help, 1);

nrf_cli_getopt_option_t help1[1] = {{"optname..............40..............40","short..10", "helptext........30.........30"}};
nrf_cli_help_print(p_cli, help1, 1);

nrf_cli_getopt_option_t help2[1] = {{"optname..........20","short........50...............50...............50", "helptext...........20"}};
nrf_cli_help_print(p_cli, help3, 1);

nrf_cli_getopt_option_t help3[1] = {{"optname10","short...............64.............64...........64...........64", "helptext.........20"}};
nrf_cli_help_print(p_cli, help3, 1);

nrf_cli_getopt_option_t help4[1] = {{"optname10","short...............66..............66............66...........66", "helptext.........20"}};
nrf_cli_help_print(p_cli, help5, 1);

nrf_cli_getopt_option_t help5[1] = {{"optname10","short...............65..............65............65..........65", "helptext.........20"}};
nrf_cli_help_print(p_cli, help5, 1);

console output:

With short input everything is okay:

00> Options:
00>   -h, --help                                :Show command help.
00>   short............20, optname..........20:helptext.........20


With long input shorter options, helptext correctly wraps:

00> Options:
00>   -h, --help                                          :Show command help.
00>   short..10, optname..............40..............40:helptext........30.......
00> ..30


With options approaching line width limit(80), help text and "Show command help." wrapping starts to break:

00> Options:
00>   -h, --help                                                              :Show
00> comma
00> nd
00> help.
00>   short........50...............50...............50, optname..........20:helpt
00> ext..
00> .....
00> ....2
00> 0

With options at 74 chars line width limit(80), help text and "Show command help." wrapping is at its worst:

00> Options:
00>   -h, --help                                                                  :S
00> h
00> o
00> w
00> c
00> o
00> m
00> m
00> a
00> n
00> d
00> h
00> e
00> l
00> p
00> .
00>   short...............64.............64...........64...........64, optname10:h
00> e
00> l
00> p
00> t
00> e
00> x
00> t
00> .
00> .
00> .
00> .
00> .
00> .
00> .
00> .
00> .
00> 2
00> 0


With options at 76 chars the length check fails and the line does not wrap, formatting looks fine but is wider than 80 chars:

00> Options:
00>   -h, --help                                                                    :Show command help.
00>   short...............66..............66............66...........66, optname10:helptext.........20


With options length at exactly 75 chars wrapping is stuck in an infinite loop until watchdog causes a reboot:

00> Options:
00>   -h, --help                                                                   :
00> 
00> 
00> 
00> 
00> 
00> 
00> 
00> 
00> 
00> 

Has a workaround for this wrapping already been implemented or will we need to implement ourselves?

[Einar Thorsrud]

Hi,

Thanks for reporting. I have created a ticket for the first issue. Regarding the line wrapping I have not found any references to it nor do I have a workaround to suggest at this point. Which SDK version do you see this on?

[Lumdev]

Thanks Einar, we ended up doing the workaround ourselves. Either of the two attached patches addresses the issue where aa 74 char option+short length causes infinite looping/crashing. The first one keeps to 80 char wrapping, the second simply removed wrapping. Nothing additional needed on my end, just leaving these here in the hopes it helps someone else in the future

Date: Thu, 17 Jun 2021 16:57:02 -0400
Subject: [PATCH] [ULC-755]Fix infinite loop error, mostly align to 80 chars

---
 .../v17.0.2/components/libraries/cli/nrf_cli.c    | 15 +++++++++++++--

diff --git a/shared/nordic/nRF5x/sdk/v17.0.2/components/libraries/cli/nrf_cli.c b/shared/nordic/nRF5x/sdk/v17.0.2/components/libraries/cli/nrf_cli.c
index 49618815..5d261f27 100644
--- a/shared/nordic/nRF5x/sdk/v17.0.2/components/libraries/cli/nrf_cli.c
+++ b/shared/nordic/nRF5x/sdk/v17.0.2/components/libraries/cli/nrf_cli.c
@@ -2974,7 +2974,7 @@ static void format_offset_string_print(nrf_cli_t const * p_cli,
         size_t idx = 0;
         length = cli_strlen(p_str) - offset;
 
-        if (length <= p_cli->p_ctx->vt100_ctx.cons.terminal_wid - terminal_offset)
+        if (length + terminal_offset <= p_cli->p_ctx->vt100_ctx.cons.terminal_wid)
         {
             for (idx = 0; idx < length; idx++)
             {
@@ -2998,6 +2998,11 @@ static void format_offset_string_print(nrf_cli_t const * p_cli,
                to not divide words. */
             length = p_cli->p_ctx->vt100_ctx.cons.terminal_wid - terminal_offset;
 
+            if (terminal_offset > p_cli->p_ctx->vt100_ctx.cons.terminal_wid)
+            {
+                length = 0;
+            }
+
             while (1)
             {
                 /* Determining line break. */
@@ -3027,6 +3032,7 @@ static void format_offset_string_print(nrf_cli_t const * p_cli,
             {
                 ++offset;
             }
+            terminal_offset = 0;
             cursor_next_line_move(p_cli);
             cursor_right_move(p_cli, terminal_offset);
         }
@@ -3073,7 +3079,12 @@ void nrf_cli_help_print(nrf_cli_t const *               p_cli,
             }
         }
     }
-    longest_string += cli_strlen(opt_sep) + tab_len;
+
+    longest_string += cli_strlen(opt_sep);
+    if (longest_string >= p_cli->p_ctx->vt100_ctx.cons.terminal_wid - tab_len)
+    {
+        longest_string = p_cli->p_ctx->vt100_ctx.cons.terminal_wid - tab_len;
+    }
 
     nrf_cli_fprintf(p_cli,
                     NRF_CLI_NORMAL,
-- 
2.28.0.windows.1

Date: Thu, 17 Jun 2021 17:06:01 -0400
Subject: [PATCH] Fix infinite loop error, ignore all alignment

---
 .../components/libraries/cli/nrf_cli.c        | 62 +++----------------

diff --git a/shared/nordic/nRF5x/sdk/v17.0.2/components/libraries/cli/nrf_cli.c b/shared/nordic/nRF5x/sdk/v17.0.2/components/libraries/cli/nrf_cli.c
index 49618815..d55ca471 100644
--- a/shared/nordic/nRF5x/sdk/v17.0.2/components/libraries/cli/nrf_cli.c
+++ b/shared/nordic/nRF5x/sdk/v17.0.2/components/libraries/cli/nrf_cli.c
@@ -2974,62 +2974,20 @@ static void format_offset_string_print(nrf_cli_t const * p_cli,
         size_t idx = 0;
         length = cli_strlen(p_str) - offset;
 
-        if (length <= p_cli->p_ctx->vt100_ctx.cons.terminal_wid - terminal_offset)
+        for (idx = 0; idx < length; idx++)
         {
-            for (idx = 0; idx < length; idx++)
+            if (*(p_str + offset + idx) == '\n')
             {
-                if (*(p_str + offset + idx) == '\n')
-                {
-                    transport_buffer_flush(p_cli);
-                    cli_write(p_cli, p_str + offset, idx, NULL);
-                    offset += idx + 1;
-                    cursor_next_line_move(p_cli);
-                    cursor_right_move(p_cli, terminal_offset);
-                    break;
-                }
-            }
-            /* String will fit in one line. */
-            nrf_fprintf(p_cli->p_fprintf_ctx, p_str + offset);
-            break;
-        }
-        else
-        {
-            /* String is longer than terminal line so text needs to divide in the way
-               to not divide words. */
-            length = p_cli->p_ctx->vt100_ctx.cons.terminal_wid - terminal_offset;
-
-            while (1)
-            {
-                /* Determining line break. */
-                if (isspace((int)(*(p_str + offset + idx))))
-                {
-                    length = idx;
-                    if (*(p_str + offset + idx) == '\n')
-                    {
-                        break;
-                    }
-                }
-                if ((idx + terminal_offset) >= p_cli->p_ctx->vt100_ctx.cons.terminal_wid)
-                {
-                    /* End of line reached. */
-                    break;
-                }
-                ++idx;
-            }
-
-            /* Writing one line, fprintf IO buffer must be flushed before calling cli_write. */
-            transport_buffer_flush(p_cli);
-            cli_write(p_cli, p_str + offset, length, NULL);
-            offset += length;
-
-            /* Calculating text offset to ensure that next line will not begin with a space. */
-            while (isspace((int)(*(p_str + offset))))
-            {
-                ++offset;
+                transport_buffer_flush(p_cli);
+                cli_write(p_cli, p_str + offset, idx, NULL);
+                offset += idx + 1;
+                cursor_next_line_move(p_cli);
+                cursor_right_move(p_cli, terminal_offset);
+                break;
             }
-            cursor_next_line_move(p_cli);
-            cursor_right_move(p_cli, terminal_offset);
         }
+        nrf_fprintf(p_cli->p_fprintf_ctx, p_str + offset);
+        break;
     }
     cursor_next_line_move(p_cli);
 }
-- 
2.28.0.windows.1