On Wed, Nov 12, 2025 at 09:17:10PM -0500, Matt Panaro <[email protected]> wrote:
> addresses https://savannah.gnu.org/bugs/index.php?66299 > the ls program itself has both a parameter and a recognized environment > variable, TIME_STYLE, that formats the output of the file's timestamp in the > listing. To achieve partial parity for the -ls output of find, the code is > refactored to honor the environment variable (a style parameter is not > implemented herein). > > diff --git a/lib/listfile.c b/lib/listfile.c > index e6ee70e3..78f89991 100644 > --- a/lib/listfile.c > +++ b/lib/listfile.c > @@ -341,17 +341,27 @@ list_file (const char *name, > /* Use strftime rather than ctime, because the former can produce > locale-dependent names for the month (%b). > > + As ls does, honor the TIME_STYLE environment variable, if present; > + otherwise: > Output the year if the file is fairly old or in the future. > POSIX says the cutoff is 6 months old; > approximate this by 6*30 days. > Allow a 1 hour slop factor for what is considered "the future", > to allow for NFS server/client clock disagreement. */ > - char const *fmt = > - ((current_time - 6 * 30 * 24 * 60 * 60 <= statp->st_mtime > - && statp->st_mtime <= current_time + 60 * 60) > - ? "%b %e %H:%M" > - : "%b %e %Y"); > - > + char *ts = getenv("TIME_STYLE"); > + char const *fmt; > + if(ts) { > + /* ls expects TIME_STYLE to start with a '+'; if it were re-used > + here, the '+' should be ignored when passing into strftime, > + below. */ > + if(*ts == '+') ts++; > + fmt = ts; > + } else { > + fmt = ((current_time - 6 * 30 * 24 * 60 * 60 <= statp->st_mtime > + && statp->st_mtime <= current_time + 60 * 60) > + ? "%b %e %H:%M" > + : "%b %e %Y"); > + } > while (!strftime (buf, bufsize, fmt, when_local)) > buf = alloca (bufsize *= 2); > > -- > 2.51.0 > I think the use of alloca a should be changed to malloc/realloc if taking in an environment variable. If the length of $TIME_STYLE were a gigabyte, it would end up with many calls to alloca with no cleanup of earlier calls until the function ends, and so it could crash the program when the stack filled up. Obviously, the initial buffer size of 256 bytes was fine/unnecessary before, and would still be fine for all real uses, but if you want to be extra careful, it might be wise to replace: buf = alloca (bufsize *= 2); with something like: { if (buf == init_bigbuf) buf = malloc(bufsize = strlen(fmt) * 4); else if ((p = realloc(buf, bufsize * 2)) buf = p, bufsize *= 2; } . . . if (buf != init_bigbuf) free(buf), buf = NULL; But that doesn't handle allocation failure, so it would need more work. Maybe this: { char *p; if (buf == init_bigbuf) buf = malloc (bufsize = strlen (fmt) * 2); else if ((p = realloc (buf, bufsize * 2)) buf = p, bufsize *= 2; else /* realloc failed */ free(buf), buf = NULL; if (!buf) /* malloc or realloc failed */ { buf = init_bigbuf; bufize = sizeof init_bigbuf; fmt = ((current_time - 6 * 30 * 24 * 60 * 60 <= statp->st_mtime && statp->st_mtime <= current_time + 60 * 60) ? "%b %e %H:%M" : "%b %e %Y"); } } . . . if (buf != init_bigbuf) free(buf), buf = NULL; i.e., When faced with allocation failure, ignore $TIME_STYLE and fall back to the original behaviour (which would never use more than 256 bytes, so any further allocation wouldn't be necessary. If that all seems too silly, at least consider ignoring $TIME_STYLE when the user is root. P.S. The new code in the patch uses tabs but the original code uses only spaces. P.P.S. getenv takes a variable amount of time, depending on where the variable appears in the list. It might be worthwhile to call getenv (or secure_getenv) once at the start, or once when first needed, and then remember its value for each use. It might not make much difference (less than 2-100ns each time), but it seems wasteful to repeat the same search through the list of environment variables for every file being listed. cheers, raf
