GNU bug report logs - #78762
[PATCH] Fix segfault in profiler-cpu-log and profiler-memory-log

Previous Next

Full log

Message #8 received at 78762 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Zach Shaftel <zach <at> shaf.tel>
Cc: 78762 <at> debbugs.gnu.org
Subject: Re: bug#78762: [PATCH] Fix segfault in profiler-cpu-log and
 profiler-memory-log
Date: Thu, 12 Jun 2025 08:03:10 +0300

> Date: Wed, 11 Jun 2025 16:03:12 -0400
> From:  Zach Shaftel via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> a simple NULL dereference fix. the profiler log would be null if
> profiler-*-log is called without a profiler-*-start before it, or if a
> previous profiler-*-log call had flushed the log and the profiler wasn't
> restarted since then. then export_log would try to read the NULL log and
> segfault.
> 
> to reproduce the bug:
> emacs --batch -f profiler-cpu-log
> 
> this patch causes profiler-cpu-log and profiler-memory-log to just
> return nil if the log is null.

Thanks, please see a few minor comments below.

> >From 2c1b3bc8b020acbef496dd991bd08bc0f5505528 Mon Sep 17 00:00:00 2001
> From: Zach Shaftel <zach <at> shaf.tel>
> Date: Wed, 11 Jun 2025 15:37:31 -0400
> Subject: [PATCH] Fix segfault in profiler-cpu-log and profiler-memory-log
> 
> * src/profiler.c (export_log): Check if a log has been allocated first,
> and return nil if it hasn't.
> (Fprofiler_cpu_log, Fprofiler_memory_log): Mention the possibly nil
> result.

The last sentence should say "...in the doc string."  Or just "Doc
fix" should be enough, since the nature of the change is clear from
the diffs.

Also, in the next revisions of the patch mention the bug number in the
commit log message, since you now know its number.

> @@ -534,7 +534,11 @@ DEFUN ("profiler-cpu-log", Fprofiler_cpu_log, Sprofiler_cpu_log,
>  The log is a hash-table mapping backtraces to counters which represent
>  the amount of time spent at those points.  Every backtrace is a vector
>  of functions, where the last few elements may be nil.
> -Before returning, a new log is allocated for future samples.  */)
> +
> +If the profiler has not run since the last invocation of
> +`profiler-cpu-log' (or was never run at all), nil is returned.  If the
> +profiler is currently running, a new log is allocated for future samples
> +before returning.  */)

Please avoid passive tense as much as possible.  Here, "return nil"
and "allocate a new log" is shorter and more clear.

> +  if (plog->log == NULL)

We usually prefer the C style:

     if (!plog->log)

> +    /* We haven't collected any new data, so return nil.  */
> +    return Qnil;

No need to add comments that just describe what the code already says.
Comments which explain what the code does should tell something that
isn't clear by just looking at the code.

> @@ -639,7 +646,11 @@ DEFUN ("profiler-memory-log",
>  The log is a hash-table mapping backtraces to counters which represent
>  the amount of memory allocated at those points.  Every backtrace is a vector
>  of functions, where the last few elements may be nil.
> -Before returning, a new log is allocated for future samples.  */)
> +
> +If the profiler has not run since the last invocation of
> +`profiler-memory-log' (or was never run at all), nil is returned.  If
> +the profiler is currently running, a new log is allocated for future
> +samples before returning.  */)

Same comment as above for the other doc string.

Also, can you add a test for this?

Finally, the same problem exists on the emacs-30 release branch, so
could you please post your next revision of the patch relative to the
release branch?

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.