You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
188 lines
7.0 KiB
188 lines
7.0 KiB
commit f1b15d2005f5125529171db3be39026a1157c2a8 |
|
Author: Simon Kissane <skissane@gmail.com> |
|
Date: Sat Feb 11 08:58:02 2023 +1100 |
|
|
|
gmon: fix memory corruption issues [BZ# 30101] |
|
|
|
V2 of this patch fixes an issue in V1, where the state was changed to ON not |
|
OFF at end of _mcleanup. I hadn't noticed that (counterintuitively) ON=0 and |
|
OFF=3, hence zeroing the buffer turned it back on. So set the state to OFF |
|
after the memset. |
|
|
|
1. Prevent double free, and reads from unallocated memory, when |
|
_mcleanup is (incorrectly) called two or more times in a row, |
|
without an intervening call to __monstartup; with this patch, the |
|
second and subsequent calls effectively become no-ops instead. |
|
While setting tos=NULL is minimal fix, safest action is to zero the |
|
whole gmonparam buffer. |
|
|
|
2. Prevent memory leak when __monstartup is (incorrectly) called two |
|
or more times in a row, without an intervening call to _mcleanup; |
|
with this patch, the second and subsequent calls effectively become |
|
no-ops instead. |
|
|
|
3. After _mcleanup, treat __moncontrol(1) as __moncontrol(0) instead. |
|
With zeroing of gmonparam buffer in _mcleanup, this stops the |
|
state incorrectly being changed to GMON_PROF_ON despite profiling |
|
actually being off. If we'd just done the minimal fix to _mcleanup |
|
of setting tos=NULL, there is risk of far worse memory corruption: |
|
kcount would point to deallocated memory, and the __profil syscall |
|
would make the kernel write profiling data into that memory, |
|
which could have since been reallocated to something unrelated. |
|
|
|
4. Ensure __moncontrol(0) still turns off profiling even in error |
|
state. Otherwise, if mcount overflows and sets state to |
|
GMON_PROF_ERROR, when _mcleanup calls __moncontrol(0), the __profil |
|
syscall to disable profiling will not be invoked. _mcleanup will |
|
free the buffer, but the kernel will still be writing profiling |
|
data into it, potentially corrupted arbitrary memory. |
|
|
|
Also adds a test case for (1). Issues (2)-(4) are not feasible to test. |
|
|
|
Signed-off-by: Simon Kissane <skissane@gmail.com> |
|
Reviewed-by: DJ Delorie <dj@redhat.com> |
|
(cherry picked from commit bde121872001d8f3224eeafa5b7effb871c3fbca) |
|
|
|
diff --git a/gmon/Makefile b/gmon/Makefile |
|
index 706f50f7dd4cae84..7fd9db8f749a0843 100644 |
|
--- a/gmon/Makefile |
|
+++ b/gmon/Makefile |
|
@@ -1,4 +1,5 @@ |
|
-# Copyright (C) 1995-2021 Free Software Foundation, Inc. |
|
+# Copyright (C) 1995-2023 Free Software Foundation, Inc. |
|
+# Copyright The GNU Toolchain Authors. |
|
# This file is part of the GNU C Library. |
|
|
|
# The GNU C Library is free software; you can redistribute it and/or |
|
@@ -25,7 +26,7 @@ include ../Makeconfig |
|
headers := sys/gmon.h sys/gmon_out.h sys/profil.h |
|
routines := gmon mcount profil sprofil prof-freq |
|
|
|
-tests = tst-sprofil tst-gmon tst-mcount-overflow |
|
+tests = tst-sprofil tst-gmon tst-mcount-overflow tst-mcleanup |
|
ifeq ($(build-profile),yes) |
|
tests += tst-profile-static |
|
tests-static += tst-profile-static |
|
@@ -68,6 +69,14 @@ ifeq ($(run-built-tests),yes) |
|
tests-special += $(objpfx)tst-mcount-overflow-check.out |
|
endif |
|
|
|
+CFLAGS-tst-mcleanup.c := -fno-omit-frame-pointer -pg |
|
+tst-mcleanup-no-pie = yes |
|
+CRT-tst-mcleanup := $(csu-objpfx)g$(start-installed-name) |
|
+tst-mcleanup-ENV := GMON_OUT_PREFIX=$(objpfx)tst-mcleanup.data |
|
+ifeq ($(run-built-tests),yes) |
|
+tests-special += $(objpfx)tst-mcleanup.out |
|
+endif |
|
+ |
|
CFLAGS-tst-gmon-static.c := $(PIE-ccflag) -fno-omit-frame-pointer -pg |
|
CRT-tst-gmon-static := $(csu-objpfx)gcrt1.o |
|
tst-gmon-static-no-pie = yes |
|
@@ -123,6 +132,10 @@ $(objpfx)tst-mcount-overflow-check.out: tst-mcount-overflow-check.sh $(objpfx)ts |
|
$(SHELL) $< $(objpfx)tst-mcount-overflow > $@; \ |
|
$(evaluate-test) |
|
|
|
+$(objpfx)tst-mcleanup.out: clean-tst-mcleanup-data |
|
+clean-tst-mcleanup-data: |
|
+ rm -f $(objpfx)tst-mcleanup.data.* |
|
+ |
|
$(objpfx)tst-gmon-gprof.out: tst-gmon-gprof.sh $(objpfx)tst-gmon.out |
|
$(SHELL) $< $(GPROF) $(objpfx)tst-gmon $(objpfx)tst-gmon.data.* > $@; \ |
|
$(evaluate-test) |
|
diff --git a/gmon/gmon.c b/gmon/gmon.c |
|
index 689bf80141e559ca..5e99a7351dc71666 100644 |
|
--- a/gmon/gmon.c |
|
+++ b/gmon/gmon.c |
|
@@ -102,11 +102,8 @@ __moncontrol (int mode) |
|
{ |
|
struct gmonparam *p = &_gmonparam; |
|
|
|
- /* Don't change the state if we ran into an error. */ |
|
- if (p->state == GMON_PROF_ERROR) |
|
- return; |
|
- |
|
- if (mode) |
|
+ /* Treat start request as stop if error or gmon not initialized. */ |
|
+ if (mode && p->state != GMON_PROF_ERROR && p->tos != NULL) |
|
{ |
|
/* start */ |
|
__profil((void *) p->kcount, p->kcountsize, p->lowpc, s_scale); |
|
@@ -116,7 +113,9 @@ __moncontrol (int mode) |
|
{ |
|
/* stop */ |
|
__profil(NULL, 0, 0, 0); |
|
- p->state = GMON_PROF_OFF; |
|
+ /* Don't change the state if we ran into an error. */ |
|
+ if (p->state != GMON_PROF_ERROR) |
|
+ p->state = GMON_PROF_OFF; |
|
} |
|
} |
|
libc_hidden_def (__moncontrol) |
|
@@ -146,6 +145,14 @@ __monstartup (u_long lowpc, u_long highpc) |
|
maxarcs = MAXARCS; |
|
#endif |
|
|
|
+ /* |
|
+ * If we are incorrectly called twice in a row (without an |
|
+ * intervening call to _mcleanup), ignore the second call to |
|
+ * prevent leaking memory. |
|
+ */ |
|
+ if (p->tos != NULL) |
|
+ return; |
|
+ |
|
/* |
|
* round lowpc and highpc to multiples of the density we're using |
|
* so the rest of the scaling (here and in gprof) stays in ints. |
|
@@ -463,9 +470,14 @@ _mcleanup (void) |
|
{ |
|
__moncontrol (0); |
|
|
|
- if (_gmonparam.state != GMON_PROF_ERROR) |
|
+ if (_gmonparam.state != GMON_PROF_ERROR && _gmonparam.tos != NULL) |
|
write_gmon (); |
|
|
|
/* free the memory. */ |
|
free (_gmonparam.tos); |
|
+ |
|
+ /* reset buffer to initial state for safety */ |
|
+ memset(&_gmonparam, 0, sizeof _gmonparam); |
|
+ /* somewhat confusingly, ON=0, OFF=3 */ |
|
+ _gmonparam.state = GMON_PROF_OFF; |
|
} |
|
diff --git a/gmon/tst-mcleanup.c b/gmon/tst-mcleanup.c |
|
new file mode 100644 |
|
index 0000000000000000..b259653ec833aca4 |
|
--- /dev/null |
|
+++ b/gmon/tst-mcleanup.c |
|
@@ -0,0 +1,31 @@ |
|
+/* Test program for repeated invocation of _mcleanup |
|
+ Copyright The GNU Toolchain Authors. |
|
+ This file is part of the GNU C Library. |
|
+ |
|
+ The GNU C Library is free software; you can redistribute it and/or |
|
+ modify it under the terms of the GNU Lesser General Public |
|
+ License as published by the Free Software Foundation; either |
|
+ version 2.1 of the License, or (at your option) any later version. |
|
+ |
|
+ The GNU C Library is distributed in the hope that it will be useful, |
|
+ but WITHOUT ANY WARRANTY; without even the implied warranty of |
|
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU |
|
+ Lesser General Public License for more details. |
|
+ |
|
+ You should have received a copy of the GNU Lesser General Public |
|
+ License along with the GNU C Library; if not, see |
|
+ <https://www.gnu.org/licenses/>. */ |
|
+ |
|
+/* Intentionally calls _mcleanup() twice: once manually, it will be |
|
+ called again as an atexit handler. This is incorrect use of the API, |
|
+ but the point of the test is to make sure we don't crash when the |
|
+ API is misused in this way. */ |
|
+ |
|
+#include <sys/gmon.h> |
|
+ |
|
+int |
|
+main (void) |
|
+{ |
|
+ _mcleanup(); |
|
+ return 0; |
|
+}
|
|
|