Him Grisha,
Thank you for your review.
I maccept 3 out of the 5 suggested chanhes:-
On Sun, May 03, 2026 at 06:29:34PM -0400, Grisha Levit wrote:
> examples/loadables/rev.c
> - include config.h to fix build (e.g., with unlocked-io) STYLE ONLY
> - getlen: avoid reading one byte before start of line GOOD CATCH
> - reverse_line: make sure BUF can fit the separator, if needed GOOD CATCH
> - rev_internal: do not require an argument for the -0 option GOOD CATCH
> - rev_internal: make sure to close any FD we opened, even if FD==0 BREAKS REV
> ---
> examples/loadables/rev.c | 29 +++++++++++++++++------------
> 1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/examples/loadables/rev.c b/examples/loadables/rev.c
> index b8d7d4d8..e15ca445 100644
> --- a/examples/loadables/rev.c
> +++ b/examples/loadables/rev.c
> @@ -43,6 +43,8 @@
shmbutil.h already includes config.h, so I never considered doing it. As a
matter of style, the maintainer may prefer to have rev.c include config.h since
rev.c does use macros from it (e.g. ARRAY_VARS).
As a separate issue, I think I now understand why array.h doesn't include
config.h: one should only include array.h if ARRAY_VARS is defined by config.h.
Will add to the patch I'm putting together.
>
> /* Headers */
>
> +#include "config.h"
> +
> #include <errno.h>
> #include <fcntl.h>
> #include <stdio.h>
> @@ -86,6 +88,7 @@ getlen(char *last_trlg_byte, int num_bytes_left)
Yes p can access off buffer start iff the line starts with invalid utf8.
>
> if ((*p-- & mask[1]) != mask[0])
> goto not_utf_8;
> + num_bytes_left--;
> n = 2;
> for (i = num_bytes_left >= 3 ? 3 : num_bytes_left; i > 0; i--, p--, n++)
> { /* 3 more bytes max */
> @@ -119,9 +122,9 @@ reverse_line(SHELL_VAR *v, arrayind_t *ind, char *line,
> size_t len,
Verified in gdb: the separator would overwrite the trailing NUL. Didn't notice
in original testing - likely fluked a NUL byte in location after separator.
> * with NULL value and putting an allocated buffer in it.
> */
> bind_array_element (v, (*ind)++, (char *)NULL, 0);
> - buf = xmalloc(len + 1); /* +1 for NUL */
> + buf = xmalloc(len + outputsep + 1); /* +1 for NUL */
> (((ARRAY *)v->value)->lastref)->value = buf;
> - buf[len] = '\0';
> + buf[len + outputsep] = '\0';
> } /* if (v) */
> #endif
>
> @@ -177,7 +180,7 @@ rev_internal(WORD_LIST *list)
Oops, must have forgotten to test that. I'm only learning getopt_long :)
> ind = 0;
>
> reset_internal_getopt();
> - while ((opt = internal_getopt(list, "0:a:h")) != -1)
> + while ((opt = internal_getopt(list, "0a:h")) != -1)
> switch (opt)
> {
> case '0':
> @@ -221,15 +224,17 @@ rev_internal(WORD_LIST *list)
The original code is correct. The effect of including this patch is that rev
with no arguments is a no-op. rev with no arguments *should* read lines from
standard input (fd 0).
> /* for each file */
>
> if (l == 0)
> - fd = 0;
> + fd = -1;
> else
> - SYSCALL(fd, open(l->word->word, O_RDONLY));
> - if (fd == -1)
> - {
> - file_error(l->word->word);
> - rval = EXECUTION_FAILURE;
> - goto next_file;
> - }
> + {
> + SYSCALL(fd, open(l->word->word, O_RDONLY));
> + if (fd == -1)
> + {
> + file_error(l->word->word);
> + rval = EXECUTION_FAILURE;
> + goto next_file;
> + }
> + }
>
> #ifndef __CYGWIN__
> unbuffered_read = (lseek(fd, 0L, SEEK_CUR) < 0) && (errno == ESPIPE);
> @@ -250,7 +255,7 @@ rev_internal(WORD_LIST *list)
> }
> reverse_line(v, &ind, line, n, outputsep, sep);
> } /* while ((n = zgetline(...) !=-1) */
> - if (fd != 0)
> + if (fd != -1)
> close(fd);
>
> next_file:
> --
> 2.54.0
>
Sorry Chet for the delay in replying: I have other commitments (including
babysitting my grandchildren, playing Bridge, lunch with old
colleagues/friends).
Grisha, did you by any chance use some AI tool to create your reply? If so, it's
quite good - it did find 3 real bugs: what tool was it? If not, I take my hat
off to you for finding them.
Cheers ... Duncan.
