GNU bug report logs - #51105
29.0.50; Buffer overflow bug in ns_compute_glyph_string_overhangs

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 51105 in the body.
You can then email your comments to 51105 AT debbugs.gnu.org in the normal way.

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

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

From: Daniel Martín <mardani29 <at> yahoo.es>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; Buffer overflow bug in ns_compute_glyph_string_overhangs
Date: Sat, 09 Oct 2021 02:30:33 +0200

There is a buffer overflow bug in the function
ns_compute_glyph_string_overhangs with some particular information
received from the display engine.

(I haven't reduced the test case yet so you may not reproduce the
issue with the following recipe.)

  emacs -Q

Attach a debugger to the Emacs process and add the following
conditional breakpoint:

  br set -f nsterm.m -l 2853 -c 's->nchars==0'

Continue running Emacs

  M-x eww RET wikipedia.org RET

The debugger will stop with the following backtrace:

* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x000000010e25a20e emacs`ns_compute_glyph_string_overhangs(s=0x00007ffee232ef40) at nsterm.m:2853:7
    frame #1: 0x000000010da4cbdf emacs`draw_glyphs(w=0x00006210000ac130, x=66, row=0x000062b00029ae00, area=TEXT_AREA, start=0, end=12, hl=DRAW_NORMAL_TEXT, overlaps=0) at xdisp.c:29036:4
    frame #2: 0x000000010da49bd0 emacs`gui_write_glyphs(w=0x00006210000ac130, updated_row=0x000062b00029ae00, start=0x0000629001be4200, updated_area=TEXT_AREA, len=12) at xdisp.c:31179:7
    frame #3: 0x000000010d90bc4d emacs`update_text_area(w=0x00006210000ac130, updated_row=0x000062b00029ae00, vpos=28) at dispnew.c:3934:2
    frame #4: 0x000000010d902191 emacs`update_window_line(w=0x00006210000ac130, vpos=28, mouse_face_overwritten_p=0x00007ffee2331720) at dispnew.c:4177:11
    frame #5: 0x000000010d8d84f7 emacs`update_window(w=0x00006210000ac130, force_p=true) at dispnew.c:3680:19
    frame #6: 0x000000010d8d9bbc emacs`update_window_tree(w=0x00006210000ac130, force_p=true) at dispnew.c:3405:14
    frame #7: 0x000000010d8d67e6 emacs`update_frame(f=0x00006210000ad530, force_p=true, inhibit_hairy_id_p=false) at dispnew.c:3240:18
    frame #8: 0x000000010d9db568 emacs`redisplay_internal at xdisp.c:16160:16
    frame #9: 0x000000010d9eb0a9 emacs`redisplay_preserve_echo_area(from_where=12) at xdisp.c:16429:7
    frame #10: 0x000000010e0cb8e1 emacs`wait_reading_process_output(time_limit=0, nsecs=0, read_kbd=-1, do_display=true, wait_for_cell=0x0000000000000000, wait_proc=0x0000000000000000, just_wait_proc=0) at process.c:5789:7
    frame #11: 0x000000010dd99c82 emacs`kbd_buffer_get_event(kbp=0x00007ffee23371c0, used_mouse_menu=0x00007ffee23386c0, end_time=0x0000000000000000) at keyboard.c:3924:4
    frame #12: 0x000000010dd9825e emacs`read_event_from_main_queue(end_time=0x0000000000000000, local_getcjmp=0x00007ffee2338300, used_mouse_menu=0x00007ffee23386c0) at keyboard.c:2198:7
    frame #13: 0x000000010dd6a19a emacs`read_decoded_event_from_main_queue(end_time=0x0000000000000000, local_getcjmp=0x00007ffee2338300, prev_event=0x0000000000000000, used_mouse_menu=0x00007ffee23386c0) at keyboard.c:2262:11
    frame #14: 0x000000010dd632c8 emacs`read_char(commandflag=1, map=0x00006290003eb8a3, prev_event=0x0000000000000000, used_mouse_menu=0x00007ffee23386c0, end_time=0x0000000000000000) at keyboard.c:2892:11
    frame #15: 0x000000010dd58e1d emacs`read_key_sequence(keybuf=0x00007ffee23393a0, prompt=0x0000000000000000, dont_downcase_last=false, can_return_switch_frame=true, fix_current_buffer=true, prevent_redisplay=false) at keyboard.c:9619:12
    frame #16: 0x000000010dd539f3 emacs`command_loop_1 at keyboard.c:1392:15
    frame #17: 0x000000010dfa45d9 emacs`internal_condition_case(bfun=(emacs`command_loop_1 at keyboard.c:1278), handlers=0x0000000000000090, hfun=(emacs`cmd_error at keyboard.c:936)) at eval.c:1453:25
    frame #18: 0x000000010dd52903 emacs`command_loop_2(handlers=0x0000000000000090) at keyboard.c:1133:11
    frame #19: 0x000000010dfa2ff9 emacs`internal_catch(tag=0x000000000000df80, func=(emacs`command_loop_2 at keyboard.c:1129), arg=0x0000000000000090) at eval.c:1184:25
    frame #20: 0x000000010dd50f81 emacs`command_loop at keyboard.c:1111:2
    frame #21: 0x000000010dd50c9b emacs`recursive_edit_1 at keyboard.c:720:9
    frame #22: 0x000000010dd5147a emacs`Frecursive_edit at keyboard.c:803:3
    frame #23: 0x000000010dd4a05a emacs`main(argc=2, argv=0x00007ffee233a310) at emacs.c:2310:3
    frame #24: 0x00007fff20496f3d libdyld.dylib`start + 1

This line in nsterm.m will be executed and is problematic:

  codes[1] = *(s->char2b + s->nchars - 1);

When s->nchars is 0, the code will reference one position before
s->char2b.

I have two questions:

1) Is there any reason the function chooses the first and last glyphs
instead of passing the whole glyph string and rely on text_extents to
perfom boundary checks? That is, I propose:

diff --git a/src/nsterm.m b/src/nsterm.m
index a6c2e7505b..207da60481 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2853,11 +2853,7 @@ Hide the window (X11 semantics)
   if (s->char2b)
     {
       struct font_metrics metrics;
-      unsigned int codes[2];
-      codes[0] = *(s->char2b);
-      codes[1] = *(s->char2b + s->nchars - 1);
-
-      font->driver->text_extents (font, codes, 2, &metrics);
+      font->driver->text_extents (font, s->char2b, s->nchars, &metrics);
       s->left_overhang = -metrics.lbearing;
       s->right_overhang
 	= metrics.rbearing > metrics.width

This way to call the text_extents API is also implemented in w32term.c
and xterm.c.

2) The root cause of the issue may be that s->nchars is 0 when it
shouldn't.  Is there any legitimate scenario where the display engine
may call this routine with s->nchars equal to 0? If so, what are those
situations?


In GNU Emacs 29.0.50 (build 1, x86_64-apple-darwin20.6.0, NS appkit-2022.60 Version 11.6 (Build 20G165))
 of 2021-10-09 built on Daniels-MacBook-Pro.local
Repository revision: 36d7c4af7c83c4f3ea9ab9fdd0822b986564d78e
Repository branch: master
Windowing system distributor 'Apple', version 10.3.2022
System Description:  macOS 11.6

Configured using:
 'configure 'CFLAGS=-O0 -g3''

Configured features:
ACL DBUS GIF GLIB GMP GNUTLS JPEG JSON LCMS2 LIBXML2 MODULES NOTIFY
KQUEUE NS PDUMPER PNG RSVG THREADS TIFF TOOLKIT_SCROLL_BARS XIM ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg rfc6068 epg-config gnus-util rmail
rmail-loaddefs auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json map text-property-search time-date
seq gv subr-x byte-opt bytecomp byte-compile cconv mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs
cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
iso-transl tooltip eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel term/ns-win ns-win ucs-normalize
mule-util term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice button
loaddefs faces cus-face macroexp files window text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads dbusbind kqueue cocoa ns
lcms2 multi-tty make-network-process emacs)

Memory information:
((conses 16 49678 8809)
 (symbols 48 6572 1)
 (strings 32 17870 1691)
 (string-bytes 1 591830)
 (vectors 16 12905)
 (vector-slots 8 177066 9811)
 (floats 8 21 51)
 (intervals 56 191 0)
 (buffers 992 10))

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Daniel Martín <mardani29 <at> yahoo.es>
Cc: 51105 <at> debbugs.gnu.org
Subject: Re: bug#51105: 29.0.50;
 Buffer overflow bug in ns_compute_glyph_string_overhangs
Date: Sat, 09 Oct 2021 09:40:09 +0300

> Date: Sat, 09 Oct 2021 02:30:33 +0200
> From:  Daniel Martín via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> 2) The root cause of the issue may be that s->nchars is 0 when it
> shouldn't.  Is there any legitimate scenario where the display engine
> may call this routine with s->nchars equal to 0? If so, what are those
> situations?

I think if the glyph string has composition glyphs, nchars can be
zero.  What is the value of s->first_glyph->type in the case where it
happens?

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

From: Daniel Martín <mardani29 <at> yahoo.es>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 51105 <at> debbugs.gnu.org
Subject: Re: bug#51105: 29.0.50; Buffer overflow bug in
 ns_compute_glyph_string_overhangs
Date: Sat, 09 Oct 2021 12:06:36 +0200

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

>> Date: Sat, 09 Oct 2021 02:30:33 +0200
>> From:  Daniel Martín via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>> 
>> 2) The root cause of the issue may be that s->nchars is 0 when it
>> shouldn't.  Is there any legitimate scenario where the display engine
>> may call this routine with s->nchars equal to 0? If so, what are those
>> situations?
>
> I think if the glyph string has composition glyphs, nchars can be
> zero.  What is the value of s->first_glyph->type in the case where it
> happens?

Yep, it seems so:

(lldb) fr v s->first_glyph->type
(unsigned int:3) s->first_glyph->type = 1

I've found a 2006 commit that seemed to handle this particular pointer
arithmetic logic for when the type of the first glyph is STRETCH_GLYPH:
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=825de9a1027073beaec38ab1572e9d954f8a1eb0

Now I think that the right thing to do may be to modify nsterm.m, switch
on the glyph type and, if the glyph type is COMPOSITE_GLYPH, call
composition_gstring_width to get the glyph metrics.  Function
composition_gstring_width uses the values from fields s->cmp_from and
s->cmp_to, and would avoid the buffer overflow:

(lldb) fr v s->cmp_from
(int) s->cmp_from = 6
(lldb) fr v s->cmp_to
(int) s->cmp_to = 7

WDYT? I can prepare a patch of this type if you agree.

I'll try to get the sequence of codepoints from the glyph string in the
debugger, so we can have a reduced test case (ie. the exact string from
the Wikipedia's front page that causes the issue).

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Daniel Martín <mardani29 <at> yahoo.es>, Alan Third
 <alan <at> idiocy.org>
Cc: 51105 <at> debbugs.gnu.org
Subject: Re: bug#51105: 29.0.50; Buffer overflow bug in
 ns_compute_glyph_string_overhangs
Date: Sat, 09 Oct 2021 14:43:18 +0300

> From: Daniel Martín <mardani29 <at> yahoo.es>
> Cc: 51105 <at> debbugs.gnu.org
> Date: Sat, 09 Oct 2021 12:06:36 +0200
> 
> Now I think that the right thing to do may be to modify nsterm.m, switch
> on the glyph type and, if the glyph type is COMPOSITE_GLYPH, call
> composition_gstring_width to get the glyph metrics.  Function
> composition_gstring_width uses the values from fields s->cmp_from and
> s->cmp_to, and would avoid the buffer overflow:
> 
> (lldb) fr v s->cmp_from
> (int) s->cmp_from = 6
> (lldb) fr v s->cmp_to
> (int) s->cmp_to = 7
> 
> WDYT? I can prepare a patch of this type if you agree.

SGTM, but I'd like to hear Alan's opinion as well, as I don't feel I
know enough about the NS display backend.

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

From: Alan Third <alan <at> idiocy.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 51105 <at> debbugs.gnu.org,
 Daniel Martín <mardani29 <at> yahoo.es>
Subject: Re: bug#51105: 29.0.50; Buffer overflow bug in
 ns_compute_glyph_string_overhangs
Date: Sat, 9 Oct 2021 14:57:40 +0100

On Sat, Oct 09, 2021 at 02:43:18PM +0300, Eli Zaretskii wrote:
> > From: Daniel Martín <mardani29 <at> yahoo.es>
> > Cc: 51105 <at> debbugs.gnu.org
> > Date: Sat, 09 Oct 2021 12:06:36 +0200
> > 
> > Now I think that the right thing to do may be to modify nsterm.m, switch
> > on the glyph type and, if the glyph type is COMPOSITE_GLYPH, call
> > composition_gstring_width to get the glyph metrics.  Function
> > composition_gstring_width uses the values from fields s->cmp_from and
> > s->cmp_to, and would avoid the buffer overflow:
> > 
> > (lldb) fr v s->cmp_from
> > (int) s->cmp_from = 6
> > (lldb) fr v s->cmp_to
> > (int) s->cmp_to = 7
> > 
> > WDYT? I can prepare a patch of this type if you agree.
> 
> SGTM, but I'd like to hear Alan's opinion as well, as I don't feel I
> know enough about the NS display backend.

I don't know much about this part of the code, but it sounds good to
me too.
-- 
Alan Third

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

From: Daniel Martín <mardani29 <at> yahoo.es>
To: Alan Third <alan <at> idiocy.org>
Cc: 51105 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#51105: 29.0.50; Buffer overflow bug in
 ns_compute_glyph_string_overhangs
Date: Sat, 09 Oct 2021 21:35:22 +0200

[Message part 1 (text/plain, inline)]

Alan Third <alan <at> idiocy.org> writes:

> On Sat, Oct 09, 2021 at 02:43:18PM +0300, Eli Zaretskii wrote:
>> > From: Daniel Martín <mardani29 <at> yahoo.es>
>> > Cc: 51105 <at> debbugs.gnu.org
>> > Date: Sat, 09 Oct 2021 12:06:36 +0200
>> > 
>> > Now I think that the right thing to do may be to modify nsterm.m, switch
>> > on the glyph type and, if the glyph type is COMPOSITE_GLYPH, call
>> > composition_gstring_width to get the glyph metrics.  Function
>> > composition_gstring_width uses the values from fields s->cmp_from and
>> > s->cmp_to, and would avoid the buffer overflow:
>> > 
>> > (lldb) fr v s->cmp_from
>> > (int) s->cmp_from = 6
>> > (lldb) fr v s->cmp_to
>> > (int) s->cmp_to = 7
>> > 
>> > WDYT? I can prepare a patch of this type if you agree.
>> 
>> SGTM, but I'd like to hear Alan's opinion as well, as I don't feel I
>> know enough about the NS display backend.
>
> I don't know much about this part of the code, but it sounds good to
> me too.

A reduced test case to reproduce the problem is to paste "العربية" in the
*scratch* buffer.

I've attached a patch that fixes the issue.

[0001-Fix-buffer-overflow-in-ns_compute_glyph_string_overh.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

Let me know if you like it and please install it on my behalf if so.
Thanks.

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

From: Daniel Martín <mardani29 <at> yahoo.es>
To: Daniel Martín via "Bug reports for GNU Emacs, the Swiss
 army knife of text editors" <bug-gnu-emacs <at> gnu.org>
Cc: 51105 <at> debbugs.gnu.org, Alan Third <alan <at> idiocy.org>,
 Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#51105: 29.0.50; Buffer overflow bug in
 ns_compute_glyph_string_overhangs
Date: Sat, 09 Oct 2021 21:41:57 +0200

[Message part 1 (text/plain, inline)]

Daniel Martín via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs <at> gnu.org> writes:

>
> A reduced test case to reproduce the problem is to paste "العربية" in the
> *scratch* buffer.
>
> I've attached a patch that fixes the issue.
>
>
>
> Let me know if you like it and please install it on my behalf if so.
> Thanks.

Sorry, there was an indentation problem in the previous patch.  Here's
an updated one.

[0001-Fix-buffer-overflow-in-ns_compute_glyph_string_overh.patch (text/x-patch, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Daniel Martín <mardani29 <at> yahoo.es>
Cc: 51105 <at> debbugs.gnu.org, alan <at> idiocy.org, eliz <at> gnu.org
Subject: Re: bug#51105: 29.0.50; Buffer overflow bug in
 ns_compute_glyph_string_overhangs
Date: Fri, 05 Nov 2021 03:39:01 +0100

Daniel Martín <mardani29 <at> yahoo.es> writes:

> Sorry, there was an indentation problem in the previous patch.  Here's
> an updated one.

It seemed like Alan agreed with the fix, and I tested it now on my M1
Apple laptop, and it didn't break anything obvious, so I've now pushed
Daniel's patch to the trunk.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

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