Bug#1011609: bogl-bterm: [PATCH] Several improvements
Zhang Boyang, le ven. 10 juin 2022 17:00:16 +0900, a ecrit: > Here is a patch for another bug. Please refer to the commit message for > details. Applied, thanks for your patches! Samuel
Bug#1011609: bogl-bterm: [PATCH] Several improvements
Hello,
Here is a patch for another bug. Please refer to the commit message for
details.
Sorry to disturb you again.
Best Regards,
Zhang BoyangFrom d569b4fbc4233673032a1c5f7463890d5e2223dd Mon Sep 17 00:00:00 2001
From: Zhang Boyang
Date: Fri, 10 Jun 2022 15:16:33 +0800
Subject: [PATCH v6] Fix crash caused by invalid term->yorig
The SCR(x, y) may return negative value if term->yorig is negative or
too large, causing memory corruption and crash. There are two ways to
trigger this bug.
1) When scrolling up, term->yorig is decremented by one. If term->yorig
is zero, it can be -1 after the decrement, so SCR(0, 0) will become
negative, causing crash. Below is the test command:
bterm -f myfont.bgf -- python3 -c 'print("hello\033Mworld"); input("OK!")'
2) When scrolling down, term->yorig is incremented by one. There is no
check for integer overflow. When term->yorig is large enough, the
calculation in SCR(x, y) will overflow and it will return negative
value, causing crash. Below is the test command:
bterm -f myfont.bgf -- python3 -c 'print("\n"*22); input("OK!")'
This patch fixes the problem by limiting term->yorig to [0, term->ysize)
so there will be no negative value or overflow anymore.
---
bogl-term.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/bogl-term.c b/bogl-term.c
index 2d35d1d..f7fd735 100644
--- a/bogl-term.c
+++ b/bogl-term.c
@@ -188,7 +188,7 @@ cursor_down (struct bogl_term *term)
}
dirty_scroll(term);
-++term->yorig;
+term->yorig = (term->yorig + 1) % term->ysize;
for (i = 0; i < term->xsize; i++)
{
@@ -464,7 +464,7 @@ bogl_term_out (struct bogl_term *term, char *s, int n)
/* Move all other lines down. Fortunately, this is easy. */
dirty_backscroll(term);
-term->yorig--;
+term->yorig = (term->yorig - 1 + term->ysize) % term->ysize;
/* Clear the top line. */
for (i = SCR (0, 0); i < SCR (term->xsize, 0); i++)
--
2.30.2
Bug#1011609: bogl-bterm: [PATCH] Several improvements
Hello,
Applied, thanks!
Samuel
Zhang Boyang, le mer. 08 juin 2022 17:09:29 +0900, a ecrit:
> Hi,
>
> Changes in [PATCH v5]:
>
> Fix-incorrect-signal-handling.patch:
> Fix compiler warnings about implicit declaration of bogl_signal().
>
> Font-scaler.patch
> Use a lookup table to speed up font scaling.
>
> ===
>
> I'm sorry for sending a lot of emails to you, but I think this email is
> probably the last one of this email series. :-)
>
> Best Regards,
> Zhang Boyang
> From 7d399b7223bc194c0b61aab8c6bd252fd0d43ded Mon Sep 17 00:00:00 2001
> From: Zhang Boyang
> Date: Tue, 24 May 2022 12:58:01 +0800
> Subject: [PATCH v5 2/4] Fix incorrect signal handling
>
> There are problems in signal handlers. Signal handlers must not call any
> non-async-signal-safe functions, and they must save-restore errno if
> errno is modified inside signal handlers. This patch fixes these
> problems by deferring real tasks to main loop. This patch also
> introduces bogl_signal(), which wraps around sigaction() thus signal
> handlers can be installed in a portable way. Since signal related
> problems are fixed, the previously temporarily disabled font unmapping
> is now re-enabled.
> ---
> bogl.c | 50 +-
> bogl.h | 2 ++
> boglP.h | 2 ++
> bterm.c | 36 ++--
> 4 files changed, 55 insertions(+), 35 deletions(-)
>
> diff --git a/bogl.c b/bogl.c
> index 6b9996b..86bc1e0 100644
> --- a/bogl.c
> +++ b/bogl.c
> @@ -65,6 +65,7 @@
> /* Global variables. */
> int bogl_xres, bogl_yres, bogl_bpp; /* bogl.h */
> int bogl_refresh;
> +volatile int vt_switch_pending = 0;
> volatile char *bogl_frame; /* boglP.h */
> int bogl_drawing;
> int bogl_line_len;
> @@ -120,7 +121,7 @@ static void draw_disable (void);
> static void kbd_init (void);
> static void kbd_done (void);
>
> -static void vt_switch (int);
> +static void sigusr2 (int);
>
> static struct fb_fix_screeninfo fb_fix;
> /* Initialize BOGL. */
> @@ -181,7 +182,7 @@ bogl_init (void)
>mode.relsig = SIGUSR2;
>mode.acqsig = SIGUSR2;
>
> - signal (SIGUSR2, vt_switch);
> + bogl_signal (SIGUSR2, sigusr2);
>
>if (-1 == ioctl (tty, VT_SETMODE, ))
> return bogl_fail ("can't set VT mode: %s", strerror (errno));
> @@ -295,7 +296,7 @@ bogl_done (void)
>
>munmap ((void *) bogl_frame, fb_fix.smem_len);
>
> - signal (SIGUSR2, SIG_DFL);
> + bogl_signal (SIGUSR2, SIG_DFL);
>
>ioctl (tty, KDSETMODE, KD_TEXT);
>
> @@ -583,32 +584,18 @@ draw_disable (void)
> /* Signal handler called whenever the kernel wants to switch to or
> from our tty. */
> static void
> -vt_switch (int sig unused)
> +sigusr2 (int sig unused)
> {
> - signal (SIGUSR2, vt_switch);
> + vt_switch_pending = 1;
> +}
> +void
> +vt_switch (void)
> +{
> + vt_switch_pending = 0;
>
> - /* If a BOGL drawing function is in progress then we cannot mode
> - switch right now because the drawing function would continue to
> - scribble on the screen after the switch. So disable further
> - drawing and schedule an alarm to try again in .1 second. */
>if (bogl_drawing)
> {
> - draw_disable ();
> -
> - signal (SIGALRM, vt_switch);
> -
> - {
> - struct itimerval duration;
> -
> - duration.it_interval.tv_sec = 0;
> - duration.it_interval.tv_usec = 0;
> - duration.it_value.tv_sec = 0;
> - duration.it_value.tv_usec = 10;
> - if (-1 == setitimer (ITIMER_REAL, , NULL))
> - bogl_fail ("can't set timer: %s", strerror (errno));
> - }
> -
> - return;
> + abort();
> }
>
>if (visible)
> @@ -666,3 +653,16 @@ bogl_cloexec(int fd)
>
>return 0;
> }
> +
> +/* Install signal handler in a portable way (i.e. sigaction wrapper) */
> +int
> +bogl_signal(int signum, void (*handler) (int))
> +{
> + struct sigaction sa;
> +
> + sa.sa_handler = handler;
> + sigemptyset (_mask);
> + sa.sa_flags = SA_RESTART;
> +
> + return sigaction (signum, , NULL);
> +}
> diff --git a/bogl.h b/bogl.h
> index 3b2cb83..4d99788 100644
> --- a/bogl.h
> +++ b/bogl.h
> @@ -69,6 +69,8 @@ extern int bogl_xres, bogl_yres, bogl_ncols;
>
> /* 1=Must refresh screen due to tty change. */
> extern int bogl_refresh;
> +extern volatile int vt_switch_pending;
> +extern void vt_switch (void);
>
> /* Generic routines. */
> int bogl_init (void);
> diff --git a/boglP.h b/boglP.h
> index be99979..9a2b4aa 100644
> --- a/boglP.h
> +++ b/boglP.h
> @@ -27,4 +27,6 @@ int bogl_fail (const char *, ...);
>
> int bogl_cloexec (int fd);
>
> +int bogl_signal (int signum, void (*handler) (int));
> +
> #endif /* boglP_h */
> diff --git a/bterm.c b/bterm.c
> index dfae8b9..e86a748 100644
> --- a/bterm.c
> +++ b/bterm.c
> @@ -39,6 +39,7 @@
> #include
>
> #include "bogl.h"
> +#include "boglP.h"
> #include "bogl-bgf.h"
> #include "bogl-term.h"
>
> @@ -160,7 +161,6 @@ void sigchld(int sig)
>
Bug#1011609: bogl-bterm: [PATCH] Several improvements
Hi,
Changes in [PATCH v5]:
Fix-incorrect-signal-handling.patch:
Fix compiler warnings about implicit declaration of bogl_signal().
Font-scaler.patch
Use a lookup table to speed up font scaling.
===
I'm sorry for sending a lot of emails to you, but I think this email is
probably the last one of this email series. :-)
Best Regards,
Zhang BoyangFrom 7d399b7223bc194c0b61aab8c6bd252fd0d43ded Mon Sep 17 00:00:00 2001
From: Zhang Boyang
Date: Tue, 24 May 2022 12:58:01 +0800
Subject: [PATCH v5 2/4] Fix incorrect signal handling
There are problems in signal handlers. Signal handlers must not call any
non-async-signal-safe functions, and they must save-restore errno if
errno is modified inside signal handlers. This patch fixes these
problems by deferring real tasks to main loop. This patch also
introduces bogl_signal(), which wraps around sigaction() thus signal
handlers can be installed in a portable way. Since signal related
problems are fixed, the previously temporarily disabled font unmapping
is now re-enabled.
---
bogl.c | 50 +-
bogl.h | 2 ++
boglP.h | 2 ++
bterm.c | 36 ++--
4 files changed, 55 insertions(+), 35 deletions(-)
diff --git a/bogl.c b/bogl.c
index 6b9996b..86bc1e0 100644
--- a/bogl.c
+++ b/bogl.c
@@ -65,6 +65,7 @@
/* Global variables. */
int bogl_xres, bogl_yres, bogl_bpp; /* bogl.h */
int bogl_refresh;
+volatile int vt_switch_pending = 0;
volatile char *bogl_frame; /* boglP.h */
int bogl_drawing;
int bogl_line_len;
@@ -120,7 +121,7 @@ static void draw_disable (void);
static void kbd_init (void);
static void kbd_done (void);
-static void vt_switch (int);
+static void sigusr2 (int);
static struct fb_fix_screeninfo fb_fix;
/* Initialize BOGL. */
@@ -181,7 +182,7 @@ bogl_init (void)
mode.relsig = SIGUSR2;
mode.acqsig = SIGUSR2;
- signal (SIGUSR2, vt_switch);
+ bogl_signal (SIGUSR2, sigusr2);
if (-1 == ioctl (tty, VT_SETMODE, ))
return bogl_fail ("can't set VT mode: %s", strerror (errno));
@@ -295,7 +296,7 @@ bogl_done (void)
munmap ((void *) bogl_frame, fb_fix.smem_len);
- signal (SIGUSR2, SIG_DFL);
+ bogl_signal (SIGUSR2, SIG_DFL);
ioctl (tty, KDSETMODE, KD_TEXT);
@@ -583,32 +584,18 @@ draw_disable (void)
/* Signal handler called whenever the kernel wants to switch to or
from our tty. */
static void
-vt_switch (int sig unused)
+sigusr2 (int sig unused)
{
- signal (SIGUSR2, vt_switch);
+ vt_switch_pending = 1;
+}
+void
+vt_switch (void)
+{
+ vt_switch_pending = 0;
- /* If a BOGL drawing function is in progress then we cannot mode
- switch right now because the drawing function would continue to
- scribble on the screen after the switch. So disable further
- drawing and schedule an alarm to try again in .1 second. */
if (bogl_drawing)
{
- draw_disable ();
-
- signal (SIGALRM, vt_switch);
-
- {
- struct itimerval duration;
-
- duration.it_interval.tv_sec = 0;
- duration.it_interval.tv_usec = 0;
- duration.it_value.tv_sec = 0;
- duration.it_value.tv_usec = 10;
- if (-1 == setitimer (ITIMER_REAL, , NULL))
- bogl_fail ("can't set timer: %s", strerror (errno));
- }
-
- return;
+ abort();
}
if (visible)
@@ -666,3 +653,16 @@ bogl_cloexec(int fd)
return 0;
}
+
+/* Install signal handler in a portable way (i.e. sigaction wrapper) */
+int
+bogl_signal(int signum, void (*handler) (int))
+{
+ struct sigaction sa;
+
+ sa.sa_handler = handler;
+ sigemptyset (_mask);
+ sa.sa_flags = SA_RESTART;
+
+ return sigaction (signum, , NULL);
+}
diff --git a/bogl.h b/bogl.h
index 3b2cb83..4d99788 100644
--- a/bogl.h
+++ b/bogl.h
@@ -69,6 +69,8 @@ extern int bogl_xres, bogl_yres, bogl_ncols;
/* 1=Must refresh screen due to tty change. */
extern int bogl_refresh;
+extern volatile int vt_switch_pending;
+extern void vt_switch (void);
/* Generic routines. */
int bogl_init (void);
diff --git a/boglP.h b/boglP.h
index be99979..9a2b4aa 100644
--- a/boglP.h
+++ b/boglP.h
@@ -27,4 +27,6 @@ int bogl_fail (const char *, ...);
int bogl_cloexec (int fd);
+int bogl_signal (int signum, void (*handler) (int));
+
#endif /* boglP_h */
diff --git a/bterm.c b/bterm.c
index dfae8b9..e86a748 100644
--- a/bterm.c
+++ b/bterm.c
@@ -39,6 +39,7 @@
#include
#include "bogl.h"
+#include "boglP.h"
#include "bogl-bgf.h"
#include "bogl-term.h"
@@ -160,7 +161,6 @@ void sigchld(int sig)
quit_status = status;
quit = 1;
}
- signal(SIGCHLD, sigchld);
errno = errsv;
}
@@ -176,7 +176,7 @@ void spawn_shell(int ptyfd, int ttyfd, char * const *command_args)
child_pid = fork();
if (child_pid) {
/* Change ownership and permissions of ttyfd device! */
-signal(SIGCHLD, sigchld);
+bogl_signal(SIGCHLD, sigchld);
return;
}
@@ -212,10 +212,16 @@ void set_window_size(int ttyfd, int x, int y)
static char *font_name;
static struct
Bug#1011609: bogl-bterm: [PATCH] Several improvements
Control: tags -1 + pending Zhang Boyang, le ven. 03 juin 2022 17:04:26 +0900, a ecrit: > Changes in [PATCH V4]: > > Tiny improvement: detect 8K+ monitors and assign 8x scale factor for them. Applied, thanks! Samuel
Bug#1011609: bogl-bterm: [PATCH] Several improvements
Hi,
Changes in [PATCH V3]:
Please refer to https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1011609#30
Changes in [PATCH V4]:
Tiny improvement: detect 8K+ monitors and assign 8x scale factor for them.
Best Regards,
Zhang BoyangFrom 540b82ac07c5a582da3d6e6ad1cdc2fd83c36b2e Mon Sep 17 00:00:00 2001
From: Zhang Boyang
Date: Mon, 23 May 2022 14:10:52 +0800
Subject: [PATCH v4 1/4] Better font reload handling
Previous font reload code will leak a mmap on each reload. This patch
adds the ability to munmap old font after reload. However, this also
introduces a bug, if font reload is triggered while drawing in progress,
after signal handler returns, the drawing code will continue to use old
font which has been freed, causing crash. So the munmap is temporarily
disabled until we fix async-signal-safety problems completely.
---
bogl-bgf.c | 63 +++---
bogl-bgf.h | 1 +
bterm.c| 9
3 files changed, 55 insertions(+), 18 deletions(-)
diff --git a/bogl-bgf.c b/bogl-bgf.c
index 1032028..beed3c8 100644
--- a/bogl-bgf.c
+++ b/bogl-bgf.c
@@ -5,38 +5,55 @@
#include
#include
#include
+#include
#include "bogl.h"
#include "boglP.h"
#include "bogl-font.h"
+#include "bogl-bgf.h"
+
+struct bogl_bgf {
+ void *f;/* mmap area */
+ off_t size; /* size of mmap area */
+ struct bogl_font font; /* font descriptor */
+};
+#define bgf_of(font) \
+ ((struct bogl_bgf *)((char *)(font) - offsetof(struct bogl_bgf, font)))
struct bogl_font *bogl_mmap_font(char *file)
{
- int fd;
+ struct bogl_bgf *bgf = NULL;
+ struct bogl_font *font = NULL;
+ int fd = -1;
struct stat buf;
- void *f;
- struct bogl_font *font;
+ void *f = MAP_FAILED;
+
+ bgf = (struct bogl_bgf *)malloc(sizeof(struct bogl_bgf));
+ if (!bgf)
+goto fail;
+ font = >font;
fd = open(file, O_RDONLY);
if (fd == -1)
-return 0;
+goto fail;
if (bogl_cloexec(fd) < 0)
-return 0;
+goto fail;
if (fstat(fd, ))
-return 0;
+goto fail;
+ bgf->size = buf.st_size;
+
+ if (buf.st_size < 4)
+goto fail;
f = mmap(0, buf.st_size, PROT_READ, MAP_SHARED, fd, 0);
- if (f == (void *)-1)
-return 0;
+ if (f == MAP_FAILED)
+goto fail;
+ bgf->f = f;
if (memcmp("BGF1", f, 4))
-return 0;
-
- font = (struct bogl_font *)malloc(sizeof(struct bogl_font));
- if (!font)
-return 0;
+goto fail;
memcpy(font, f + 4, sizeof(*font));
font->name = ((void *)font->name - (void *)0) + f;
@@ -44,5 +61,25 @@ struct bogl_font *bogl_mmap_font(char *file)
font->index = ((void *)font->index - (void *)0) + f;
font->content = ((void *)font->content - (void *)0) + f;
+done:
+ if (fd != -1)
+close(fd);
return font;
+
+fail:
+ if (bgf) {
+free(bgf);
+bgf = NULL;
+font = NULL;
+ }
+ if (f != MAP_FAILED)
+munmap(f, buf.st_size);
+ goto done;
+}
+
+void bogl_munmap_font(struct bogl_font *font)
+{
+ struct bogl_bgf *bgf = bgf_of(font);
+ munmap(bgf->f, bgf->size);
+ free(bgf);
}
diff --git a/bogl-bgf.h b/bogl-bgf.h
index e9fb994..f14a260 100644
--- a/bogl-bgf.h
+++ b/bogl-bgf.h
@@ -1,2 +1,3 @@
struct bogl_font *bogl_mmap_font(char *file);
+void bogl_munmap_font(struct bogl_font *font);
diff --git a/bterm.c b/bterm.c
index 605644f..dfae8b9 100644
--- a/bterm.c
+++ b/bterm.c
@@ -224,11 +224,10 @@ void reload_font(int sig)
return;
}
- /* This leaks a mmap. Since the font reloading feature is only expected
- to be used once per session (for instance, in debian-installer, after
- the font is replaced with a larger version containing more characters),
- we don't worry about the leak. */
- free(term->font);
+ /* BUG: Unmapping old font in this signal handler may cause crash if
+ drawing is in progress, so disable this temporarily until we fix
+ async-signal-safety problems completely. */
+ //bogl_munmap_font(term->font);
term->font = font;
term->xstep = bogl_font_glyph(term->font, ' ', 0);
--
2.30.2
From b222a9a15b0a2706927ecde8ce163243b076bbcb Mon Sep 17 00:00:00 2001
From: Zhang Boyang
Date: Tue, 24 May 2022 12:58:01 +0800
Subject: [PATCH v4 2/4] Fix incorrect signal handling
There are problems in signal handlers. Signal handlers must not call any
non-async-signal-safe functions, and they must save-restore errno if
errno is modified inside signal handlers. This patch fixes these
problems by deferring real tasks to main loop. This patch also
introduces bogl_signal(), which wraps around sigaction() thus signal
handlers can be installed in a portable way. Since signal related
problems are fixed, the previously temporarily disabled font unmapping
is now re-enabled.
---
bogl.c | 50 +-
bogl.h | 2 ++
boglP.h | 2 ++
bterm.c | 35 +--
4 files changed, 54 insertions(+), 35 deletions(-)
diff --git a/bogl.c b/bogl.c
index 6b9996b..86bc1e0 100644
--- a/bogl.c
+++ b/bogl.c
@@
Bug#1011609: bogl-bterm: [PATCH] Several improvements
Hi,
I forgot to attach the patches in the last mail. Here are the updated
patches.
Best Regards,
Zhang BoyangFrom 540b82ac07c5a582da3d6e6ad1cdc2fd83c36b2e Mon Sep 17 00:00:00 2001
From: Zhang Boyang
Date: Mon, 23 May 2022 14:10:52 +0800
Subject: [PATCH v3 1/4] Better font reload handling
Previous font reload code will leak a mmap on each reload. This patch
adds the ability to munmap old font after reload. However, this also
introduces a bug, if font reload is triggered while drawing in progress,
after signal handler returns, the drawing code will continue to use old
font which has been freed, causing crash. So the munmap is temporarily
disabled until we fix async-signal-safety problems completely.
---
bogl-bgf.c | 63 +++---
bogl-bgf.h | 1 +
bterm.c| 9
3 files changed, 55 insertions(+), 18 deletions(-)
diff --git a/bogl-bgf.c b/bogl-bgf.c
index 1032028..beed3c8 100644
--- a/bogl-bgf.c
+++ b/bogl-bgf.c
@@ -5,38 +5,55 @@
#include
#include
#include
+#include
#include "bogl.h"
#include "boglP.h"
#include "bogl-font.h"
+#include "bogl-bgf.h"
+
+struct bogl_bgf {
+ void *f;/* mmap area */
+ off_t size; /* size of mmap area */
+ struct bogl_font font; /* font descriptor */
+};
+#define bgf_of(font) \
+ ((struct bogl_bgf *)((char *)(font) - offsetof(struct bogl_bgf, font)))
struct bogl_font *bogl_mmap_font(char *file)
{
- int fd;
+ struct bogl_bgf *bgf = NULL;
+ struct bogl_font *font = NULL;
+ int fd = -1;
struct stat buf;
- void *f;
- struct bogl_font *font;
+ void *f = MAP_FAILED;
+
+ bgf = (struct bogl_bgf *)malloc(sizeof(struct bogl_bgf));
+ if (!bgf)
+goto fail;
+ font = >font;
fd = open(file, O_RDONLY);
if (fd == -1)
-return 0;
+goto fail;
if (bogl_cloexec(fd) < 0)
-return 0;
+goto fail;
if (fstat(fd, ))
-return 0;
+goto fail;
+ bgf->size = buf.st_size;
+
+ if (buf.st_size < 4)
+goto fail;
f = mmap(0, buf.st_size, PROT_READ, MAP_SHARED, fd, 0);
- if (f == (void *)-1)
-return 0;
+ if (f == MAP_FAILED)
+goto fail;
+ bgf->f = f;
if (memcmp("BGF1", f, 4))
-return 0;
-
- font = (struct bogl_font *)malloc(sizeof(struct bogl_font));
- if (!font)
-return 0;
+goto fail;
memcpy(font, f + 4, sizeof(*font));
font->name = ((void *)font->name - (void *)0) + f;
@@ -44,5 +61,25 @@ struct bogl_font *bogl_mmap_font(char *file)
font->index = ((void *)font->index - (void *)0) + f;
font->content = ((void *)font->content - (void *)0) + f;
+done:
+ if (fd != -1)
+close(fd);
return font;
+
+fail:
+ if (bgf) {
+free(bgf);
+bgf = NULL;
+font = NULL;
+ }
+ if (f != MAP_FAILED)
+munmap(f, buf.st_size);
+ goto done;
+}
+
+void bogl_munmap_font(struct bogl_font *font)
+{
+ struct bogl_bgf *bgf = bgf_of(font);
+ munmap(bgf->f, bgf->size);
+ free(bgf);
}
diff --git a/bogl-bgf.h b/bogl-bgf.h
index e9fb994..f14a260 100644
--- a/bogl-bgf.h
+++ b/bogl-bgf.h
@@ -1,2 +1,3 @@
struct bogl_font *bogl_mmap_font(char *file);
+void bogl_munmap_font(struct bogl_font *font);
diff --git a/bterm.c b/bterm.c
index 605644f..dfae8b9 100644
--- a/bterm.c
+++ b/bterm.c
@@ -224,11 +224,10 @@ void reload_font(int sig)
return;
}
- /* This leaks a mmap. Since the font reloading feature is only expected
- to be used once per session (for instance, in debian-installer, after
- the font is replaced with a larger version containing more characters),
- we don't worry about the leak. */
- free(term->font);
+ /* BUG: Unmapping old font in this signal handler may cause crash if
+ drawing is in progress, so disable this temporarily until we fix
+ async-signal-safety problems completely. */
+ //bogl_munmap_font(term->font);
term->font = font;
term->xstep = bogl_font_glyph(term->font, ' ', 0);
--
2.30.2
From b222a9a15b0a2706927ecde8ce163243b076bbcb Mon Sep 17 00:00:00 2001
From: Zhang Boyang
Date: Tue, 24 May 2022 12:58:01 +0800
Subject: [PATCH v3 2/4] Fix incorrect signal handling
There are problems in signal handlers. Signal handlers must not call any
non-async-signal-safe functions, and they must save-restore errno if
errno is modified inside signal handlers. This patch fixes these
problems by deferring real tasks to main loop. This patch also
introduces bogl_signal(), which wraps around sigaction() thus signal
handlers can be installed in a portable way. Since signal related
problems are fixed, the previously temporarily disabled font unmapping
is now re-enabled.
---
bogl.c | 50 +-
bogl.h | 2 ++
boglP.h | 2 ++
bterm.c | 35 +--
4 files changed, 54 insertions(+), 35 deletions(-)
diff --git a/bogl.c b/bogl.c
index 6b9996b..86bc1e0 100644
--- a/bogl.c
+++ b/bogl.c
@@ -65,6 +65,7 @@
/* Global variables. */
int bogl_xres, bogl_yres, bogl_bpp; /* bogl.h */
int bogl_refresh;
+volatile int
Bug#1011609: bogl-bterm: [PATCH] Several improvements
Hi, Thanks for reviewing. I did some changes on my patch. I also closed the PR on salsa because we are using debian bug tracker instead of gitlab pull requests. On 2022/5/30 03:49, Samuel Thibault wrote: Then is it not possible to exchange the patch order? So that whatever the number of patches being applied, things work completely. The two patch is tightly related. I guess why the original code leak a font mmap intentionally is just to avoid the font-unmap-during-drawing-will-cause-crash problem. The async-signal-safefy patch will defer font reloading to main loop to avoid this problem (and fix other problems like calling malloc() in signal handlers which is very dangerous). I updated my patch. The first patch (Better font reload handling) will comment out the call to bogl_munmap_font(), avoiding crash but still have a leak, just like the original code. So if someone only applies the first patch, it will work as before (not crash but still leak memory). The second patch (Fix incorrect signal handling) will re-enable the call previously commented out in first patch. So if someone want to apply the second patch, he/she must apply the first patch first, make dependency more clear. So, things are always working if (a) only first patch is applied, or (b) both first and second patch are applied, or (c) non of these two patch are applied, and (d) it is not possible to only apply second patch because the patch command will complain about conflicts. While at it, please use MAP_FAILED rather than the hardcoded (void*)-1 Fixed. :-) This looks unrelated and bogus? This is because I want to maintain a constant look of signal handlers. The reason behind it is there is no need to call signal() to install handler again because glibc's signal() now provide BSD semantics per man page of signal(). I updated my patch and use a sigaction wrapper to address this explicitly, and this will make the code more portable. Do you have a reference that documents this? Unfortunately there is no document about this, mainly because this is a workaround for buggy drivers. I updated comments in code to address this more explicitly. MAP_FAILED here as well. Fixed. :-) Please make these an inline function rather than copy/pasting them from bogl_mmap_font. Fixed. :-) Best Regards, Zhang Boyang
Bug#1011609: bogl-bterm: [PATCH] Several improvements
Hello,
Zhang Boyang, le dim. 29 mai 2022 14:23:19 +0800, a ecrit:
> Subject: [PATCH v2 1/8] Better quit handling
That looks good, applied!
> From 302952c64952197d694039358df407ec3ffa418a Mon Sep 17 00:00:00 2001
> From: Zhang Boyang
> Date: Mon, 23 May 2022 14:10:52 +0800
> Subject: [PATCH v2 2/8] Better font reload handling
>
> Previous font reload code will leak a mmap on each reload. This patch
> adds the ability to munmap old font after reload. However, this also
> introduces a bug, if font reload is triggered while drawing in progress,
> after signal handler returns, the drawing code will continue to use old
> font which has been freed, causing crash. This will be fixed in next
> patch.
Then is it not possible to exchange the patch order? So that whatever
the number of patches being applied, things work completely.
> - void *f;
> - struct bogl_font *font;
> + void *f = (void *)-1;
[...]
>f = mmap(0, buf.st_size, PROT_READ, MAP_SHARED, fd, 0);
>if (f == (void *)-1)
While at it, please use MAP_FAILED rather than the hardcoded (void*)-1
Otherwise it looks good.
> From 4b2e0651edf19cab4b162c686c21e81682c854a0 Mon Sep 17 00:00:00 2001
> From: Zhang Boyang
> Date: Tue, 24 May 2022 12:58:01 +0800
> Subject: [PATCH v2 3/8] Fix incorrect signal handling
>
> There are problems in signal handlers. Signal handlers must not call any
> non-async-signal-safe functions, and they must save-restore errno if
> errno is modified inside signal handlers. This patch fixes these
> problems by deferring real tasks to main loop. This also fixes crashes
> caused by font reloading which was mentioned in previous patch.
> diff --git a/bterm.c b/bterm.c
> index 28ccb53..b0a1189 100644
> --- a/bterm.c
> +++ b/bterm.c
> @@ -160,7 +160,6 @@ void sigchld(int sig)
>quit_status = status;
> quit = 1;
>}
> - signal(SIGCHLD, sigchld);
>errno = errsv;
> }
>
This looks unrelated and bogus?
Otherwise it looks good.
> From e7fe989fbbda4acfd9971604b7ffa84899cb343a Mon Sep 17 00:00:00 2001
> From: Zhang Boyang
> Date: Mon, 23 May 2022 22:07:37 +0800
> Subject: [PATCH v2 4/8] Use ioctl(FBIOPAN_DISPLAY) to update screen after
> drawing
>
> Some framebuffer driver like i915 need explicit notify after drawing,
> otherwise modifications in graphics buffer will not be reflected on
> screen, so use FBIOPAN_DISPLAY to do this notify.
Do you have a reference that documents this?
Otherwise it looks good.
> From 04d88a6ff7114750c67cdb58e07122bb430763a9 Mon Sep 17 00:00:00 2001
> From: Zhang Boyang
> Date: Tue, 24 May 2022 23:49:31 +0800
> Subject: [PATCH v2 5/8] Font scaler
[...]
> +bgf_scale_inline(struct bogl_bgf *bgf, int scale)
> +{
> + void *new_f = (void *)-1;
> + off_t new_size;
> + struct bogl_font new_font;
> +
> + /* old_size*pow(scale,2) should enough and have some waste here */
> + new_size = bgf->size * scale * scale;
> +
> + /* Allocate new memory */
> + new_f = mmap(0, new_size, PROT_READ | PROT_WRITE, MAP_PRIVATE |
> MAP_ANONYMOUS, -1, 0);
> + if (new_f == (void *)-1) {
MAP_FAILED here as well.
> +/* If memory allocation failed, skip scaling silently */
> +goto fail;
> + }
> +
> + /* Copy old font data to new memory */
> + memcpy(new_f, bgf->f, bgf->size);
> +
> + /* Set font metadata */
> + struct bogl_font *font = _font;
> + memcpy(font, new_f + 4, sizeof(*font));
> + font->name = ((void *)font->name - (void *)0) + new_f;
> + font->offset = ((void *)font->offset - (void *)0) + new_f;
> + font->index = ((void *)font->index - (void *)0) + new_f;
> + font->content = ((void *)font->content - (void *)0) + new_f;
Please make these an inline function rather than copy/pasting them from
bogl_mmap_font.
Otherwise it looks good.
> Subject: [PATCH v2 6/8] Fix character occasionally disappears from right edge
> of screen
That looks good, applied!
> Subject: [PATCH v2 7/8] Fix out-of-bound read in tcfb's cmap_lookup()
That looks good, applied!
> Subject: [PATCH v2 8/8] Fix rightmost pixel of each line not cleared by
> bogl_vga16_text()
That looks good, applied!
And thanks!
Samuel
Bug#1011609: bogl-bterm: [PATCH] Several improvements
This is the v2 patch series. There are some reordering, squashing, and
minor changes compared to previously proposed patch series.
An all-in-one patch for quilt is also attached, which can be directly
applied to the git repo. (Same as the merge request it self)From c8de527c0c0ff5b8e2a3c10c1d26e5674ffccf50 Mon Sep 17 00:00:00 2001
From: Zhang Boyang
Date: Tue, 24 May 2022 01:27:10 +0800
Subject: [PATCH v2 1/8] Better quit handling
Previous SIGCHLD handler is not async-signal-safe because it calls
exit(), and it also doesn't save-restore errno. Using _exit() will be
async-signal-safe safe but will result in unclean quit. In the end, this
patch will set a flag variable in signal handler, then defer real
cleanup and quitting to main loop.
---
bterm.c | 21 +
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/bterm.c b/bterm.c
index d7b574f..605644f 100644
--- a/bterm.c
+++ b/bterm.c
@@ -64,7 +64,7 @@ static const unsigned char palette[16][3] =
static int child_pid = 0;
static struct termios ttysave;
-static int quit = 0;
+static volatile int quit = 0, quit_status = 0;
/* Out of memory. Give up. */
static void out_of_memory (void)
@@ -144,24 +144,29 @@ void send_hangup(void)
void sigchld(int sig)
{
+ int errsv = errno;
int status;
if (waitpid(child_pid, , WNOHANG) > 0) {
child_pid = 0;
/* Reset ownership and permissions of ttyfd device? */
tcsetattr(0, TCSAFLUSH, );
if (WIFEXITED (status))
- exit(WEXITSTATUS (status));
-if (WIFSIGNALED (status))
- exit(128 + WTERMSIG (status));
-if (WIFSTOPPED (status))
- exit(128 + WSTOPSIG (status));
-exit(status);
+ quit_status = WEXITSTATUS (status);
+else if (WIFSIGNALED (status))
+ quit_status = 128 + WTERMSIG (status);
+else if (WIFSTOPPED (status))
+ quit_status = 128 + WSTOPSIG (status);
+else
+ quit_status = status;
+quit = 1;
}
signal(SIGCHLD, sigchld);
+ errno = errsv;
}
void sigterm(int sig)
{
+ quit_status = 128 + SIGTERM;
quit = 1;
}
@@ -420,5 +425,5 @@ int main(int argc, char *argv[])
}
}
- return 0;
+ exit(quit_status);
}
--
2.30.2
From 302952c64952197d694039358df407ec3ffa418a Mon Sep 17 00:00:00 2001
From: Zhang Boyang
Date: Mon, 23 May 2022 14:10:52 +0800
Subject: [PATCH v2 2/8] Better font reload handling
Previous font reload code will leak a mmap on each reload. This patch
adds the ability to munmap old font after reload. However, this also
introduces a bug, if font reload is triggered while drawing in progress,
after signal handler returns, the drawing code will continue to use old
font which has been freed, causing crash. This will be fixed in next
patch.
---
bogl-bgf.c | 61 +++---
bogl-bgf.h | 1 +
bterm.c| 6 +-
3 files changed, 51 insertions(+), 17 deletions(-)
diff --git a/bogl-bgf.c b/bogl-bgf.c
index 1032028..0383f4f 100644
--- a/bogl-bgf.c
+++ b/bogl-bgf.c
@@ -5,38 +5,55 @@
#include
#include
#include
+#include
#include "bogl.h"
#include "boglP.h"
#include "bogl-font.h"
+#include "bogl-bgf.h"
+
+struct bogl_bgf {
+ void *f;/* mmap area */
+ off_t size; /* size of mmap area */
+ struct bogl_font font; /* font descriptor */
+};
+#define bgf_of(font) \
+ ((struct bogl_bgf *)((char *)(font) - offsetof(struct bogl_bgf, font)))
struct bogl_font *bogl_mmap_font(char *file)
{
- int fd;
+ struct bogl_bgf *bgf = NULL;
+ struct bogl_font *font = NULL;
+ int fd = -1;
struct stat buf;
- void *f;
- struct bogl_font *font;
+ void *f = (void *)-1;
+
+ bgf = (struct bogl_bgf *)malloc(sizeof(struct bogl_bgf));
+ if (!bgf)
+goto fail;
+ font = >font;
fd = open(file, O_RDONLY);
if (fd == -1)
-return 0;
+goto fail;
if (bogl_cloexec(fd) < 0)
-return 0;
+goto fail;
if (fstat(fd, ))
-return 0;
+goto fail;
+ bgf->size = buf.st_size;
+
+ if (buf.st_size < 4)
+goto fail;
f = mmap(0, buf.st_size, PROT_READ, MAP_SHARED, fd, 0);
if (f == (void *)-1)
-return 0;
+goto fail;
+ bgf->f = f;
if (memcmp("BGF1", f, 4))
-return 0;
-
- font = (struct bogl_font *)malloc(sizeof(struct bogl_font));
- if (!font)
-return 0;
+goto fail;
memcpy(font, f + 4, sizeof(*font));
font->name = ((void *)font->name - (void *)0) + f;
@@ -44,5 +61,25 @@ struct bogl_font *bogl_mmap_font(char *file)
font->index = ((void *)font->index - (void *)0) + f;
font->content = ((void *)font->content - (void *)0) + f;
+done:
+ if (fd != -1)
+close(fd);
return font;
+
+fail:
+ if (bgf) {
+free(bgf);
+bgf = NULL;
+font = NULL;
+ }
+ if (f != (void *)-1)
+munmap(f, buf.st_size);
+ goto done;
+}
+
+void bogl_munmap_font(struct bogl_font *font)
+{
+ struct bogl_bgf *bgf = bgf_of(font);
+ munmap(bgf->f, bgf->size);
+ free(bgf);
}
diff --git a/bogl-bgf.h b/bogl-bgf.h
index e9fb994..f14a260 100644
--- a/bogl-bgf.h
+++
Bug#1011609: bogl-bterm: [PATCH] Several improvements
Hi,
Another small patch. :-)
Best Regards,
Zhang BoyangFrom ae763e89f00575e56a7242e27c9b0789c0de411e Mon Sep 17 00:00:00 2001
From: Zhang Boyang
Date: Sun, 29 May 2022 02:45:32 +0800
Subject: [PATCH] Don't call FBIOPAN_DISPLAY when using the vga16fb driver
When using vga16fb, there is no need to call FBIOPAN_DISPLAY, and it may
cause screen flicker on certain hardwares.
---
bogl.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/bogl.c b/bogl.c
index 9b628b1..5cbe96e 100644
--- a/bogl.c
+++ b/bogl.c
@@ -462,6 +462,15 @@ bogl_fb_set_palette (int c, int nc, const unsigned char palette[][3])
void
bogl_update (void)
{
+#if BOGL_VGA16_FB
+ if (type == FB_TYPE_VGA_PLANES)
+{
+ /* There is no need to call FBIOPAN_DISPLAY when using vga16fb driver.
+ What's worse, it may cause screen flicker on certain hardwares.
+ So make bogl_update() a no-op here. */
+ return;
+}
+#endif
ioctl (fb, FBIOPAN_DISPLAY, _var);
}
--
2.30.2
Bug#1011609: bogl-bterm: [PATCH] Several improvements
Hi,
Here is another small patch. Please see the commit message of the patch
for details. The merge request on salsa is also updated.
Best Regards,
Zhang BoyangFrom 36020995b989563efed3cfe6028cd93c192c0208 Mon Sep 17 00:00:00 2001
From: Zhang Boyang
Date: Sat, 28 May 2022 16:39:23 +0800
Subject: [PATCH] Fix rightmost pixel of each line not cleared by
bogl_vga16_text()
The bogl_vga16_clear() function use [x1, x2) as clear range, so there is
no need to minus x2 by 1.
---
bogl-vga16.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/bogl-vga16.c b/bogl-vga16.c
index 6c2cbcf..3fcbe51 100644
--- a/bogl-vga16.c
+++ b/bogl-vga16.c
@@ -373,7 +373,7 @@ bogl_vga16_text (int xx, int yy, const char *s, int n, int fg, int bg, int ul,
{
int x2 = xx + bogl_metrics (s, n, font);
if (x2 >= bogl_xres)
- x2 = bogl_xres - 1;
+ x2 = bogl_xres;
bogl_vga16_clear (xx, yy, x2, yy + h, bg);
}
--
2.30.2
Bug#1011609: bogl-bterm: [PATCH] Several improvements
Package: bogl-bterm
Hello,
I made several improvements to bterm. Please see these patches for
details. For convenience, I also created a merge request at
https://salsa.debian.org/debian/bogl/-/merge_requests/2
Best Regards,
Zhang BoyangFrom 1e253d724658ffc22f1339212af3d2754249b251 Mon Sep 17 00:00:00 2001
From: Zhang Boyang
Date: Mon, 23 May 2022 14:10:52 +0800
Subject: [PATCH 1/7] Better font reload handling
Previous font reload code will leak a mmap on each reload. This patch
adds the ability to munmap old font after reload. However, this also
introduces a bug, if font reload is triggered while drawing in progress,
after signal handler returns, the drawing code will continue to use old
font which has been freed, causing crash. This will be fixed in later
patches.
---
bogl-bgf.c | 61 +++---
bogl-bgf.h | 1 +
bterm.c| 10 -
3 files changed, 54 insertions(+), 18 deletions(-)
diff --git a/bogl-bgf.c b/bogl-bgf.c
index 1032028..0383f4f 100644
--- a/bogl-bgf.c
+++ b/bogl-bgf.c
@@ -5,38 +5,55 @@
#include
#include
#include
+#include
#include "bogl.h"
#include "boglP.h"
#include "bogl-font.h"
+#include "bogl-bgf.h"
+
+struct bogl_bgf {
+ void *f;/* mmap area */
+ off_t size; /* size of mmap area */
+ struct bogl_font font; /* font descriptor */
+};
+#define bgf_of(font) \
+ ((struct bogl_bgf *)((char *)(font) - offsetof(struct bogl_bgf, font)))
struct bogl_font *bogl_mmap_font(char *file)
{
- int fd;
+ struct bogl_bgf *bgf = NULL;
+ struct bogl_font *font = NULL;
+ int fd = -1;
struct stat buf;
- void *f;
- struct bogl_font *font;
+ void *f = (void *)-1;
+
+ bgf = (struct bogl_bgf *)malloc(sizeof(struct bogl_bgf));
+ if (!bgf)
+goto fail;
+ font = >font;
fd = open(file, O_RDONLY);
if (fd == -1)
-return 0;
+goto fail;
if (bogl_cloexec(fd) < 0)
-return 0;
+goto fail;
if (fstat(fd, ))
-return 0;
+goto fail;
+ bgf->size = buf.st_size;
+
+ if (buf.st_size < 4)
+goto fail;
f = mmap(0, buf.st_size, PROT_READ, MAP_SHARED, fd, 0);
if (f == (void *)-1)
-return 0;
+goto fail;
+ bgf->f = f;
if (memcmp("BGF1", f, 4))
-return 0;
-
- font = (struct bogl_font *)malloc(sizeof(struct bogl_font));
- if (!font)
-return 0;
+goto fail;
memcpy(font, f + 4, sizeof(*font));
font->name = ((void *)font->name - (void *)0) + f;
@@ -44,5 +61,25 @@ struct bogl_font *bogl_mmap_font(char *file)
font->index = ((void *)font->index - (void *)0) + f;
font->content = ((void *)font->content - (void *)0) + f;
+done:
+ if (fd != -1)
+close(fd);
return font;
+
+fail:
+ if (bgf) {
+free(bgf);
+bgf = NULL;
+font = NULL;
+ }
+ if (f != (void *)-1)
+munmap(f, buf.st_size);
+ goto done;
+}
+
+void bogl_munmap_font(struct bogl_font *font)
+{
+ struct bogl_bgf *bgf = bgf_of(font);
+ munmap(bgf->f, bgf->size);
+ free(bgf);
}
diff --git a/bogl-bgf.h b/bogl-bgf.h
index e9fb994..f14a260 100644
--- a/bogl-bgf.h
+++ b/bogl-bgf.h
@@ -1,2 +1,3 @@
struct bogl_font *bogl_mmap_font(char *file);
+void bogl_munmap_font(struct bogl_font *font);
diff --git a/bterm.c b/bterm.c
index d7b574f..22cbef5 100644
--- a/bterm.c
+++ b/bterm.c
@@ -215,15 +215,13 @@ void reload_font(int sig)
font = bogl_mmap_font (font_name);
if (font == NULL)
{
- fprintf(stderr, "Bad font\n");
+ /* Fail silently. To simplify implementation, debian-installer may
+ trigger redundant font reload requests, and the font file may not
+ available at that time, so handle it gracefully. */
return;
}
- /* This leaks a mmap. Since the font reloading feature is only expected
- to be used once per session (for instance, in debian-installer, after
- the font is replaced with a larger version containing more characters),
- we don't worry about the leak. */
- free(term->font);
+ bogl_munmap_font(term->font);
term->font = font;
term->xstep = bogl_font_glyph(term->font, ' ', 0);
--
2.30.2
From 0f1b0b57a2b36ae4e04a5a32c9a55634a1cbae39 Mon Sep 17 00:00:00 2001
From: Zhang Boyang
Date: Mon, 23 May 2022 22:07:37 +0800
Subject: [PATCH 2/7] Use ioctl(FBIOPAN_DISPLAY) to update screen after drawing
Some framebuffer driver like i915 need explicit notify after drawing,
otherwise modifications in graphics buffer will not be reflected on
screen, so use FBIOPAN_DISPLAY to do this notify.
---
bogl-term.c | 1 +
bogl.c | 10 +-
bogl.h | 1 +
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/bogl-term.c b/bogl-term.c
index d5780d3..1f68523 100644
--- a/bogl-term.c
+++ b/bogl-term.c
@@ -863,4 +863,5 @@ bogl_term_redraw (struct bogl_term *term)
{
show_cursor(term, 1);
}
+bogl_update();
}
diff --git a/bogl.c b/bogl.c
index 6b9996b..a3014e8 100644
--- a/bogl.c
+++ b/bogl.c
@@ -123,12 +123,12 @@ static void kbd_done (void);
static void vt_switch
