Re: cooledit printf(%n) fixes

2021-09-19 Thread Theo Buehler
On Sun, Sep 19, 2021 at 10:48:32PM +0200, Ingo Schwarze wrote:
> Hi Marc, hi Naddy,
> 
> Christian Weisgerber wrote on Sun, Sep 19, 2021 at 09:23:18PM +0200:
> 
> [...]
> > Let's focus on fixing bugs!
> > This includes the list of remaining ports with %n warnings:
> > 
> > editors/cooledit
> [...]
> 
> I think the patch appended below fixes those crashes in cooledit
> that are directly related to printf(%n).
> 
> OK?

ok tb

> There are many other places in the codebase where %n occurs, but i
> failed to find any others related to printf(3).

Same here. The other occurrences of %n are either fed into sscanf() or
are part of commands that sent through some kind of homegrown percent
substitution.



Re: cooledit printf(%n) fixes

2021-09-19 Thread Theo de Raadt
> Note that the port is very low quality in general.

This is the ports tree: you sound like a broken record.



cooledit printf(%n) fixes

2021-09-19 Thread Ingo Schwarze
Hi Marc, hi Naddy,

Christian Weisgerber wrote on Sun, Sep 19, 2021 at 09:23:18PM +0200:

[...]
> Let's focus on fixing bugs!
> This includes the list of remaining ports with %n warnings:
> 
> editors/cooledit
[...]

I think the patch appended below fixes those crashes in cooledit
that are directly related to printf(%n).

OK?

Note that the port is very low quality in general.  The code is full
of insecure idioms and the program is full of bugs that make even
minimal testing rather painful.  I don't think anybody can use this
port for serious work in its present state.

 * It sometimes has outrageous startup times, up to around 20 seconds
   on my notebook.
 * Putting it into the background with Ctrl-Z does not work.
 * Sometimes, it just freezes totally.
 * When assigning keys, it sometimes closes the window that is
   intended to capture the key combination when pressing a modifier
   key, which means that binding keys sometimes does not work.
 * Sometimes, it segfaults on exit (plain segfault, not SIGABRT).
 * And that is merely what i stumbled across without even trying
   to use it for real...

There are many other places in the codebase where %n occurs, but i
failed to find any others related to printf(3).  It has large numbers
of its own printf-like wrapper functions and clearly sometimes assembles
format strings dynamically in temporary buffers, but i did not find
indications that %n might be put into such dynamically-built format
strings.  Consequenly, i'm not completely sure that this patch resolves
all printf(%n) issues in the code base, but it does not seem unlikely
either.  Even if were not 100% complete, committing this would seem
better than doing nothing.

Yours,
  Ingo


Index: Makefile
===
RCS file: /cvs/ports/editors/cooledit/Makefile,v
retrieving revision 1.48
diff -u -p -r1.48 Makefile
--- Makefile23 Feb 2021 19:39:21 -  1.48
+++ Makefile19 Sep 2021 20:40:00 -
@@ -3,7 +3,7 @@
 COMMENT =  easy to use, graphical editor
 
 DISTNAME = cooledit-3.17.17
-REVISION = 4
+REVISION = 5
 
 CATEGORIES =   editors
 
Index: patches/patch-editor_editoptions_c
===
RCS file: patches/patch-editor_editoptions_c
diff -N patches/patch-editor_editoptions_c
--- /dev/null   1 Jan 1970 00:00:00 -
+++ patches/patch-editor_editoptions_c  19 Sep 2021 20:40:00 -
@@ -0,0 +1,28 @@
+$OpenBSD$
+
+Do not use sprintf(3) %n.
+
+Index: editor/editoptions.c
+--- editor/editoptions.c.orig
 editor/editoptions.c
+@@ -378,15 +378,15 @@ int save_user_defined_keys (struct key_list k_list[], 
+ 
+ for (i = 0; k_list[i].key_name[0]; i++) {
+   if (k_list[i].keycode2) {
+-  sprintf (p, "%s\t%x %x %x %x %x %x\n%n", k_list[i].key_name, 
k_list[i].state0, k_list[i].keycode0,
+-   k_list[i].state1, k_list[i].keycode1, k_list[i].state2, 
k_list[i].keycode2, );
++  n = sprintf (p, "%s\t%x %x %x %x %x %x\n", k_list[i].key_name, 
k_list[i].state0, k_list[i].keycode0,
++   k_list[i].state1, k_list[i].keycode1, k_list[i].state2, 
k_list[i].keycode2);
+   p += n;
+   } else if (k_list[i].keycode1) {
+-  sprintf (p, "%s\t%x %x %x %x\n%n", k_list[i].key_name, 
k_list[i].state0, k_list[i].keycode0,
+-   k_list[i].state1, k_list[i].keycode1, );
++  n = sprintf (p, "%s\t%x %x %x %x\n", k_list[i].key_name, 
k_list[i].state0, k_list[i].keycode0,
++   k_list[i].state1, k_list[i].keycode1);
+   p += n;
+   } else if (k_list[i].keycode0) {
+-  sprintf (p, "%s\t%x %x\n%n", k_list[i].key_name, k_list[i].state0, 
k_list[i].keycode0, );
++  n = sprintf (p, "%s\t%x %x\n", k_list[i].key_name, 
k_list[i].state0, k_list[i].keycode0);
+   p += n;
+   }
+ }
Index: patches/patch-editor_options_c
===
RCS file: patches/patch-editor_options_c
diff -N patches/patch-editor_options_c
--- /dev/null   1 Jan 1970 00:00:00 -
+++ patches/patch-editor_options_c  19 Sep 2021 20:40:00 -
@@ -0,0 +1,35 @@
+$OpenBSD$
+
+Do not use sprintf(3) %n.
+
+Index: editor/options.c
+--- editor/options.c.orig
 editor/options.c
+@@ -405,22 +405,22 @@ int save_setup (const char *file)
+ 
+ for (i = 0; string_options[i].name; i++) {
+   if (*string_options[i].value) {
+-  sprintf (p, "%s = %s\n%n", string_options[i].name, 
*string_options[i].value, );
++  r = sprintf (p, "%s = %s\n", string_options[i].name, 
*string_options[i].value);
+   p += r;
+   }
+ }
+ for (i = 0; integer_options[i].name; i++) {
+   if (integer_options[i].type == TYPE_HIDDEN_HEX_VALUE)
+-  sprintf (p, "%s = 0x%X\n%n", integer_options[i].name, 
*integer_options[i].value, );
++  r = sprintf (p, "%s = 0x%X\n", integer_options[i].name,