| Message ID | 20241230163153.2399453-1-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | assert: Remove the use of %n from __assert_fail_base (BZ #32456) | expand |
* Adhemerval Zanella: > The require size for mmap can be inferred from __vasprintf return > value. It also fixes tst-assert-2 when building with --enable-fortify, > where even if the format is not translated, __readonly_area fails > because malloc can not be used. > > Checked on aarch64-linux-gnu. > --- > assert/assert-perr.c | 2 +- > assert/assert.c | 27 ++++++++++----------------- > 2 files changed, 11 insertions(+), 18 deletions(-) > > diff --git a/assert/assert-perr.c b/assert/assert-perr.c > index 0010cbc278..8a03620b4c 100644 > --- a/assert/assert-perr.c > +++ b/assert/assert-perr.c > @@ -32,7 +32,7 @@ __assert_perror_fail (int errnum, > char errbuf[1024]; > > char *e = __strerror_r (errnum, errbuf, sizeof errbuf); > - __assert_fail_base (_("%s%s%s:%u: %s%sUnexpected error: %s.\n%n"), > + __assert_fail_base (_("%s%s%s:%u: %s%sUnexpected error: %s.\n"), > e, file, line, function); > } > libc_hidden_def (__assert_perror_fail) I think you have to update the translations in the same commit, otherwise we'll end up with crashes due to %n leading to a write through an uninitialized pointer. > @@ -56,12 +49,12 @@ __assert_fail_base (const char *fmt, const char *assertion, const char *file, > FATAL_PREPARE; > #endif > > - int total; > - if (__asprintf (&str, fmt, > - __progname, __progname[0] ? ": " : "", > - file, line, > - function ? function : "", function ? ": " : "", > - assertion, &total) >= 0) > + int total = __asprintf (&str, fmt, > + __progname, __progname[0] ? ": " : "", > + file, line, > + function ? function : "", function ? ": " : "", > + assertion); > + if (total >= 0) > { > /* Print the message. */ > (void) __fxprintf (NULL, "%s", str); Somewhat unrelated: The code is rather odd. Why would we use mmap here in addition to malloc? It doesn't matter that much anymore because malloc's own asserts no longer use this code. Still I suppose we could allocate a single patch and use __snprintf there. Anyway, this can wait for a later cleanup. We should remove the use of __fxprintf, too, although it won't be 100% backwards-compatible (for example, write the message only if there is a file descriptor associated with the stderr stream, so that we can write bytes directly without going through the wide stream state). Thanks, Florian
On 30/12/24 14:06, Florian Weimer wrote: > * Adhemerval Zanella: > >> The require size for mmap can be inferred from __vasprintf return >> value. It also fixes tst-assert-2 when building with --enable-fortify, >> where even if the format is not translated, __readonly_area fails >> because malloc can not be used. >> >> Checked on aarch64-linux-gnu. >> --- >> assert/assert-perr.c | 2 +- >> assert/assert.c | 27 ++++++++++----------------- >> 2 files changed, 11 insertions(+), 18 deletions(-) >> >> diff --git a/assert/assert-perr.c b/assert/assert-perr.c >> index 0010cbc278..8a03620b4c 100644 >> --- a/assert/assert-perr.c >> +++ b/assert/assert-perr.c >> @@ -32,7 +32,7 @@ __assert_perror_fail (int errnum, >> char errbuf[1024]; >> >> char *e = __strerror_r (errnum, errbuf, sizeof errbuf); >> - __assert_fail_base (_("%s%s%s:%u: %s%sUnexpected error: %s.\n%n"), >> + __assert_fail_base (_("%s%s%s:%u: %s%sUnexpected error: %s.\n"), >> e, file, line, function); >> } >> libc_hidden_def (__assert_perror_fail) > > I think you have to update the translations in the same commit, > otherwise we'll end up with crashes due to %n leading to a write through > an uninitialized pointer. Right, I will do it on next version. > >> @@ -56,12 +49,12 @@ __assert_fail_base (const char *fmt, const char *assertion, const char *file, >> FATAL_PREPARE; >> #endif >> >> - int total; >> - if (__asprintf (&str, fmt, >> - __progname, __progname[0] ? ": " : "", >> - file, line, >> - function ? function : "", function ? ": " : "", >> - assertion, &total) >= 0) >> + int total = __asprintf (&str, fmt, >> + __progname, __progname[0] ? ": " : "", >> + file, line, >> + function ? function : "", function ? ": " : "", >> + assertion); >> + if (total >= 0) >> { >> /* Print the message. */ >> (void) __fxprintf (NULL, "%s", str); > > Somewhat unrelated: The code is rather odd. Why would we use mmap here > in addition to malloc? It doesn't matter that much anymore because > malloc's own asserts no longer use this code. Still I suppose we could > allocate a single patch and use __snprintf there. Anyway, this can wait > for a later cleanup. We should remove the use of __fxprintf, too, > although it won't be 100% backwards-compatible (for example, write the > message only if there is a file descriptor associated with the stderr > stream, so that we can write bytes directly without going through the > wide stream state). The patch that added mmap (f8a3b5bf8fa1d0c43d2458e03cc109a04fdef194) was not clear about the rationale, but my wild guess would be to have a direct way to get the abort message in a post-mortem analysis (instead to try dig it from malloc metadata). But it is not really clear to me the why we need to keep the __abort_msg, and if this is really useful. Also, the assert implementation now requires multiple allocation (the translation, the asprintf, and the mmap), which adds a lot of overhead and multiple point of failures that makes the interface moot (although mot likely memory allocation won't fail).
* Adhemerval Zanella Netto: > The patch that added mmap (f8a3b5bf8fa1d0c43d2458e03cc109a04fdef194) was > not clear about the rationale, but my wild guess would be to have a > direct way to get the abort message in a post-mortem analysis (instead > to try dig it from malloc metadata). Sure, but if we are using malloc anyway, we could just allocate it using malloc and write it to some global pointer? And perhaps we should just stop copying the message after the first assertion failure. I mean, if an application continues to run after an assertion failure with some SIGABRT handler, some degraded debugging functionality seems perfectly fine to me. Anyway, separate matter, unrelated to this patch. > But it is not really clear to me the why we need to keep the __abort_msg, > and if this is really useful. Also, the assert implementation now requires > multiple allocation (the translation, the asprintf, and the mmap), which > adds a lot of overhead and multiple point of failures that makes the interface > moot (although mot likely memory allocation won't fail). The issue is that the assert expression and file name won't end up in a core file if we don't make a copy. Copying it makes sense, but a (possibly truncated) on-stack copy seems perfectly sufficient to me. Thanks, Florian
On 30/12/24 17:00, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >> The patch that added mmap (f8a3b5bf8fa1d0c43d2458e03cc109a04fdef194) was >> not clear about the rationale, but my wild guess would be to have a >> direct way to get the abort message in a post-mortem analysis (instead >> to try dig it from malloc metadata). > > Sure, but if we are using malloc anyway, we could just allocate it using > malloc and write it to some global pointer? Ah right, you mean the double allocation (asnprint and then mmap). It is not clear to me why it is required as well. > > And perhaps we should just stop copying the message after the first > assertion failure. I mean, if an application continues to run after an > assertion failure with some SIGABRT handler, some degraded debugging > functionality seems perfectly fine to me. Sounds reasonable. > > Anyway, separate matter, unrelated to this patch. > >> But it is not really clear to me the why we need to keep the __abort_msg, >> and if this is really useful. Also, the assert implementation now requires >> multiple allocation (the translation, the asprintf, and the mmap), which >> adds a lot of overhead and multiple point of failures that makes the interface >> moot (although mot likely memory allocation won't fail). > > The issue is that the assert expression and file name won't end up in a > core file if we don't make a copy. Copying it makes sense, but a > (possibly truncated) on-stack copy seems perfectly sufficient to me. It would require a quite large stack (to accommodate PATH_MAX), so I am not sure it would be a improvement over mmap.
On Dez 30 2024, Florian Weimer wrote: > * Adhemerval Zanella: > >> The require size for mmap can be inferred from __vasprintf return >> value. It also fixes tst-assert-2 when building with --enable-fortify, >> where even if the format is not translated, __readonly_area fails >> because malloc can not be used. >> >> Checked on aarch64-linux-gnu. >> --- >> assert/assert-perr.c | 2 +- >> assert/assert.c | 27 ++++++++++----------------- >> 2 files changed, 11 insertions(+), 18 deletions(-) >> >> diff --git a/assert/assert-perr.c b/assert/assert-perr.c >> index 0010cbc278..8a03620b4c 100644 >> --- a/assert/assert-perr.c >> +++ b/assert/assert-perr.c >> @@ -32,7 +32,7 @@ __assert_perror_fail (int errnum, >> char errbuf[1024]; >> >> char *e = __strerror_r (errnum, errbuf, sizeof errbuf); >> - __assert_fail_base (_("%s%s%s:%u: %s%sUnexpected error: %s.\n%n"), >> + __assert_fail_base (_("%s%s%s:%u: %s%sUnexpected error: %s.\n"), >> e, file, line, function); >> } >> libc_hidden_def (__assert_perror_fail) > > I think you have to update the translations in the same commit, > otherwise we'll end up with crashes due to %n leading to a write through > an uninitialized pointer. Cannot happen, as there are no translations.
diff --git a/assert/assert-perr.c b/assert/assert-perr.c index 0010cbc278..8a03620b4c 100644 --- a/assert/assert-perr.c +++ b/assert/assert-perr.c @@ -32,7 +32,7 @@ __assert_perror_fail (int errnum, char errbuf[1024]; char *e = __strerror_r (errnum, errbuf, sizeof errbuf); - __assert_fail_base (_("%s%s%s:%u: %s%sUnexpected error: %s.\n%n"), + __assert_fail_base (_("%s%s%s:%u: %s%sUnexpected error: %s.\n"), e, file, line, function); } libc_hidden_def (__assert_perror_fail) diff --git a/assert/assert.c b/assert/assert.c index 3a16f85394..3834ddd864 100644 --- a/assert/assert.c +++ b/assert/assert.c @@ -15,24 +15,17 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ -#include <assert.h> -#include <atomic.h> +#include <intprops.h> #include <ldsodefs.h> #include <libintl.h> -#include <stdio.h> -#include <stdlib.h> -#include <sysdep.h> -#include <unistd.h> -#include <sys/mman.h> +#include <libio/iolibio.h> #include <setvmaname.h> #include <sys/uio.h> -#include <intprops.h> +#include <unistd.h> extern const char *__progname; -#include <wchar.h> -#include <libio/iolibio.h> #define fflush(s) _IO_fflush (s) /* This function, when passed a string containing an asserted @@ -56,12 +49,12 @@ __assert_fail_base (const char *fmt, const char *assertion, const char *file, FATAL_PREPARE; #endif - int total; - if (__asprintf (&str, fmt, - __progname, __progname[0] ? ": " : "", - file, line, - function ? function : "", function ? ": " : "", - assertion, &total) >= 0) + int total = __asprintf (&str, fmt, + __progname, __progname[0] ? ": " : "", + file, line, + function ? function : "", function ? ": " : "", + assertion); + if (total >= 0) { /* Print the message. */ (void) __fxprintf (NULL, "%s", str); @@ -129,6 +122,6 @@ void __assert_fail (const char *assertion, const char *file, unsigned int line, const char *function) { - __assert_fail_base (_("%s%s%s:%u: %s%sAssertion `%s' failed.\n%n"), + __assert_fail_base (_("%s%s%s:%u: %s%sAssertion `%s' failed.\n"), assertion, file, line, function); }