[Solved] st - simple terminal emulator - enabling command-line set color option

[jazzbiker]

Hi, Core people!

St is super-light terminal emulator from suckless.org team. Version 0.2 existed in TC 4.x repo. Recent version is 0.8.4 and for my taste in this version maintainer - Hiltjo Posthuma - achieved great improvement of the smoothness of screen refresh thanks to changing approach from fps to latency technique. I switched to st because TinyCore needs Tiny terminal emulators :-) Extension is less than 40k, binary executable is 50k. The only dependency is libXft. UTF-8 is ok.
     Now about contras. No screen buffer present, What You get is What You See - WYGIWYS! less is waiting for You! I don't expect this the problem, by the way inthe fresh kernels fbdev will be bufferless. The property, that made me sad were colors, defined during compile only.
     There are a bunch of patches for st on suckless.org, and among them there is the patch, which enables command line color setting option - http://st.suckless.org/patches/colors_at_launch/st-colors-at-launch-0.8.4.diff. BUT! this patch allows array out-of-bound writes, as You can see. I've corrected this patch, adding bound checks, and now enjoy st.
     I don't know, is anybody interested in this terminal emulator. And is it allowed to submit extensions with code patches, not intensively and widely tested?

     Those patches on suckless.org distracted me a bit. I've played with another patch, intended for reading X resources, and, You will be surprised, it have one out-of-bound write case too. These patches are supplied not by suckless.org team, but they post the patches probably without proper testing, so, please be careful.

     Finally, my question is - should I submit st.tcz with corrected patch?

     Patches are quite simple so I will post them.

1. Original from http://st.suckless.org/patches/colors_at_launch/st-colors-at-launch-0.8.4.diff

Code: [Select]

From 268d767b16d21f6f936bd5e3dfcd4f0187a8e979 Mon Sep 17 00:00:00 2001
From: MLquest8 <miskuzius@gmail.com>
Date: Sun, 12 Jul 2020 09:47:25 +0400
Subject: [PATCH] allow to alocate colors as launch options. Example: -C
 "#color@num"

---
 x.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/x.c b/x.c
index 210f184..a07e358 100644
--- a/x.c
+++ b/x.c
@@ -1980,6 +1980,8 @@ usage(void)
 int
 main(int argc, char *argv[])
 {
+    int i;
+    char *colval;
  xw.l = xw.t = 0;
  xw.isfixed = False;
  xsetcursor(cursorshape);
@@ -2024,6 +2026,11 @@ main(int argc, char *argv[])
  case 'v':
  die("%s " VERSION "\n", argv0);
  break;
+    case 'C':
+        colval = strtok(EARGF(usage()), "@");
+        i = atoi(strtok(NULL, "@"));
+ colorname[i] = colval;
+ break;
  default:
  usage();
  } ARGEND;
--
2.26.2

2. Corrected

Code: [Select]

--- x.c.orig
+++ x.c
@@ -1980,6 +1980,8 @@
 int
 main(int argc, char *argv[])
 {
+    int i;
+    char *colval, *coln;
        xw.l = xw.t = 0;
        xw.isfixed = False;
        xsetcursor(cursorshape);
@@ -2023,6 +2025,15 @@
                break;
        case 'v':
                die("%s " VERSION "\n", argv0);
+               break;
+       case 'C':
+               colval = strtok(EARGF(usage()), "@");
+               coln = strtok(NULL, "@");
+               if(!coln)
+                       break;
+               i = atoi(coln);
+               if(( i >= 0) && (i < (sizeof(colorname)/sizeof(char *))))
+                       colorname[i] = colval;
                break;
        default:
                usage();

Have a nice Core!

[Rich]

Hi jazzbiker

... And is it allowed to submit extensions with code patches, not intensively and widely tested? ...

Patches are not an issue. We run a patched kernel. Busybox has patches. Also, you can always include a note in the
comments section of the  .tcz.info  file to highlight that fact.

... Finally, my question is - should I submit st.tcz with corrected patch? ...

Yes. If a program is patched, the patches should be included. A small readme file indicating you patched the original
patch file would also be prudent.

Just one small observation:

Code: [Select]

+               i = atoi(coln);
+               if(( i >= 0) && (i < (sizeof(colorname)/sizeof(char *))))If  coln  points to a non-numeric string,  atoi  will return 0, the same as if had pointed to the string  "0". By testing for

Code: [Select]

i >= 0an error will default to the first entry in  colorname[].

If you want something that distinguishes between  zero  and an error, I think  strtol  would work.

[hiro]

there are reasons this is a patch and not in mainline st, people who actually know about st and do not compile it themselves will expect something like "st.tcz" to be the mainline version. but if you mark that diversion from mainline in the name (e.g. st-color.tcz) it all becomes predictable, should be no issue, even if multiple people upload their own favourite patched package of st.

[Rich]

Hi hiro

... but if you mark that diversion from mainline in the name (e.g. st-color.tcz) it all becomes predictable, ...

Excellent idea.

[jazzbiker]

Hi hiro

... but if you mark that diversion from mainline in the name (e.g. st-color.tcz) it all becomes predictable, ...

Excellent idea.

Double this!

strtol will behave in the same way, returning 0 in reply to junk input. How about

Code: [Select]

      if(( sscanf(coln,"%u",&i) ) && (i < (sizeof(colorname)/sizeof(char *))))
             colorname[i] = colval;
?

Thanks, Rich, hiro!

Definitely it is the strange patch to be published. Such plumbs are for temporary fast workarounds, not for use by someone outside Your housing. I don't understand the meaning of the action. Bound-check is not a bloat, I am quite sure. And the author and team definitely can  understand this patch behaviour. Does it means, that st source is to be inspected too? I'm in hesitation...

[Rich]

Hi jazzbiker

... strtol will behave in the same way, returning 0 in reply to junk input. ...

Yes, but  strtol  also includes an error catching mechanism. Try this:

Code: [Select]

----- Snip -----

int i;
char *colval, *coln, *endptr;

 ----- Snip -----

case 'C':
colval = strtok(EARGF(usage()), "@");
coln = strtok(NULL, "@");
errno=0;
endptr=NULL;
i=strtol(coln, &endptr, 10);
if((errno != 0) || (coln == endptr))
usage();
if(( i >= 0) && (i < (sizeof(colorname)/sizeof(char *))))
colorname[i] = colval;
break;
default:
usage();
See these for more information:
https://man7.org/linux/man-pages/man3/strtol.3.html
https://stackoverflow.com/questions/26080829/detecting-strtol-failure

[hiro]

btw, the people on suckless mailing list will be happy to discuss all this in detail, too. i was reading there for a long time.

[jazzbiker]

Hi, hiro!

If You are the subscriber and member of suckless mailing list, would You please report about this mis-patch there? If it will not take much of Your time, of course.

Hi, Rich!

I've made some testing, and the results are the next:

Code: [Select]

-rwxr-xr-x    1 tc       staff        59916 Nov  2 19:44 st
-rwxr-xr-x    1 tc       staff        48944 Nov  2 19:47 st
--- x.c.orig
+++ x.c
@@ -1980,6 +1980,9 @@
 int
 main(int argc, char *argv[])
 {
+       int i;
+       char *colval, *coln, *endptr;
+
        xw.l = xw.t = 0;
        xw.isfixed = False;
        xsetcursor(cursorshape);
@@ -2023,6 +2026,17 @@
                break;
        case 'v':
                die("%s " VERSION "\n", argv0);
+               break;
+       case 'C':
+               colval = strtok(EARGF(usage()), "@");
+               coln = strtok(NULL, "@");
+               errno=0;
+               endptr=NULL;
+               i=strtol(coln, &endptr, 10);
+               if((errno != 0) || (coln == endptr))
+                       usage();
+               if(( i >= 0) && (i < (sizeof(colorname)/sizeof(char *))))
+                       colorname[i] = colval;
                break;
        default:
                usage();
-rwxr-xr-x    1 tc       staff        61008 Nov  2 19:51 st
-rwxr-xr-x    1 tc       staff        50004 Nov  2 19:53 st
--- x.c.orig
+++ x.c
@@ -1980,6 +1980,9 @@
 int
 main(int argc, char *argv[])
 {
+       unsigned int i;
+       char *colval, *coln;
+
        xw.l = xw.t = 0;
        xw.isfixed = False;
        xsetcursor(cursorshape);
@@ -2023,6 +2026,13 @@
                break;
        case 'v':
                die("%s " VERSION "\n", argv0);
+               break;
+       case 'C':
+               colval = strtok(EARGF(usage()), "@");
+               coln = strtok(NULL, "@");
+               if((!coln) || (!sscanf(coln,"%u",&i)) || (i >= (sizeof(colorname)/sizeof(char *))))
+                       usage();
+               colorname[i] = colval;
                break;
        default:
                usage();
-rwxr-xr-x    1 tc       staff        61052 Nov  2 20:01 st
-rwxr-xr-x    1 tc       staff        50004 Nov  2 20:01 st


Pairs of st sizes are before and after sstriping.

Thanks for strtol description, I will gratefully take it into consideration, using such basic tools must be correct, thanks! Primarily I use libc.pdf from GNU, but mans on library functions appear to be easier in perception, I will use them too, thanks again.

[hiro]

hey, i'm not a subscriber any more.
i was pointing out that it should be easy for you to become one.
the community is lively and very happy to discuss patches.

[jazzbiker]

Ok, sorry for misunderstanding. Will do it soon. Thanks.

[jazzbiker]

Hi, Rich and hiro and all the Core people!

Finally after all this discussion I've submited unpatched, pure, fresh and clean st-0.8.4 :-) No bloat, no time losses , no cry:-) All this beautifulizing is harming and eye-damaging waste of time, red must be red and blue must be blue. Any program, which needs its own colors is free to use any, not harming the other stuff appearance, suckless guys know the true way. Thank You for Your advices, they were interesting and useful. Hope, not for me only.

Have a nice Core!

[jazzbiker]

Hi, Rich!

Nevertheless I stepped away the idea of cooking the patched st, seems that in this thread anyone, who have such needs, can find some usefull reciepts and precautions. Can You please mark the thread as SOLVED?

[Rich]

Hi jazzbiker

... Can You please mark the thread as SOLVED?

Done.