Bug#852757: apt calls malloc inside SIGWINCH handler, leading to deadlock
Hello,
On 2021/11/25 21:03, David Kalnischkies wrote:
Hi,
On Thu, Nov 25, 2021 at 07:30:45PM +0800, Zhang Boyang wrote:
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.
Indeed, looks like your mail was dropped at some point. If you supply
sufficient details listmasters might be able to tell you what exactly
went wrong.
(The BTS seems to assign your mails ~ -10 points which is very good, but
at the same time the list mail which passed through got 0, which is not
bad, but also not particular good citing e.g. "MIME_BASE64_TEXT_BOGUS(1)
R_DKIM_REJECT(1)". With patches you might be better of in salsa btw.)
I talked to the listmaster. The listmaster told me my mail is too large
for deity mail list. I will try to reduce my mail size in future :)
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.
Not having looked at the issue itself I just gave the patch a casual
glance: PackageManagerFancy is a public – hence exported – interface,
so you can't change member variables or add virtual methods as that
breaks the ABI.
I revised my patch. Now the patch only add static member variables,
which is not harmful to ABI. The Pluse() method already exists in base
class but newly overridden. I think this change will not crash existing
programs. However, existing compiled programs can still use
BaseClass::Pluse() because compilers may optimize virtual calls.
Personally I think this is not harmful because this will only cause
status line not redrawing, which may consider acceptable.
At some point we will have one I am sure, but if at all possible it
could be better to investigate if that can be solved some other way.
(Not exactly sure why that is public at all, given there seems to also
be a factory function to be able to hide it?)
If the Pluse() must not be overridden. I think the only way is to modify
pkgDPkgPM::Go(), let it call some static method in PackageManagerFancy
just after pselect(). However I'm not sure because I'm new to apt.
Can we perhaps make it (still) easier to reproduce? apt can be told to
use a different dpkg via Dir::Bin::dpkg – can we have that be a script
which just spits out 'offending' lines (= is it the fd messages or the
messages on stdout for example – or both) ?
Yes, with temporary modifications to code. I added another patch, which
will call malloc() in a tight loop while simulating. To reproduce the
problem, apply that patch and send SIGWINCH to apt continuously, like:
while :; do killall -SIGWINCH apt; done
Then, run apt like:
./apt install -s gnome
You will see apt crash in few seconds after install simulation started.
(Sorry for not being very specific at the moment as I am a bit starved
for Debian time; will try to give that some proper thought/review
~next week)
Best regards
David Kalnischkies
Have a nice day :)
Zhang Boyang
From 4c8f9d5ab13f3069caefeada38658b72f0ad1782 Mon Sep 17 00:00:00 2001
From: Zhang Boyang <zhangboyang.id@gmail.com>
Date: Thu, 2 Dec 2021 00:16:07 +0800
Subject: [PATCH 1/2] Make heavy use of malloc() while simulating
---
apt-pkg/algorithms.cc | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/apt-pkg/algorithms.cc b/apt-pkg/algorithms.cc
index fb0b7dca7..27bbe5490 100644
--- a/apt-pkg/algorithms.cc
+++ b/apt-pkg/algorithms.cc
@@ -119,6 +119,16 @@ bool pkgSimulate::Install(PkgIterator iPkg,string File)
}
bool pkgSimulate::RealInstall(PkgIterator iPkg,string /*File*/)
{
+ // Use malloc() heavily, try trigger bug #852757
+ for (int round = 1; round <= 100000; round++)
+ {
+ vector<void *> arr;
+ for (int sz = 1; sz <= 100; sz++)
+ arr.push_back(malloc(sz));
+ for (auto ptr: arr)
+ free(ptr);
+ }
+
// Adapt the iterator
PkgIterator Pkg = Sim.FindPkg(iPkg.Name(), iPkg.Arch());
Flags[Pkg->ID] = 1;
--
2.30.2
From ff09c6cacb3c2b5878d44fb370bf460fd27f6ec3 Mon Sep 17 00:00:00 2001
From: Zhang Boyang <zhangboyang.id@gmail.com>
Date: Thu, 2 Dec 2021 00:21:48 +0800
Subject: [PATCH 2/2] 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.
This patch implemented Anders Kaseorg's idea. The signal handler will
only set a flag, which is async-signal-safe, and actual redrawing will
be deferred to PackageManagerFancy::Pulse().
Note that the virtual function PackageManagerFancy::Pulse() already
exists in base class but newly overridden in PackageManagerFancy, so the
ABI compatibility should be OK. However, existing compiled programs may
not aware of this new function and continue to use old Pulse() if
compiler had done heavy optimization. Fortunately this is not too
harmful because this will only cause status line not redrawing, which
may consider acceptable.
Closes: #852757
---
apt-pkg/install-progress.cc | 34 ++++++++++++++++++++++++++--------
apt-pkg/install-progress.h | 8 ++++++--
2 files changed, 32 insertions(+), 10 deletions(-)
diff --git a/apt-pkg/install-progress.cc b/apt-pkg/install-progress.cc
index aadd28e51..8a6d87cd2 100644
--- a/apt-pkg/install-progress.cc
+++ b/apt-pkg/install-progress.cc
@@ -222,22 +222,42 @@ PackageManagerFancy::PackageManagerFancy()
: d(NULL), child_pty(-1)
{
// setup terminal size
- old_SIGWINCH = signal(SIGWINCH, PackageManagerFancy::staticSIGWINCH);
+ if (instances.empty())
+ SIGWINCH_orig = signal(SIGWINCH, PackageManagerFancy::staticSIGWINCH);
instances.push_back(this);
}
std::vector<PackageManagerFancy*> PackageManagerFancy::instances;
+sighandler_t PackageManagerFancy::SIGWINCH_orig;
+volatile sig_atomic_t PackageManagerFancy::SIGWINCH_flag = 0;
PackageManagerFancy::~PackageManagerFancy()
{
instances.erase(find(instances.begin(), instances.end(), this));
- signal(SIGWINCH, old_SIGWINCH);
+ if (instances.empty())
+ signal(SIGWINCH, SIGWINCH_orig);
}
void PackageManagerFancy::staticSIGWINCH(int signum)
{
- std::vector<PackageManagerFancy *>::const_iterator I;
- for(I = instances.begin(); I != instances.end(); ++I)
- (*I)->HandleSIGWINCH(signum);
+ SIGWINCH_flag = 1;
+}
+
+void PackageManagerFancy::CheckSIGWINCH()
+{
+ if (SIGWINCH_flag)
+ {
+ SIGWINCH_flag = 0;
+ int errsv = errno;
+ int const nr_terminal_rows = GetTerminalSize().rows;
+ SetupTerminalScrollArea(nr_terminal_rows);
+ DrawStatusLine();
+ errno = errsv;
+ }
+}
+
+void PackageManagerFancy::Pulse()
+{
+ CheckSIGWINCH();
}
PackageManagerFancy::TermSize
@@ -296,9 +316,7 @@ void PackageManagerFancy::SetupTerminalScrollArea(int nr_rows)
void PackageManagerFancy::HandleSIGWINCH(int)
{
- int const nr_terminal_rows = GetTerminalSize().rows;
- SetupTerminalScrollArea(nr_terminal_rows);
- DrawStatusLine();
+ // for abi compatibility, do not use
}
void PackageManagerFancy::Start(int a_child_pty)
diff --git a/apt-pkg/install-progress.h b/apt-pkg/install-progress.h
index 94b66ed9b..617ce2a35 100644
--- a/apt-pkg/install-progress.h
+++ b/apt-pkg/install-progress.h
@@ -126,11 +126,14 @@ namespace Progress {
private:
APT_HIDDEN static void staticSIGWINCH(int);
static std::vector<PackageManagerFancy*> instances;
+ static sighandler_t SIGWINCH_orig;
+ static volatile sig_atomic_t SIGWINCH_flag;
+ APT_HIDDEN void CheckSIGWINCH();
APT_HIDDEN bool DrawStatusLine();
protected:
void SetupTerminalScrollArea(int nr_rows);
- void HandleSIGWINCH(int);
+ void HandleSIGWINCH(int); // for abi compatibility, do not use
typedef struct {
int rows;
@@ -138,12 +141,13 @@ namespace Progress {
} TermSize;
TermSize GetTerminalSize();
- sighandler_t old_SIGWINCH;
+ sighandler_t old_SIGWINCH; // for abi compatibility, do not use
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
Reply to: