GNU bug report logs - #75969
Buffer overflow in line numbering code

Previous Next

To reply to this bug, email your comments to 75969 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

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

From: Pip Cet <pipcet <at> protonmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Buffer overflow in line numbering code
Date: Fri, 31 Jan 2025 15:02:40 +0000

This code currently crashes the master branch:

gdb --ar ./src/emacs -Q --eval '(setq display-line-numbers t)' --eval '(setq display-line-numbers-width 1024)'

Looking at the code, there's no reason it wouldn't.

Applying this tiny patch:

From bfcd8d83faa7c75173211cb36039360dfbb9c76f Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet <at> protonmail.com>
Subject: [PATCH] Fix buffer overflow in maybe_produce_line_number (bug#tbd)

* src/xdisp.c (maybe_produce_line_number): Allocate 'lnum_buf' with
'SAFE_ALLOCA', not directly on the stack.  Allocate enough bytes.
SAFE_FREE when done.
---
 src/xdisp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index a801caae06f..9f0e6578d01 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -25006,7 +25006,6 @@ maybe_produce_line_number (struct it *it)
 
   /* Produce the glyphs for the line number.  */
   struct it tem_it;
-  char lnum_buf[INT_STRLEN_BOUND (ptrdiff_t) + 1];
   bool beyond_zv = IT_BYTEPOS (*it) >= ZV_BYTE;
   ptrdiff_t lnum_offset = -1; /* to produce 1-based line numbers */
   int lnum_face_id = merge_faces (it->w, Qline_number, 0, DEFAULT_FACE_ID);
@@ -25075,6 +25074,8 @@ maybe_produce_line_number (struct it *it)
      called when no display elements were produced from the
      following line, so the paragraph direction might be unknown.
      Therefore we cheat and add 2 blanks, one on either side.  */
+  USE_SAFE_ALLOCA;
+  char *lnum_buf = SAFE_ALLOCA (it->lnum_width + 1);
   pint2str (lnum_buf, it->lnum_width + 1, lnum_to_display);
   strcat (lnum_buf, " ");
 
@@ -25146,6 +25147,7 @@ maybe_produce_line_number (struct it *it)
 	  it->lnum_pixel_width = 0;
 	  bidi_unshelve_cache (itdata, false);
 	  inhibit_free_realized_faces = save_free_realized_faces;
+	  SAFE_FREE ();
 	  return;
 	}
     }
@@ -25206,6 +25208,7 @@ maybe_produce_line_number (struct it *it)
   it->line_number_produced_p = true;
 
   bidi_unshelve_cache (itdata, false);
+  SAFE_FREE ();
 }
 
 /* Return true if this glyph row needs a line number to be produced
-- 
2.48.1

fixes things.  But now we've already fixed doprnt to be useful enough
for make_formatted_string, we might as well get rid of pint2str and
pint2hrstr entirely.

As far as I can tell, pint2str (buf, width, d) is equivalent to
sprintf (buf, "% *d", width, d).  I'm not so sure about pint2hrstr
because when something is said to be human-readable that usually
excludes me.

Pip

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 75969 <at> debbugs.gnu.org
Subject: Re: bug#75969: Buffer overflow in line numbering code
Date: Fri, 31 Jan 2025 17:22:57 +0200

> Date: Fri, 31 Jan 2025 15:02:40 +0000
> From:  Pip Cet via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> This code currently crashes the master branch:
> 
> gdb --ar ./src/emacs -Q --eval '(setq display-line-numbers t)' --eval '(setq display-line-numbers-width 1024)'
> 
> Looking at the code, there's no reason it wouldn't.

I'd prefer to limit the actual value used to
INT_STRLEN_BOUND(ptrdiff_t).  There should be no reason to use larger
values.

> As far as I can tell, pint2str (buf, width, d) is equivalent to
> sprintf (buf, "% *d", width, d).  I'm not so sure about pint2hrstr
> because when something is said to be human-readable that usually
> excludes me.

sprintf can call malloc, whereas pint2str doesn't.  It is also much
more expensive.  What are the problems with pint2str that we'd want to
remove it?

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

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 75969 <at> debbugs.gnu.org
Subject: Re: bug#75969: Buffer overflow in line numbering code
Date: Fri, 31 Jan 2025 16:22:49 +0000

"Eli Zaretskii" <eliz <at> gnu.org> writes:

>> Date: Fri, 31 Jan 2025 15:02:40 +0000
>> From:  Pip Cet via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>>
>> This code currently crashes the master branch:
>>
>> gdb --ar ./src/emacs -Q --eval '(setq display-line-numbers t)' --eval '(setq display-line-numbers-width 1024)'
>>
>> Looking at the code, there's no reason it wouldn't.
>
> I'd prefer to limit the actual value used to
> INT_STRLEN_BOUND(ptrdiff_t).  There should be no reason to use larger
> values.

I wouldn't mind a fixed limit, but making it depend on the word size of
the machine seems problematic to me.

>> As far as I can tell, pint2str (buf, width, d) is equivalent to
>> sprintf (buf, "% *d", width, d).  I'm not so sure about pint2hrstr
>> because when something is said to be human-readable that usually
>> excludes me.
>
> sprintf can call malloc, whereas pint2str doesn't.

Is there a reason we shouldn't call malloc here (most sprintf
implementations probably wouldn't, but SAFE_ALLOCA might)?

> It is also much more expensive.

Which one?  pint2str creates a reversed number in memory, appends a
number of spaces, then flips the entire buffer in place to finally
arrive at the desired string.

And, oops, afterwards, it strcats another space to the whole thing, most
likely iterating over the buffer again, and extending the buffer size by
another byte I may have failed to account for.  sprintf may be faster,
yes, but it's also safer.

> What are the problems with pint2str

It caused a crashable buffer overflow which escaped scrutiny because
people careful enough to check sprintf where it appears didn't react in
the same way to "pint2str".

And while interesting, the algorithm it uses is quite strange.

Most importantly, we need all the compiler warnings we can get, and
sprintf has good compiler warnings while pint2str doesn't.

>  that we'd want to remove it?

I did not propose to remove it, at this point.

(Of course I'd love to see it gone and replaced by standard C formatted
I/O).

Pip

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 75969 <at> debbugs.gnu.org
Subject: Re: bug#75969: Buffer overflow in line numbering code
Date: Fri, 31 Jan 2025 19:03:10 +0200

> Date: Fri, 31 Jan 2025 16:22:49 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: 75969 <at> debbugs.gnu.org
> 
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
> 
> > I'd prefer to limit the actual value used to
> > INT_STRLEN_BOUND(ptrdiff_t).  There should be no reason to use larger
> > values.
> 
> I wouldn't mind a fixed limit, but making it depend on the word size of
> the machine seems problematic to me.

Why? it's a natural limitation, not an arbitrary one.

> >> As far as I can tell, pint2str (buf, width, d) is equivalent to
> >> sprintf (buf, "% *d", width, d).  I'm not so sure about pint2hrstr
> >> because when something is said to be human-readable that usually
> >> excludes me.
> >
> > sprintf can call malloc, whereas pint2str doesn't.
> 
> Is there a reason we shouldn't call malloc here (most sprintf
> implementations probably wouldn't, but SAFE_ALLOCA might)?

Maybe not today, but in the past redisplay could be triggered
asynchronously, from a signal handler, and malloc was not reentrant.

> > It is also much more expensive.
> 
> Which one?

sprintf.

> pint2str creates a reversed number in memory, appends a
> number of spaces, then flips the entire buffer in place to finally
> arrive at the desired string.

Yes, but formatted output is orders of magnitude more complex.

> > What are the problems with pint2str
> 
> It caused a crashable buffer overflow which escaped scrutiny because
> people careful enough to check sprintf where it appears didn't react in
> the same way to "pint2str".

AFAIU, the crash was because the buffer was smaller than the length of
the string we were trying to produce.  Fixing that will prevent
crashes.

IOW, it isn't that pint2str, it's the too-large value of
display-line-numbers-width.

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

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 75969 <at> debbugs.gnu.org
Subject: Re: bug#75969: Buffer overflow in line numbering code
Date: Fri, 31 Jan 2025 17:25:16 +0000

"Eli Zaretskii" <eliz <at> gnu.org> writes:

>> It caused a crashable buffer overflow which escaped scrutiny because
>> people careful enough to check sprintf where it appears didn't react in
>> the same way to "pint2str".
>
> AFAIU, the crash was because the buffer was smaller than the length of
> the string we were trying to produce.
> Fixing that will prevent crashes.

I don't think we're disagreeing about any of that.  Again, I'm not
proposing the function be removed, just that its buffer is increased by
one more byte than what my proposed patch did.

That I'll continue to consider the function problematic isn't of any
relevance to Emacs.

> IOW, it isn't that pint2str, it's the too-large value of
> display-line-numbers-width.

Claming that in a machine-independent way seems fine by me, just let me
know which size you prefer.  My proposal would be the intmax_t bounds
because when those change, a whole lot of Emacs code will need reviewing
anyway.

Pip

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 75969 <at> debbugs.gnu.org
Subject: Re: bug#75969: Buffer overflow in line numbering code
Date: Fri, 31 Jan 2025 20:49:31 +0200

> Date: Fri, 31 Jan 2025 17:25:16 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: 75969 <at> debbugs.gnu.org
> 
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
> 
> > IOW, it isn't that pint2str, it's the too-large value of
> > display-line-numbers-width.
> 
> Claming that in a machine-independent way seems fine by me, just let me
> know which size you prefer.  My proposal would be the intmax_t bounds
> because when those change, a whole lot of Emacs code will need reviewing
> anyway.

It's okay to allow intmax_t, but in any case the limitation should be
in sync with the dimension of the buffer.

Previous Next

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