From e29aaea52c0924e631f5fb69092626e1345be468 Mon Sep 17 00:00:00 2001 From: Florian Weimer Date: Mon, 22 May 2023 15:22:04 +0200 Subject: [PATCH] gmon: Various bug fixes (#2180155) Resolves: #2180155 --- glibc-rh2180155-1.patch | 70 ++++++ glibc-rh2180155-2.patch | 477 ++++++++++++++++++++++++++++++++++++++++ glibc-rh2180155-3.patch | 191 ++++++++++++++++ glibc.spec | 8 +- 4 files changed, 745 insertions(+), 1 deletion(-) create mode 100644 glibc-rh2180155-1.patch create mode 100644 glibc-rh2180155-2.patch create mode 100644 glibc-rh2180155-3.patch diff --git a/glibc-rh2180155-1.patch b/glibc-rh2180155-1.patch new file mode 100644 index 0000000..f38a0ec --- /dev/null +++ b/glibc-rh2180155-1.patch @@ -0,0 +1,70 @@ +commit 801af9fafd4689337ebf27260aa115335a0cb2bc +Author: Леонид Юрьев (Leonid Yuriev) +Date: Sat Feb 4 14:41:38 2023 +0300 + + gmon: Fix allocated buffer overflow (bug 29444) + + The `__monstartup()` allocates a buffer used to store all the data + accumulated by the monitor. + + The size of this buffer depends on the size of the internal structures + used and the address range for which the monitor is activated, as well + as on the maximum density of call instructions and/or callable functions + that could be potentially on a segment of executable code. + + In particular a hash table of arcs is placed at the end of this buffer. + The size of this hash table is calculated in bytes as + p->fromssize = p->textsize / HASHFRACTION; + + but actually should be + p->fromssize = ROUNDUP(p->textsize / HASHFRACTION, sizeof(*p->froms)); + + This results in writing beyond the end of the allocated buffer when an + added arc corresponds to a call near from the end of the monitored + address range, since `_mcount()` check the incoming caller address for + monitored range but not the intermediate result hash-like index that + uses to write into the table. + + It should be noted that when the results are output to `gmon.out`, the + table is read to the last element calculated from the allocated size in + bytes, so the arcs stored outside the buffer boundary did not fall into + `gprof` for analysis. Thus this "feature" help me to found this bug + during working with https://sourceware.org/bugzilla/show_bug.cgi?id=29438 + + Just in case, I will explicitly note that the problem breaks the + `make test t=gmon/tst-gmon-dso` added for Bug 29438. + There, the arc of the `f3()` call disappears from the output, since in + the DSO case, the call to `f3` is located close to the end of the + monitored range. + + Signed-off-by: Леонид Юрьев (Leonid Yuriev) + + Another minor error seems a related typo in the calculation of + `kcountsize`, but since kcounts are smaller than froms, this is + actually to align the p->froms data. + + Co-authored-by: DJ Delorie + Reviewed-by: Carlos O'Donell + +diff --git a/gmon/gmon.c b/gmon/gmon.c +index dee64803ada583d7..bf76358d5b1aa2da 100644 +--- a/gmon/gmon.c ++++ b/gmon/gmon.c +@@ -132,6 +132,8 @@ __monstartup (u_long lowpc, u_long highpc) + p->lowpc = ROUNDDOWN(lowpc, HISTFRACTION * sizeof(HISTCOUNTER)); + p->highpc = ROUNDUP(highpc, HISTFRACTION * sizeof(HISTCOUNTER)); + p->textsize = p->highpc - p->lowpc; ++ /* This looks like a typo, but it's here to align the p->froms ++ section. */ + p->kcountsize = ROUNDUP(p->textsize / HISTFRACTION, sizeof(*p->froms)); + p->hashfraction = HASHFRACTION; + p->log_hashfraction = -1; +@@ -142,7 +144,7 @@ __monstartup (u_long lowpc, u_long highpc) + instead of integer division. Precompute shift amount. */ + p->log_hashfraction = ffs(p->hashfraction * sizeof(*p->froms)) - 1; + } +- p->fromssize = p->textsize / HASHFRACTION; ++ p->fromssize = ROUNDUP(p->textsize / HASHFRACTION, sizeof(*p->froms)); + p->tolimit = p->textsize * ARCDENSITY / 100; + if (p->tolimit < MINARCS) + p->tolimit = MINARCS; diff --git a/glibc-rh2180155-2.patch b/glibc-rh2180155-2.patch new file mode 100644 index 0000000..912bc92 --- /dev/null +++ b/glibc-rh2180155-2.patch @@ -0,0 +1,477 @@ +This patch adds the required @order directives to preserve the +GLIBC_PRIVATE ABI. + +commit 31be941e4367c001b2009308839db5c67bf9dcbc +Author: Simon Kissane +Date: Sat Feb 11 20:12:13 2023 +1100 + + gmon: improve mcount overflow handling [BZ# 27576] + + When mcount overflows, no gmon.out file is generated, but no message is printed + to the user, leaving the user with no idea why, and thinking maybe there is + some bug - which is how BZ 27576 ended up being logged. Print a message to + stderr in this case so the user knows what is going on. + + As a comment in sys/gmon.h acknowledges, the hardcoded MAXARCS value is too + small for some large applications, including the test case in that BZ. Rather + than increase it, add tunables to enable MINARCS and MAXARCS to be overridden + at runtime (glibc.gmon.minarcs and glibc.gmon.maxarcs). So if a user gets the + mcount overflow error, they can try increasing maxarcs (they might need to + increase minarcs too if the heuristic is wrong in their case.) + + Note setting minarcs/maxarcs too large can cause monstartup to fail with an + out of memory error. If you set them large enough, it can cause an integer + overflow in calculating the buffer size. I haven't done anything to defend + against that - it would not generally be a security vulnerability, since these + tunables will be ignored in suid/sgid programs (due to the SXID_ERASE default), + and if you can set GLIBC_TUNABLES in the environment of a process, you can take + it over anyway (LD_PRELOAD, LD_LIBRARY_PATH, etc). I thought about modifying + the code of monstartup to defend against integer overflows, but doing so is + complicated, and I realise the existing code is susceptible to them even prior + to this change (e.g. try passing a pathologically large highpc argument to + monstartup), so I decided just to leave that possibility in-place. + + Add a test case which demonstrates mcount overflow and the tunables. + + Document the new tunables in the manual. + + Signed-off-by: Simon Kissane + Reviewed-by: DJ Delorie + +Conflicts: + manual/tunables.texi + (missing tunables downstream) + +diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list +index f11ca5b3e8b09b43..dc2999796042dbaf 100644 +--- a/elf/dl-tunables.list ++++ b/elf/dl-tunables.list +@@ -149,4 +149,17 @@ glibc { + default: 2 + } + } ++ ++ gmon { ++ minarcs { ++ type: INT_32 ++ minval: 50 ++ default: 50 ++ } ++ maxarcs { ++ type: INT_32 ++ minval: 50 ++ default: 1048576 ++ } ++ } + } +diff --git a/gmon/Makefile b/gmon/Makefile +index d94593c9d8a882eb..54f05894d4dd8c4a 100644 +--- a/gmon/Makefile ++++ b/gmon/Makefile +@@ -25,7 +25,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 ++tests = tst-sprofil tst-gmon tst-mcount-overflow + ifeq ($(build-profile),yes) + tests += tst-profile-static + tests-static += tst-profile-static +@@ -56,6 +56,18 @@ ifeq ($(run-built-tests),yes) + tests-special += $(objpfx)tst-gmon-gprof.out + endif + ++CFLAGS-tst-mcount-overflow.c := -fno-omit-frame-pointer -pg ++tst-mcount-overflow-no-pie = yes ++CRT-tst-mcount-overflow := $(csu-objpfx)g$(start-installed-name) ++# Intentionally use invalid config where maxarcs&1 1>/dev/null | cat ++ifeq ($(run-built-tests),yes) ++tests-special += $(objpfx)tst-mcount-overflow-check.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 +@@ -103,6 +115,14 @@ $(objpfx)tst-gmon.out: clean-tst-gmon-data + clean-tst-gmon-data: + rm -f $(objpfx)tst-gmon.data.* + ++$(objpfx)tst-mcount-overflow.o: clean-tst-mcount-overflow-data ++clean-tst-mcount-overflow-data: ++ rm -f $(objpfx)tst-mcount-overflow.data.* ++ ++$(objpfx)tst-mcount-overflow-check.out: tst-mcount-overflow-check.sh $(objpfx)tst-mcount-overflow.out ++ $(SHELL) $< $(objpfx)tst-mcount-overflow > $@; \ ++ $(evaluate-test) ++ + $(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 bf76358d5b1aa2da..689bf80141e559ca 100644 +--- a/gmon/gmon.c ++++ b/gmon/gmon.c +@@ -46,6 +46,11 @@ + #include + #include + ++#if HAVE_TUNABLES ++# define TUNABLE_NAMESPACE gmon ++# include ++#endif ++ + #ifdef PIC + # include + +@@ -124,6 +129,22 @@ __monstartup (u_long lowpc, u_long highpc) + int o; + char *cp; + struct gmonparam *p = &_gmonparam; ++ long int minarcs, maxarcs; ++ ++#if HAVE_TUNABLES ++ /* Read minarcs/maxarcs tunables. */ ++ minarcs = TUNABLE_GET (minarcs, int32_t, NULL); ++ maxarcs = TUNABLE_GET (maxarcs, int32_t, NULL); ++ if (maxarcs < minarcs) ++ { ++ ERR("monstartup: maxarcs < minarcs, setting maxarcs = minarcs\n"); ++ maxarcs = minarcs; ++ } ++#else ++ /* No tunables, we use hardcoded defaults */ ++ minarcs = MINARCS; ++ maxarcs = MAXARCS; ++#endif + + /* + * round lowpc and highpc to multiples of the density we're using +@@ -146,10 +167,10 @@ __monstartup (u_long lowpc, u_long highpc) + } + p->fromssize = ROUNDUP(p->textsize / HASHFRACTION, sizeof(*p->froms)); + p->tolimit = p->textsize * ARCDENSITY / 100; +- if (p->tolimit < MINARCS) +- p->tolimit = MINARCS; +- else if (p->tolimit > MAXARCS) +- p->tolimit = MAXARCS; ++ if (p->tolimit < minarcs) ++ p->tolimit = minarcs; ++ else if (p->tolimit > maxarcs) ++ p->tolimit = maxarcs; + p->tossize = p->tolimit * sizeof(struct tostruct); + + cp = calloc (p->kcountsize + p->fromssize + p->tossize, 1); +diff --git a/gmon/mcount.c b/gmon/mcount.c +index 9d4a1a50fa6ab21a..f7180fdb83399a14 100644 +--- a/gmon/mcount.c ++++ b/gmon/mcount.c +@@ -41,6 +41,10 @@ static char sccsid[] = "@(#)mcount.c 8.1 (Berkeley) 6/4/93"; + + #include + ++#include ++#include ++#define ERR(s) __write_nocancel (STDERR_FILENO, s, sizeof (s) - 1) ++ + /* + * mcount is called on entry to each function compiled with the profiling + * switch set. _mcount(), which is declared in a machine-dependent way +@@ -170,6 +174,7 @@ done: + return; + overflow: + p->state = GMON_PROF_ERROR; ++ ERR("mcount: call graph buffer size limit exceeded, gmon.out will not be generated\n"); + return; + } + +diff --git a/gmon/sys/gmon.h b/gmon/sys/gmon.h +index b4cc3b043a2aec77..af0582a3717085b5 100644 +--- a/gmon/sys/gmon.h ++++ b/gmon/sys/gmon.h +@@ -111,6 +111,8 @@ extern struct __bb *__bb_head; + * Always allocate at least this many tostructs. This + * hides the inadequacy of the ARCDENSITY heuristic, at least + * for small programs. ++ * ++ * Value can be overridden at runtime by glibc.gmon.minarcs tunable. + */ + #define MINARCS 50 + +@@ -124,8 +126,8 @@ extern struct __bb *__bb_head; + * Used to be max representable value of ARCINDEX minus 2, but now + * that ARCINDEX is a long, that's too large; we don't really want + * to allow a 48 gigabyte table. +- * The old value of 1<<16 wasn't high enough in practice for large C++ +- * programs; will 1<<20 be adequate for long? FIXME ++ * ++ * Value can be overridden at runtime by glibc.gmon.maxarcs tunable. + */ + #define MAXARCS (1 << 20) + +diff --git a/gmon/tst-mcount-overflow-check.sh b/gmon/tst-mcount-overflow-check.sh +new file mode 100644 +index 0000000000000000..27eb5538fd573a6e +--- /dev/null ++++ b/gmon/tst-mcount-overflow-check.sh +@@ -0,0 +1,45 @@ ++#!/bin/sh ++# Test expected messages generated when mcount overflows ++# Copyright (C) 2017-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 ++# 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 ++# . ++ ++LC_ALL=C ++export LC_ALL ++set -e ++exec 2>&1 ++ ++program="$1" ++ ++check_msg() { ++ if ! grep -q "$1" "$program.out"; then ++ echo "FAIL: expected message not in output: $1" ++ exit 1 ++ fi ++} ++ ++check_msg 'monstartup: maxarcs < minarcs, setting maxarcs = minarcs' ++check_msg 'mcount: call graph buffer size limit exceeded, gmon.out will not be generated' ++ ++for data_file in $1.data.*; do ++ if [ -f "$data_file" ]; then ++ echo "FAIL: expected no data files, but found $data_file" ++ exit 1 ++ fi ++done ++ ++echo PASS +diff --git a/gmon/tst-mcount-overflow.c b/gmon/tst-mcount-overflow.c +new file mode 100644 +index 0000000000000000..06cc93ef872eb7c1 +--- /dev/null ++++ b/gmon/tst-mcount-overflow.c +@@ -0,0 +1,72 @@ ++/* Test program to trigger mcount overflow in profiling collection. ++ Copyright (C) 2017-2023 Free Software Foundation, Inc. ++ 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 ++ . */ ++ ++/* Program with sufficiently complex, yet pointless, call graph ++ that it will trigger an mcount overflow, when you set the ++ minarcs/maxarcs tunables to very low values. */ ++ ++#define PREVENT_TAIL_CALL asm volatile ("") ++ ++/* Calls REP(n) macro 16 times, for n=0..15. ++ * You need to define REP(n) before using this. ++ */ ++#define REPS \ ++ REP(0) REP(1) REP(2) REP(3) REP(4) REP(5) REP(6) REP(7) \ ++ REP(8) REP(9) REP(10) REP(11) REP(12) REP(13) REP(14) REP(15) ++ ++/* Defines 16 leaf functions named f1_0 to f1_15 */ ++#define REP(n) \ ++ __attribute__ ((noinline, noclone, weak)) void f1_##n (void) {}; ++REPS ++#undef REP ++ ++/* Calls all 16 leaf functions f1_* in succession */ ++__attribute__ ((noinline, noclone, weak)) void ++f2 (void) ++{ ++# define REP(n) f1_##n(); ++ REPS ++# undef REP ++ PREVENT_TAIL_CALL; ++} ++ ++/* Defines 16 functions named f2_0 to f2_15, which all just call f2 */ ++#define REP(n) \ ++ __attribute__ ((noinline, noclone, weak)) void \ ++ f2_##n (void) { f2(); PREVENT_TAIL_CALL; }; ++REPS ++#undef REP ++ ++__attribute__ ((noinline, noclone, weak)) void ++f3 (int count) ++{ ++ for (int i = 0; i < count; ++i) ++ { ++ /* Calls f1_0(), f2_0(), f1_1(), f2_1(), f3_0(), etc */ ++# define REP(n) f1_##n(); f2_##n(); ++ REPS ++# undef REP ++ } ++} ++ ++int ++main (void) ++{ ++ f3 (1000); ++ return 0; ++} +diff --git a/manual/tunables.texi b/manual/tunables.texi +index 7b70e80391ee87f7..00eafcf44b562b9e 100644 +--- a/manual/tunables.texi ++++ b/manual/tunables.texi +@@ -73,6 +73,9 @@ glibc.malloc.check: 0 (min: 0, max: 3) + * Elision Tunables:: Tunables in elision subsystem + * Hardware Capability Tunables:: Tunables that modify the hardware + capabilities seen by @theglibc{} ++* gmon Tunables:: Tunables that control the gmon profiler, used in ++ conjunction with gprof ++ + @end menu + + @node Tunable names +@@ -506,3 +509,59 @@ instead. + + This tunable is specific to i386 and x86-64. + @end deftp ++ ++@node gmon Tunables ++@section gmon Tunables ++@cindex gmon tunables ++ ++@deftp {Tunable namespace} glibc.gmon ++This tunable namespace affects the behaviour of the gmon profiler. ++gmon is a component of @theglibc{} which is normally used in ++conjunction with gprof. ++ ++When GCC compiles a program with the @code{-pg} option, it instruments ++the program with calls to the @code{mcount} function, to record the ++program's call graph. At program startup, a memory buffer is allocated ++to store this call graph; the size of the buffer is calculated using a ++heuristic based on code size. If during execution, the buffer is found ++to be too small, profiling will be aborted and no @file{gmon.out} file ++will be produced. In that case, you will see the following message ++printed to standard error: ++ ++@example ++mcount: call graph buffer size limit exceeded, gmon.out will not be generated ++@end example ++ ++Most of the symbols discussed in this section are defined in the header ++@code{sys/gmon.h}. However, some symbols (for example @code{mcount}) ++are not defined in any header file, since they are only intended to be ++called from code generated by the compiler. ++@end deftp ++ ++@deftp Tunable glibc.mem.minarcs ++The heuristic for sizing the call graph buffer is known to be ++insufficient for small programs; hence, the calculated value is clamped ++to be at least a minimum size. The default minimum (in units of ++call graph entries, @code{struct tostruct}), is given by the macro ++@code{MINARCS}. If you have some program with an unusually complex ++call graph, for which the heuristic fails to allocate enough space, ++you can use this tunable to increase the minimum to a larger value. ++@end deftp ++ ++@deftp Tunable glibc.mem.maxarcs ++To prevent excessive memory consumption when profiling very large ++programs, the call graph buffer is allowed to have a maximum of ++@code{MAXARCS} entries. For some very large programs, the default ++value of @code{MAXARCS} defined in @file{sys/gmon.h} is too small; in ++that case, you can use this tunable to increase it. ++ ++Note the value of the @code{maxarcs} tunable must be greater or equal ++to that of the @code{minarcs} tunable; if this constraint is violated, ++a warning will printed to standard error at program startup, and ++the @code{minarcs} value will be used as the maximum as well. ++ ++Setting either tunable too high may result in a call graph buffer ++whose size exceeds the available memory; in that case, an out of memory ++error will be printed at program startup, the profiler will be ++disabled, and no @file{gmon.out} file will be generated. ++@end deftp +diff --git a/sysdeps/unix/sysv/linux/aarch64/dl-tunables.list b/sysdeps/unix/sysv/linux/aarch64/dl-tunables.list +index 5c3c5292025607a1..265f82ef2be42fd0 100644 +--- a/sysdeps/unix/sysv/linux/aarch64/dl-tunables.list ++++ b/sysdeps/unix/sysv/linux/aarch64/dl-tunables.list +@@ -24,3 +24,7 @@ + + # Tunables added in RHEL 8.8.0 + @order glibc.rtld.dynamic_sort ++ ++# Tunables added in RHEL 8.9.0 ++@order glibc.gmon.minarcs ++@order glibc.gmon.maxarcs +diff --git a/sysdeps/unix/sysv/linux/i386/dl-tunables.list b/sysdeps/unix/sysv/linux/i386/dl-tunables.list +index b9cad4af62d9f2e5..9c1ccb86501c61e7 100644 +--- a/sysdeps/unix/sysv/linux/i386/dl-tunables.list ++++ b/sysdeps/unix/sysv/linux/i386/dl-tunables.list +@@ -31,3 +31,7 @@ + + # Tunables added in RHEL 8.8.0 + @order glibc.rtld.dynamic_sort ++ ++# Tunables added in RHEL 8.9.0 ++@order glibc.gmon.minarcs ++@order glibc.gmon.maxarcs +diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/dl-tunables.list b/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/dl-tunables.list +index ee1e6fca95e1f2da..c8bb1a8ec0283ac8 100644 +--- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/dl-tunables.list ++++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/dl-tunables.list +@@ -24,3 +24,7 @@ + + # Tunables added in RHEL 8.8.0 + @order glibc.rtld.dynamic_sort ++ ++# Tunables added in RHEL 8.9.0 ++@order glibc.gmon.minarcs ++@order glibc.gmon.maxarcs +diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/dl-tunables.list b/sysdeps/unix/sysv/linux/s390/s390-64/dl-tunables.list +index 099e28d8f8e67944..85b3a014ffcadc45 100644 +--- a/sysdeps/unix/sysv/linux/s390/s390-64/dl-tunables.list ++++ b/sysdeps/unix/sysv/linux/s390/s390-64/dl-tunables.list +@@ -23,3 +23,7 @@ + + # Tunables added in RHEL 8.8.0 + @order glibc.rtld.dynamic_sort ++ ++# Tunables added in RHEL 8.9.0 ++@order glibc.gmon.minarcs ++@order glibc.gmon.maxarcs +diff --git a/sysdeps/unix/sysv/linux/x86_64/64/dl-tunables.list b/sysdeps/unix/sysv/linux/x86_64/64/dl-tunables.list +index b9cad4af62d9f2e5..9c1ccb86501c61e7 100644 +--- a/sysdeps/unix/sysv/linux/x86_64/64/dl-tunables.list ++++ b/sysdeps/unix/sysv/linux/x86_64/64/dl-tunables.list +@@ -31,3 +31,7 @@ + + # Tunables added in RHEL 8.8.0 + @order glibc.rtld.dynamic_sort ++ ++# Tunables added in RHEL 8.9.0 ++@order glibc.gmon.minarcs ++@order glibc.gmon.maxarcs diff --git a/glibc-rh2180155-3.patch b/glibc-rh2180155-3.patch new file mode 100644 index 0000000..af5f7ec --- /dev/null +++ b/glibc-rh2180155-3.patch @@ -0,0 +1,191 @@ +commit bde121872001d8f3224eeafa5b7effb871c3fbca +Author: Simon Kissane +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 + Reviewed-by: DJ Delorie + +Conflicts: + gmon/Makefile + (copyright year update) + +diff --git a/gmon/Makefile b/gmon/Makefile +index 54f05894d4dd8c4a..1bc4ad6e14e292a9 100644 +--- a/gmon/Makefile ++++ b/gmon/Makefile +@@ -1,4 +1,5 @@ +-# Copyright (C) 1995-2018 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 ++ . */ ++ ++/* 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 ++ ++int ++main (void) ++{ ++ _mcleanup(); ++ return 0; ++} diff --git a/glibc.spec b/glibc.spec index 5524fd4..c52680c 100644 --- a/glibc.spec +++ b/glibc.spec @@ -1,6 +1,6 @@ %define glibcsrcdir glibc-2.28 %define glibcversion 2.28 -%define glibcrelease 227%{?dist} +%define glibcrelease 228%{?dist} # Pre-release tarballs are pulled in from git using a command that is # effectively: # @@ -1034,6 +1034,9 @@ Patch841: glibc-rh2154914-2.patch Patch842: glibc-rh2183081-1.patch Patch843: glibc-rh2183081-2.patch Patch844: glibc-rh2172949.patch +Patch845: glibc-rh2180155-1.patch +Patch846: glibc-rh2180155-2.patch +Patch847: glibc-rh2180155-3.patch ############################################################################## # Continued list of core "glibc" package information: @@ -2864,6 +2867,9 @@ fi %files -f compat-libpthread-nonshared.filelist -n compat-libpthread-nonshared %changelog +* Mon May 22 2023 Florian Weimer - 2.28-228 +- gmon: Various bug fixes (#2180155) + * Thu May 18 2023 Patsy Griffin - 2.28-227 - Change sgetsgent_r to set errno. (#2172949)