Bug#852757: apt calls malloc inside SIGWINCH handler, leading to deadlock

[Zhang Boyang]

Hello,

It seems my last email was marked spam. It's strange that the bug 
tracker has received my reply but the mailing list didn't. Please 
correct me if I misunderstand the situation.

My previous reply can be viewed at 
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=852757

The patch is attached again for convenience. This patch implemented 
Anders's idea. The signal handler will only set a flag. After signal 
handler completed, the pselect() in pkgDPkgPM::Go() will return 
immediately with errno set to EINTR. Then, progress->Pluse() is called 
by pkgDPkgPM::Go(), and PackageManagerFancy::Pluse() will redraw the 
status bar.

I looked around for similar bugs. I found #947279 might be the same bug, 
but I don't have much evidence.

Best regards,
Zhang BoyangFrom 6cdd7fe9476a8149bc5bf18f70f9a8a30ba92d3a Mon Sep 17 00:00:00 2001
From: Zhang Boyang <zhangboyang...@gmail.com>
Date: Wed, 24 Nov 2021 23:34:04 +0800
Subject: [PATCH] Fix incorrect SIGWINCH handling

Previously, status line is redrawn in signal handler. However, the
drawing code make heavy use of std::string and other syscalls, which may
not be async-signal-safe. This will cause deadlock, overwritten errno,
even silent memory corruption.

With this patch, the signal handler will only set a flag, which is
async-signal-safe, and actual redrawing will be deferred to Pulse().

Closes: #852757
---
 apt-pkg/install-progress.cc | 31 +++++++++++++++++++++----------
 apt-pkg/install-progress.h  |  8 +++++---
 2 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/apt-pkg/install-progress.cc b/apt-pkg/install-progress.cc
index aadd28e51..99f16bffa 100644
--- a/apt-pkg/install-progress.cc
+++ b/apt-pkg/install-progress.cc
@@ -222,22 +222,22 @@ PackageManagerFancy::PackageManagerFancy()
    : d(NULL), child_pty(-1)
 {
    // setup terminal size
-   old_SIGWINCH = signal(SIGWINCH, PackageManagerFancy::staticSIGWINCH);
-   instances.push_back(this);
+   if (instances++ == 0)
+      old_SIGWINCH = signal(SIGWINCH, PackageManagerFancy::staticSIGWINCH);
 }
-std::vector<PackageManagerFancy*> PackageManagerFancy::instances;
+int PackageManagerFancy::instances = 0;
+sighandler_t PackageManagerFancy::old_SIGWINCH;
+volatile sig_atomic_t PackageManagerFancy::SIGWINCH_flag = 0;
 
 PackageManagerFancy::~PackageManagerFancy()
 {
-   instances.erase(find(instances.begin(), instances.end(), this));
-   signal(SIGWINCH, old_SIGWINCH);
+   if (--instances == 0)
+      signal(SIGWINCH, old_SIGWINCH);
 }
 
-void PackageManagerFancy::staticSIGWINCH(int signum)
+void PackageManagerFancy::staticSIGWINCH(int)
 {
-   std::vector<PackageManagerFancy *>::const_iterator I;
-   for(I = instances.begin(); I != instances.end(); ++I)
-      (*I)->HandleSIGWINCH(signum);
+   SIGWINCH_flag = 1;
 }
 
 PackageManagerFancy::TermSize
@@ -294,13 +294,24 @@ void PackageManagerFancy::SetupTerminalScrollArea(int nr_rows)
      }
 }
 
-void PackageManagerFancy::HandleSIGWINCH(int)
+void PackageManagerFancy::HandleSIGWINCH()
 {
    int const nr_terminal_rows = GetTerminalSize().rows;
    SetupTerminalScrollArea(nr_terminal_rows);
    DrawStatusLine();
 }
 
+void PackageManagerFancy::Pulse()
+{
+   if (SIGWINCH_flag)
+   {
+      SIGWINCH_flag = 0;
+      int errsv = errno;
+      HandleSIGWINCH();
+      errno = errsv;
+   }
+}
+
 void PackageManagerFancy::Start(int a_child_pty)
 {
    child_pty = a_child_pty;
diff --git a/apt-pkg/install-progress.h b/apt-pkg/install-progress.h
index 94b66ed9b..d1dd3eb8a 100644
--- a/apt-pkg/install-progress.h
+++ b/apt-pkg/install-progress.h
@@ -125,12 +125,14 @@ namespace Progress {
     void * const d;
  private:
     APT_HIDDEN static void staticSIGWINCH(int);
-    static std::vector<PackageManagerFancy*> instances;
+    static int instances;
+    static sighandler_t old_SIGWINCH;
+    static volatile sig_atomic_t SIGWINCH_flag;
     APT_HIDDEN bool DrawStatusLine();
 
  protected:
     void SetupTerminalScrollArea(int nr_rows);
-    void HandleSIGWINCH(int);
+    void HandleSIGWINCH();
 
     typedef struct {
        int rows;
@@ -138,12 +140,12 @@ namespace Progress {
     } TermSize;
     TermSize GetTerminalSize();
 
-    sighandler_t old_SIGWINCH;
     int child_pty;
 
  public:
     PackageManagerFancy();
     virtual ~PackageManagerFancy();
+    virtual void Pulse() APT_OVERRIDE;
     virtual void Start(int child_pty=-1) APT_OVERRIDE;
     virtual void Stop() APT_OVERRIDE;
     virtual bool StatusChanged(std::string PackageName,
-- 
2.30.2