SIGSEGV in libedit

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

SIGSEGV in libedit

YASUOKA Masahiko-3
Hi,

Programs using libedit(3) crashe after the program's window size is
changed.  For example,

  1. Invoke ftp
     $ ftp
     ftp>
  2. Resize its window
  3. Enter "deb" + <Tab>

     => When the problem occurs, it crashes with a segmentation fault
     => The problem doesn't occur, it displays "debug"

The problem happens once in 5-30 times.

See the problem by gdb.

  Program terminated with signal SIGSEGV, Segmentation fault.
  #0  0x0000162ce7bd3ff2 in re_update_line (el=0x162cfa59d800,
      old=0x162c3e478e00 L"deb", '\xdfdfdfdf' <repeats 125 times><error: Cannot access memory at address 0x162c3e479000>, new=0x162c3854ae00 L"ftp> debug", i=0)
      at /home/yasuoka/src/lib/libedit/refresh.c:518
  518             while (*o)
  (gdb) bt
  #0  0x0000162ce7bd3ff2 in re_update_line (el=0x162cfa59d800,
      old=0x162c3e478e00 L"deb", '\xdfdfdfdf' <repeats 125 times><error: Cannot access memory at address 0x162c3e479000>, new=0x162c3854ae00 L"ftp> debug", i=0)
      at /home/yasuoka/src/lib/libedit/refresh.c:518
  #1  0x0000162ce7bd3c99 in re_refresh (el=0x162cfa59d800) at /home/yasuoka/src/lib/libedit/refresh.c:298
  #2  0x0000162ce7beba1c in el_wgets (el=0x162cfa59d800, nread=0x7f7ffffe1c6c) at /home/yasuoka/src/lib/libedit/read.c:577
  #3  0x0000162ce7be700a in el_gets (el=0x162cfa59d800, nread=0x7f7ffffe1c6c) at /home/yasuoka/src/lib/libedit/eln.c:74
  #4  0x0000162a32a19c60 in ?? ()
  #5  0x0000162a32a19817 in ?? ()
  #6  0x0000162a32a0b142 in ?? ()
  #7  0x0000000000000000 in ?? ()
  (gdb) p el->el_display[0]
  $1 = 0x162c3e478e00 L"deb", '\xdfdfdfdf' <repeats 125 times><error: Cannot access memory at address 0x162c3e479000>
  (gdb)

When <tab> is entered, as the result of the completion, libedit calls
re_update_line() <= re_reresh() to refresh the line.  In that
function, it tries to find a NUL char in the line buffer, but the line
buffer doesn't include any NUL, then the crash happens.

After the window size change, the line buffer is freeed and allocated
again at terminal_alloc_display().  (These are done by signal
handler.  This is a separated problem)

 409 static int
 410 terminal_alloc_display(EditLine *el)
 411 {
 412         int i;
 413         wchar_t **b;
 414         coord_t *c = &el->el_terminal.t_size;
 415
 416         b = reallocarray(NULL, c->v + 1, sizeof(*b));
 417         if (b == NULL)
 418                 goto done;
 419         for (i = 0; i < c->v; i++) {
 420                 b[i] = reallocarray(NULL, c->h + 1, sizeof(**b));
 421                 if (b[i] == NULL) {
 422                         while (--i >= 0)
 423                                 free(b[i]);
 424                         free(b);
 425                         goto done;
 426                 }
 427         }
 428         b[c->v] = NULL;
 429         el->el_display = b;

The line buffers are el->el_display[] and they are allocated by
reallocarray().  Then they are initialized at re_clear_display().

1149 protected void
1150 re_clear_display(EditLine *el)
1151 {
1152         int i;
1153        
1154         el->el_cursor.v = 0;
1155         el->el_cursor.h = 0;
1156         for (i = 0; i < el->el_terminal.t_size.v; i++)
1157                 el->el_display[i][0] = '\0';
1158         el->el_refresh.r_oldcv = 0;
1159 }

Remark that only first char is set NUL, but the rest part is kept
uninitialized.  Then user inputs "deb".  re_fastputc() is called for
each char.

1053 re_fastputc(EditLine *el, wint_t c)
1054 {
1055         wchar_t *lastline;
1056         int w;
1057
1058         w = wcwidth(c);
1059         while (w > 1 && el->el_cursor.h + w > el->el_terminal.t_size.h)
1060             re_fastputc(el, ' ');
1061
1062         terminal__putc(el, c);
1063         el->el_display[el->el_cursor.v][el->el_cursor.h++] = c;

the function just put the char like #1063, then the buffer becomes it
doesn't include any NUL char like below.

  deb + <uninitialized>

After this, by calling re_reresh() the crash happens.

I had looked into the code deeply, but it didn't become clear how
should we fix this problem.  But I suppose the code assumes the entire
buffer is initialized, so the following diff replaces the code which
initializes only first char by the code which calls
re__copy_and_pad().

comment?
ok?

Index: lib/libedit/refresh.c
===================================================================
RCS file: /var/cvs/openbsd/src/lib/libedit/refresh.c,v
retrieving revision 1.22
diff -u -p -r1.22 refresh.c
--- lib/libedit/refresh.c 11 Oct 2018 15:19:09 -0000 1.22
+++ lib/libedit/refresh.c 1 Aug 2019 04:55:34 -0000
@@ -320,7 +320,8 @@ re_refresh(EditLine *el)
 #ifdef DEBUG_REFRESH
  terminal_overwrite(el, L"C\b", 2);
 #endif /* DEBUG_REFRESH */
- el->el_display[i][0] = '\0';
+ re__copy_and_pad(el->el_display[i], L"",
+    (size_t) el->el_terminal.t_size.h);
  }
 
  el->el_refresh.r_oldcv = el->el_refresh.r_newcv; /* set for next time */
@@ -1154,7 +1155,8 @@ re_clear_display(EditLine *el)
  el->el_cursor.v = 0;
  el->el_cursor.h = 0;
  for (i = 0; i < el->el_terminal.t_size.v; i++)
- el->el_display[i][0] = '\0';
+ re__copy_and_pad(el->el_display[i], L"",
+    (size_t)el->el_terminal.t_size.h);
  el->el_refresh.r_oldcv = 0;
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: SIGSEGV in libedit

YASUOKA Masahiko-3
Hi,

I noticed the upstream NetBSD recently replaced almost all malloc(3)s
by calloc(3) in libedit.

  https://github.com/NetBSD/src/commit/b91b3c48e0edb116bd797586430cb426b575d717

This also fixes the problem.  I'll create a diff which does the same
thing.

On Thu, 01 Aug 2019 14:54:20 +0900 (JST)
YASUOKA Masahiko <[hidden email]> wrote:

> Hi,
>
> Programs using libedit(3) crashe after the program's window size is
> changed.  For example,
>
>   1. Invoke ftp
>      $ ftp
>      ftp>
>   2. Resize its window
>   3. Enter "deb" + <Tab>
>
>      => When the problem occurs, it crashes with a segmentation fault
>      => The problem doesn't occur, it displays "debug"
>
> The problem happens once in 5-30 times.
>
> See the problem by gdb.
>
>   Program terminated with signal SIGSEGV, Segmentation fault.
>   #0  0x0000162ce7bd3ff2 in re_update_line (el=0x162cfa59d800,
>       old=0x162c3e478e00 L"deb", '\xdfdfdfdf' <repeats 125 times><error: Cannot access memory at address 0x162c3e479000>, new=0x162c3854ae00 L"ftp> debug", i=0)
>       at /home/yasuoka/src/lib/libedit/refresh.c:518
>   518             while (*o)
>   (gdb) bt
>   #0  0x0000162ce7bd3ff2 in re_update_line (el=0x162cfa59d800,
>       old=0x162c3e478e00 L"deb", '\xdfdfdfdf' <repeats 125 times><error: Cannot access memory at address 0x162c3e479000>, new=0x162c3854ae00 L"ftp> debug", i=0)
>       at /home/yasuoka/src/lib/libedit/refresh.c:518
>   #1  0x0000162ce7bd3c99 in re_refresh (el=0x162cfa59d800) at /home/yasuoka/src/lib/libedit/refresh.c:298
>   #2  0x0000162ce7beba1c in el_wgets (el=0x162cfa59d800, nread=0x7f7ffffe1c6c) at /home/yasuoka/src/lib/libedit/read.c:577
>   #3  0x0000162ce7be700a in el_gets (el=0x162cfa59d800, nread=0x7f7ffffe1c6c) at /home/yasuoka/src/lib/libedit/eln.c:74
>   #4  0x0000162a32a19c60 in ?? ()
>   #5  0x0000162a32a19817 in ?? ()
>   #6  0x0000162a32a0b142 in ?? ()
>   #7  0x0000000000000000 in ?? ()
>   (gdb) p el->el_display[0]
>   $1 = 0x162c3e478e00 L"deb", '\xdfdfdfdf' <repeats 125 times><error: Cannot access memory at address 0x162c3e479000>
>   (gdb)
>
> When <tab> is entered, as the result of the completion, libedit calls
> re_update_line() <= re_reresh() to refresh the line.  In that
> function, it tries to find a NUL char in the line buffer, but the line
> buffer doesn't include any NUL, then the crash happens.
>
> After the window size change, the line buffer is freeed and allocated
> again at terminal_alloc_display().  (These are done by signal
> handler.  This is a separated problem)
>
>  409 static int
>  410 terminal_alloc_display(EditLine *el)
>  411 {
>  412         int i;
>  413         wchar_t **b;
>  414         coord_t *c = &el->el_terminal.t_size;
>  415
>  416         b = reallocarray(NULL, c->v + 1, sizeof(*b));
>  417         if (b == NULL)
>  418                 goto done;
>  419         for (i = 0; i < c->v; i++) {
>  420                 b[i] = reallocarray(NULL, c->h + 1, sizeof(**b));
>  421                 if (b[i] == NULL) {
>  422                         while (--i >= 0)
>  423                                 free(b[i]);
>  424                         free(b);
>  425                         goto done;
>  426                 }
>  427         }
>  428         b[c->v] = NULL;
>  429         el->el_display = b;
>
> The line buffers are el->el_display[] and they are allocated by
> reallocarray().  Then they are initialized at re_clear_display().
>
> 1149 protected void
> 1150 re_clear_display(EditLine *el)
> 1151 {
> 1152         int i;
> 1153        
> 1154         el->el_cursor.v = 0;
> 1155         el->el_cursor.h = 0;
> 1156         for (i = 0; i < el->el_terminal.t_size.v; i++)
> 1157                 el->el_display[i][0] = '\0';
> 1158         el->el_refresh.r_oldcv = 0;
> 1159 }
>
> Remark that only first char is set NUL, but the rest part is kept
> uninitialized.  Then user inputs "deb".  re_fastputc() is called for
> each char.
>
> 1053 re_fastputc(EditLine *el, wint_t c)
> 1054 {
> 1055         wchar_t *lastline;
> 1056         int w;
> 1057
> 1058         w = wcwidth(c);
> 1059         while (w > 1 && el->el_cursor.h + w > el->el_terminal.t_size.h)
> 1060             re_fastputc(el, ' ');
> 1061
> 1062         terminal__putc(el, c);
> 1063         el->el_display[el->el_cursor.v][el->el_cursor.h++] = c;
>
> the function just put the char like #1063, then the buffer becomes it
> doesn't include any NUL char like below.
>
>   deb + <uninitialized>
>
> After this, by calling re_reresh() the crash happens.
>
> I had looked into the code deeply, but it didn't become clear how
> should we fix this problem.  But I suppose the code assumes the entire
> buffer is initialized, so the following diff replaces the code which
> initializes only first char by the code which calls
> re__copy_and_pad().
>
> comment?
> ok?
>
> Index: lib/libedit/refresh.c
> ===================================================================
> RCS file: /var/cvs/openbsd/src/lib/libedit/refresh.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 refresh.c
> --- lib/libedit/refresh.c 11 Oct 2018 15:19:09 -0000 1.22
> +++ lib/libedit/refresh.c 1 Aug 2019 04:55:34 -0000
> @@ -320,7 +320,8 @@ re_refresh(EditLine *el)
>  #ifdef DEBUG_REFRESH
>   terminal_overwrite(el, L"C\b", 2);
>  #endif /* DEBUG_REFRESH */
> - el->el_display[i][0] = '\0';
> + re__copy_and_pad(el->el_display[i], L"",
> +    (size_t) el->el_terminal.t_size.h);
>   }
>  
>   el->el_refresh.r_oldcv = el->el_refresh.r_newcv; /* set for next time */
> @@ -1154,7 +1155,8 @@ re_clear_display(EditLine *el)
>   el->el_cursor.v = 0;
>   el->el_cursor.h = 0;
>   for (i = 0; i < el->el_terminal.t_size.v; i++)
> - el->el_display[i][0] = '\0';
> + re__copy_and_pad(el->el_display[i], L"",
> +    (size_t)el->el_terminal.t_size.h);
>   el->el_refresh.r_oldcv = 0;
>  }
>  
>

Reply | Threaded
Open this post in threaded view
|

Re: SIGSEGV in libedit

Ingo Schwarze
Hello Yasuoka-san,

YASUOKA Masahiko wrote on Thu, Aug 01, 2019 at 08:42:35PM +0900:

> I noticed the upstream NetBSD recently replaced almost all malloc(3)s
> by calloc(3) in libedit.
>
> https://github.com/NetBSD/src/commit/b91b3c48e0edb116bd797586430cb426b575d717
>
> This also fixes the problem.  I'll create a diff which does the same
> thing.

Might i suggest that you first send a diff changing only the one place
required to fix the bug asou@ reported?  That can easily be reviewed,
and i expect we can get it in quickly, and it allows a useful commit
message explaining why exactly it is needed.


Regarding all the other places in the library:  it is true that in
many situations, zeroing buffers when allocating them provides
additional safety.  But there may be exceptions; in some cases,
it may hide or rarely even cause bugs.

In any case, i hate it when people go "i have no idea whether it
is needed and what it does, but let's just change this globally
anyway".  Even if it's about something as seemingly unconspicious
as zeroing buffers.  The NetBSD commit message provides no indication
that the changed code was inspected for consequences of the change,
and as far as i know Christos (admittedly, i only met him two or
three times and didn't talk all *that* much to him) i guess it might
indeed be his style to commit changes without actually inspecting
the code.

So i think for committing to OpenBSD, each function changed needs
to be inspected and it needs to be confirmed that zeroing each
buffer in question does not cause new problems or hide other
bugs.  That may be somewhat painful work, libedit code is not very
audit friendly, but i don't think cutting corners is a good idea.

In general, code quality in libedit is somewhat below OpenBSD quality
standards, so the time spent auditing it is certainly not wasted
but spent at a place needing it.  Also, the code quality implies
that zeroing some additional buffers likely improves security at
least at some of the places - if it is checked properly.

Yours,
  Ingo

Reply | Threaded
Open this post in threaded view
|

Re: SIGSEGV in libedit

Theo de Raadt-2
> So i think for committing to OpenBSD, each function changed needs
> to be inspected and it needs to be confirmed that zeroing each
> buffer in question does not cause new problems or hide other
> bugs.

Good grief.  That's very much like saying some of the strcpy functions
in the tree were fine.

New problems caused by deterministic initialized input?  Shocking.

Reply | Threaded
Open this post in threaded view
|

Re: SIGSEGV in libedit

YASUOKA Masahiko-3
In reply to this post by Ingo Schwarze
Hi,

On Thu, 1 Aug 2019 18:02:41 +0200
Ingo Schwarze <[hidden email]> wrote:

> YASUOKA Masahiko wrote on Thu, Aug 01, 2019 at 08:42:35PM +0900:
>> I noticed the upstream NetBSD recently replaced almost all malloc(3)s
>> by calloc(3) in libedit.
>>
>> https://github.com/NetBSD/src/commit/b91b3c48e0edb116bd797586430cb426b575d717
>>
>> This also fixes the problem.  I'll create a diff which does the same
>> thing.
>
> Might i suggest that you first send a diff changing only the one place
> required to fix the bug asou@ reported?  That can easily be reviewed,
> and i expect we can get it in quickly, and it allows a useful commit
> message explaining why exactly it is needed.

This is the diff which fixes the problem by replacing malloc by calloc.

ok?

Initialize the line buffer by zero when allocation.  This fixes the
problem a crash happens after the window size change.

Index: lib/libedit/terminal.c
===================================================================
RCS file: /cvs/src/lib/libedit/terminal.c,v
retrieving revision 1.18
diff -u -p -r1.18 terminal.c
--- lib/libedit/terminal.c 12 Apr 2017 18:24:37 -0000 1.18
+++ lib/libedit/terminal.c 5 Aug 2019 07:13:08 -0000
@@ -413,11 +413,11 @@ terminal_alloc_display(EditLine *el)
  wchar_t **b;
  coord_t *c = &el->el_terminal.t_size;
 
- b = reallocarray(NULL, c->v + 1, sizeof(*b));
+ b = calloc(c->v + 1, sizeof(*b));
  if (b == NULL)
  goto done;
  for (i = 0; i < c->v; i++) {
- b[i] = reallocarray(NULL, c->h + 1, sizeof(**b));
+ b[i] = calloc(c->h + 1, sizeof(**b));
  if (b[i] == NULL) {
  while (--i >= 0)
  free(b[i]);
@@ -428,11 +428,11 @@ terminal_alloc_display(EditLine *el)
  b[c->v] = NULL;
  el->el_display = b;
 
- b = reallocarray(NULL, c->v + 1, sizeof(*b));
+ b = calloc(c->v + 1, sizeof(*b));
  if (b == NULL)
  goto done;
  for (i = 0; i < c->v; i++) {
- b[i] = reallocarray(NULL, c->h + 1, sizeof(**b));
+ b[i] = calloc(c->h + 1, sizeof(**b));
  if (b[i] == NULL) {
  while (--i >= 0)
  free(b[i]);

Reply | Threaded
Open this post in threaded view
|

Re: SIGSEGV in libedit

YASUOKA Masahiko-3
In reply to this post by Ingo Schwarze
Hi,

On Thu, 1 Aug 2019 18:02:41 +0200
Ingo Schwarze <[hidden email]> wrote:
> YASUOKA Masahiko wrote on Thu, Aug 01, 2019 at 08:42:35PM +0900:
>> I noticed the upstream NetBSD recently replaced almost all malloc(3)s
>> by calloc(3) in libedit.
>>
>> https://github.com/NetBSD/src/commit/b91b3c48e0edb116bd797586430cb426b575d717
>>
>> This also fixes the problem.  I'll create a diff which does the same
>> thing.
(snip)

> So i think for committing to OpenBSD, each function changed needs
> to be inspected and it needs to be confirmed that zeroing each
> buffer in question does not cause new problems or hide other
> bugs.  That may be somewhat painful work, libedit code is not very
> audit friendly, but i don't think cutting corners is a good idea.
>
> In general, code quality in libedit is somewhat below OpenBSD quality
> standards, so the time spent auditing it is certainly not wasted
> but spent at a place needing it.  Also, the code quality implies
> that zeroing some additional buffers likely improves security at
> least at some of the places - if it is checked properly.

After below diff is applied, I suppose that the memory region which
was undetermined when allocation will be fixed zero, but any other
change than this doesn't exist.

Also I think we need to follow the upstream's change since changes
which will happen on the upstream in future will assume this
initialization.

The diff basically is to do the same thing which is done on the
upstream, but it also replaces h_malloc() and tok_malloc() which the
upstream didn't replace yet.

ok?

Initialize the buffers when allocation.  This happened on the upstream.
http://mail-index.netbsd.org/source-changes/2019/07/23/msg107399.html

Index: lib/libedit/chared.c
===================================================================
RCS file: /cvs/src/lib/libedit/chared.c,v
retrieving revision 1.28
diff -u -p -r1.28 chared.c
--- lib/libedit/chared.c 12 Apr 2017 18:24:37 -0000 1.28
+++ lib/libedit/chared.c 5 Aug 2019 07:31:35 -0000
@@ -404,7 +404,7 @@ ch_init(EditLine *el)
  el->el_chared.c_undo.len = -1;
  el->el_chared.c_undo.cursor = 0;
 
- el->el_chared.c_redo.buf = reallocarray(NULL, EL_BUFSIZ,
+ el->el_chared.c_redo.buf = calloc(EL_BUFSIZ,
     sizeof(*el->el_chared.c_redo.buf));
  if (el->el_chared.c_redo.buf == NULL)
  return -1;
Index: lib/libedit/chartype.c
===================================================================
RCS file: /cvs/src/lib/libedit/chartype.c,v
retrieving revision 1.16
diff -u -p -r1.16 chartype.c
--- lib/libedit/chartype.c 29 Jan 2019 09:47:00 -0000 1.16
+++ lib/libedit/chartype.c 5 Aug 2019 07:31:35 -0000
@@ -145,7 +145,7 @@ ct_decode_argv(int argc, const char *arg
  if (!conv->wsize)
  return NULL;
 
- wargv = reallocarray(NULL, argc + 1, sizeof(*wargv));
+ wargv = calloc(argc + 1, sizeof(*wargv));
 
  for (i = 0, p = conv->wbuff; i < argc; ++i) {
  if (!argv[i]) {   /* don't pass null pointers to mbstowcs */
@@ -214,7 +214,7 @@ ct_visual_string(const wchar_t *s)
  return NULL;
  if (!buff) {
     buffsize = CT_BUFSIZ;
-    buff = reallocarray(NULL, buffsize, sizeof(*buff));
+    buff = calloc(buffsize, sizeof(*buff));
  }
  dst = buff;
  while (*s) {
Index: lib/libedit/filecomplete.c
===================================================================
RCS file: /cvs/src/lib/libedit/filecomplete.c,v
retrieving revision 1.12
diff -u -p -r1.12 filecomplete.c
--- lib/libedit/filecomplete.c 11 Apr 2016 20:43:33 -0000 1.12
+++ lib/libedit/filecomplete.c 5 Aug 2019 07:31:35 -0000
@@ -77,7 +77,7 @@ fn_tilde_expand(const char *txt)
  return NULL;
  } else {
  len = temp - txt + 1; /* text until string after slash */
- temp = malloc(len);
+ temp = calloc(len, 1);
  if (temp == NULL)
  return NULL;
  (void)strncpy(temp, txt + 1, len - 2);
@@ -99,7 +99,7 @@ fn_tilde_expand(const char *txt)
  txt += len;
 
  tempsz = strlen(pass->pw_dir) + 1 + strlen(txt) + 1;
- temp = malloc(tempsz);
+ temp = calloc(tempsz, 1);
  if (temp == NULL)
  return NULL;
  (void)snprintf(temp, tempsz, "%s/%s", pass->pw_dir, txt);
@@ -222,7 +222,7 @@ fn_filename_completion_function(const ch
 #endif
 
  tempsz = strlen(dirname) + len + 1;
- temp = malloc(tempsz);
+ temp = calloc(tempsz, 1);
  if (temp == NULL)
  return NULL;
  (void)snprintf(temp, tempsz, "%s%s", dirname, entry->d_name);
@@ -298,7 +298,7 @@ completion_matches(const char *text, cha
  max_equal = i;
  }
 
- retstr = malloc(max_equal + 1);
+ retstr = calloc(max_equal + 1, 1);
  if (retstr == NULL) {
  free(match_list);
  return NULL;
@@ -421,7 +421,7 @@ fn_complete(EditLine *el,
  ctemp--;
 
  len = li->cursor - ctemp;
- temp = reallocarray(NULL, len + 1, sizeof(*temp));
+ temp = calloc(len + 1, sizeof(*temp));
  (void)wcsncpy(temp, ctemp, len);
  temp[len] = '\0';
 
Index: lib/libedit/hist.c
===================================================================
RCS file: /cvs/src/lib/libedit/hist.c,v
retrieving revision 1.18
diff -u -p -r1.18 hist.c
--- lib/libedit/hist.c 12 Apr 2017 18:24:37 -0000 1.18
+++ lib/libedit/hist.c 5 Aug 2019 07:31:35 -0000
@@ -52,11 +52,10 @@ hist_init(EditLine *el)
 
  el->el_history.fun = NULL;
  el->el_history.ref = NULL;
- el->el_history.buf = reallocarray(NULL, EL_BUFSIZ,
-    sizeof(*el->el_history.buf));
- el->el_history.sz  = EL_BUFSIZ;
+ el->el_history.buf = calloc(EL_BUFSIZ, sizeof(*el->el_history.buf));
  if (el->el_history.buf == NULL)
  return -1;
+ el->el_history.sz  = EL_BUFSIZ;
  el->el_history.last = el->el_history.buf;
  return 0;
 }
Index: lib/libedit/history.c
===================================================================
RCS file: /cvs/src/lib/libedit/history.c,v
retrieving revision 1.28
diff -u -p -r1.28 history.c
--- lib/libedit/history.c 11 Apr 2016 21:17:29 -0000 1.28
+++ lib/libedit/history.c 5 Aug 2019 07:31:35 -0000
@@ -413,7 +413,7 @@ history_def_add(void *p, TYPE(HistEvent)
  if (h->cursor == &h->list)
  return history_def_enter(p, ev, str);
  len = Strlen(evp->str) + Strlen(str) + 1;
- s = reallocarray(NULL, len, sizeof(*s));
+ s = calloc(len, sizeof(*s));
  if (s == NULL) {
  he_seterrev(ev, _HE_MALLOC_FAILED);
  return -1;
@@ -495,7 +495,7 @@ static int
 history_def_insert(history_t *h, TYPE(HistEvent) *ev, const Char *str)
 {
 
- h->cursor = (hentry_t *) malloc(sizeof(hentry_t));
+ h->cursor = calloc(1, sizeof(hentry_t));
  if (h->cursor == NULL)
  goto oomem;
  if ((h->cursor->ev.str = h_strdup(str)) == NULL) {
@@ -551,7 +551,7 @@ history_def_enter(void *p, TYPE(HistEven
 static int
 history_def_init(void **p, TYPE(HistEvent) *ev __attribute__((__unused__)), int n)
 {
- history_t *h = (history_t *) malloc(sizeof(history_t));
+ history_t *h = calloc(1, sizeof(history_t));
  if (h == NULL)
  return -1;
 
@@ -596,7 +596,7 @@ TYPE(History) *
 FUN(history,init)(void)
 {
  TYPE(HistEvent) ev;
- TYPE(History) *h = (TYPE(History) *) malloc(sizeof(TYPE(History)));
+ TYPE(History) *h = calloc(1, sizeof(TYPE(History)));
  if (h == NULL)
  return NULL;
 
@@ -781,7 +781,7 @@ history_load(TYPE(History) *h, const cha
  if (strncmp(line, hist_cookie, sz) != 0)
  goto done;
 
- ptr = malloc(max_size = 1024);
+ ptr = calloc(max_size = 1024, 1);
  if (ptr == NULL)
  goto done;
  for (i = 0; (sz = getline(&line, &llen, fp)) != -1; i++) {
@@ -830,7 +830,7 @@ history_save_fp(TYPE(History) *h, FILE *
  goto done;
  if (fputs(hist_cookie, fp) == EOF)
  goto done;
- ptr = malloc(max_size = 1024);
+ ptr = calloc(max_size = 1024, 1);
  if (ptr == NULL)
  goto done;
  for (i = 0, retval = HLAST(h, &ev);
Index: lib/libedit/keymacro.c
===================================================================
RCS file: /cvs/src/lib/libedit/keymacro.c,v
retrieving revision 1.16
diff -u -p -r1.16 keymacro.c
--- lib/libedit/keymacro.c 25 May 2016 09:23:49 -0000 1.16
+++ lib/libedit/keymacro.c 5 Aug 2019 07:31:36 -0000
@@ -99,8 +99,7 @@ protected int
 keymacro_init(EditLine *el)
 {
 
- el->el_keymacro.buf = reallocarray(NULL, KEY_BUFSIZ,
-    sizeof(*el->el_keymacro.buf));
+ el->el_keymacro.buf = calloc(KEY_BUFSIZ, sizeof(*el->el_keymacro.buf));
  if (el->el_keymacro.buf == NULL)
  return -1;
  el->el_keymacro.map = NULL;
@@ -452,7 +451,7 @@ node__get(wint_t ch)
 {
  keymacro_node_t *ptr;
 
- ptr = malloc(sizeof(*ptr));
+ ptr = calloc(1, sizeof(*ptr));
  if (ptr == NULL)
  return NULL;
  ptr->ch = ch;
Index: lib/libedit/map.c
===================================================================
RCS file: /cvs/src/lib/libedit/map.c,v
retrieving revision 1.27
diff -u -p -r1.27 map.c
--- lib/libedit/map.c 6 May 2016 13:12:52 -0000 1.27
+++ lib/libedit/map.c 5 Aug 2019 07:31:36 -0000
@@ -907,23 +907,21 @@ map_init(EditLine *el)
  EL_ABORT((el->errfile, "Vi insert map incorrect\n"));
 #endif
 
- el->el_map.alt = reallocarray(NULL, N_KEYS, sizeof(el_action_t));
+ el->el_map.alt = calloc(N_KEYS, sizeof(el_action_t));
  if (el->el_map.alt == NULL)
  return -1;
- el->el_map.key = reallocarray(NULL, N_KEYS, sizeof(el_action_t));
+ el->el_map.key = calloc(N_KEYS, sizeof(el_action_t));
  if (el->el_map.key == NULL)
  return -1;
  el->el_map.emacs = el_map_emacs;
  el->el_map.vic = el_map_vi_command;
  el->el_map.vii = el_map_vi_insert;
- el->el_map.help = reallocarray(NULL, EL_NUM_FCNS,
-    sizeof(el_bindings_t));
+ el->el_map.help = calloc(EL_NUM_FCNS, sizeof(el_bindings_t));
  if (el->el_map.help == NULL)
  return -1;
  (void) memcpy(el->el_map.help, el_func_help,
     sizeof(el_bindings_t) * EL_NUM_FCNS);
- el->el_map.func = reallocarray(NULL, EL_NUM_FCNS,
-    sizeof(el_func_t));
+ el->el_map.func = calloc(EL_NUM_FCNS, sizeof(el_func_t));
  if (el->el_map.func == NULL)
  return -1;
  memcpy(el->el_map.func, el_func, sizeof(el_func_t) * EL_NUM_FCNS);
Index: lib/libedit/parse.c
===================================================================
RCS file: /cvs/src/lib/libedit/parse.c,v
retrieving revision 1.20
diff -u -p -r1.20 parse.c
--- lib/libedit/parse.c 11 Apr 2016 21:17:29 -0000 1.20
+++ lib/libedit/parse.c 5 Aug 2019 07:31:36 -0000
@@ -106,7 +106,7 @@ el_wparse(EditLine *el, int argc, const
  if (ptr == argv[0])
  return 0;
  l = ptr - argv[0] - 1;
- tprog = reallocarray(NULL, l + 1, sizeof(*tprog));
+ tprog = calloc(l + 1, sizeof(*tprog));
  if (tprog == NULL)
  return 0;
  (void) wcsncpy(tprog, argv[0], l);
Index: lib/libedit/read.c
===================================================================
RCS file: /cvs/src/lib/libedit/read.c,v
retrieving revision 1.44
diff -u -p -r1.44 read.c
--- lib/libedit/read.c 25 May 2016 09:36:21 -0000 1.44
+++ lib/libedit/read.c 5 Aug 2019 07:31:36 -0000
@@ -79,12 +79,11 @@ read_init(EditLine *el)
 {
  struct macros *ma;
 
- if ((el->el_read = malloc(sizeof(*el->el_read))) == NULL)
+ if ((el->el_read = calloc(1, sizeof(*el->el_read))) == NULL)
  return -1;
 
  ma = &el->el_read->macros;
- if ((ma->macro = reallocarray(NULL, EL_MAXMACRO,
-    sizeof(*ma->macro))) == NULL) {
+ if ((ma->macro = calloc(EL_MAXMACRO, sizeof(*ma->macro))) == NULL) {
  free(el->el_read);
  return -1;
  }
Index: lib/libedit/readline.c
===================================================================
RCS file: /cvs/src/lib/libedit/readline.c,v
retrieving revision 1.28
diff -u -p -r1.28 readline.c
--- lib/libedit/readline.c 28 Jun 2019 13:32:42 -0000 1.28
+++ lib/libedit/readline.c 5 Aug 2019 07:31:36 -0000
@@ -483,7 +483,7 @@ _rl_compat_sub(const char *str, const ch
  } else
  s++;
  }
- r = result = malloc(len + 1);
+ r = result = calloc(len + 1, 1);
  if (result == NULL)
  return NULL;
  s = str;
@@ -573,7 +573,7 @@ get_history_event(const char *cmd, int *
  else if (len == 0)
  return NULL;
  else {
- if ((pat = malloc(len + 1)) == NULL)
+ if ((pat = calloc(len + 1, 1)) == NULL)
  return NULL;
  (void)strncpy(pat, cmd + begin, len);
  pat[len] = '\0';
@@ -667,7 +667,7 @@ _history_expand_command(const char *comm
  } else {
  if (command[offs + 1] == '#') {
  /* use command so far */
- if ((aptr = malloc(offs + 1)) == NULL)
+ if ((aptr = calloc(offs + 1, 1)) == NULL)
  return -1;
  (void)strncpy(aptr, command, offs);
  aptr[offs] = '\0';
@@ -901,7 +901,7 @@ history_expand(char *str, char **output)
  if (str[0] == history_subst_char) {
  /* ^foo^foo2^ is equivalent to !!:s^foo^foo2^ */
  size_t sz = 4 + strlen(str) + 1;
- *output = malloc(sz);
+ *output = calloc(sz, 1);
  if (*output == NULL)
  return 0;
  (*output)[0] = (*output)[1] = history_expansion_char;
@@ -1049,7 +1049,7 @@ history_arg_extract(int start, int end,
  len += strlen(arr[i]) + 1;
  len++;
  max = len;
- result = malloc(len);
+ result = calloc(len, 1);
  if (result == NULL)
  goto out;
 
@@ -1112,7 +1112,7 @@ history_tokenize(const char *str)
  result = nresult;
  }
  len = i - start;
- temp = malloc(len + 1);
+ temp = calloc(len + 1, 1);
  if (temp == NULL) {
  for (i = 0; i < idx; i++)
  free(result[i]);
@@ -1434,7 +1434,7 @@ remove_history(int num)
  if (h == NULL || e == NULL)
  rl_initialize();
 
- if ((he = malloc(sizeof(*he))) == NULL)
+ if ((he = calloc(1, sizeof(*he))) == NULL)
  return NULL;
 
  if (history(h, &ev, H_DELDATA, num, &he->data) != 0) {
@@ -1472,7 +1472,7 @@ replace_history_entry(int num, const cha
  if (history(h, &ev, H_LAST) != 0)
  return NULL; /* error */
 
- if ((he = malloc(sizeof(*he))) == NULL)
+ if ((he = calloc(1, sizeof(*he))) == NULL)
  return NULL;
 
  /* look forwards for event matching specified offset */
@@ -2160,7 +2160,7 @@ rl_completion_matches(const char *str, r
 
  len = 1;
  max = 10;
- if ((list = reallocarray(NULL, max, sizeof(*list))) == NULL)
+ if ((list = calloc(max, sizeof(*list))) == NULL)
  return NULL;
 
  while ((match = (*fun)(str, (int)(len - 1))) != NULL) {
@@ -2195,7 +2195,7 @@ rl_completion_matches(const char *str, r
  if ((list[0] = strdup(str)) == NULL)
  goto out;
  } else {
- if ((list[0] = malloc(min + 1)) == NULL)
+ if ((list[0] = calloc(min + 1, 1)) == NULL)
  goto out;
  (void)memcpy(list[0], list[1], min);
  list[0][min] = '\0';
@@ -2238,7 +2238,7 @@ history_get_history_state(void)
 {
  HISTORY_STATE *hs;
 
- if ((hs = malloc(sizeof(HISTORY_STATE))) == NULL)
+ if ((hs = calloc(1, sizeof(HISTORY_STATE))) == NULL)
  return NULL;
  hs->length = history_length;
  return hs;
Index: lib/libedit/search.c
===================================================================
RCS file: /cvs/src/lib/libedit/search.c,v
retrieving revision 1.27
diff -u -p -r1.27 search.c
--- lib/libedit/search.c 6 May 2016 13:12:52 -0000 1.27
+++ lib/libedit/search.c 5 Aug 2019 07:31:36 -0000
@@ -64,8 +64,7 @@ protected int
 search_init(EditLine *el)
 {
 
- el->el_search.patbuf = reallocarray(NULL, EL_BUFSIZ,
-    sizeof(*el->el_search.patbuf));
+ el->el_search.patbuf = calloc(EL_BUFSIZ, sizeof(*el->el_search.patbuf));
  if (el->el_search.patbuf == NULL)
  return -1;
  *el->el_search.patbuf = L'\0';
Index: lib/libedit/sig.c
===================================================================
RCS file: /cvs/src/lib/libedit/sig.c,v
retrieving revision 1.19
diff -u -p -r1.19 sig.c
--- lib/libedit/sig.c 11 Apr 2016 21:17:29 -0000 1.19
+++ lib/libedit/sig.c 5 Aug 2019 07:31:36 -0000
@@ -115,7 +115,7 @@ sig_init(EditLine *el)
  size_t i;
  sigset_t *nset, oset;
 
- el->el_signal = malloc(sizeof(*el->el_signal));
+ el->el_signal = calloc(1, sizeof(*el->el_signal));
  if (el->el_signal == NULL)
  return -1;
 
Index: lib/libedit/terminal.c
===================================================================
RCS file: /cvs/src/lib/libedit/terminal.c,v
retrieving revision 1.18
diff -u -p -r1.18 terminal.c
--- lib/libedit/terminal.c 12 Apr 2017 18:24:37 -0000 1.18
+++ lib/libedit/terminal.c 5 Aug 2019 07:31:36 -0000
@@ -261,13 +261,13 @@ protected int
 terminal_init(EditLine *el)
 {
 
- el->el_terminal.t_buf = (char *)malloc(TC_BUFSIZE);
+ el->el_terminal.t_buf = calloc(TC_BUFSIZE, 1);
  if (el->el_terminal.t_buf == NULL)
  goto fail1;
- el->el_terminal.t_cap = (char *)malloc(TC_BUFSIZE);
+ el->el_terminal.t_cap = calloc(TC_BUFSIZE, 1);
  if (el->el_terminal.t_cap == NULL)
  goto fail2;
- el->el_terminal.t_fkey = reallocarray(NULL, A_K_NKEYS,
+ el->el_terminal.t_fkey = calloc(A_K_NKEYS,
     sizeof(*el->el_terminal.t_fkey));
  if (el->el_terminal.t_fkey == NULL)
  goto fail3;
Index: lib/libedit/tokenizer.c
===================================================================
RCS file: /cvs/src/lib/libedit/tokenizer.c,v
retrieving revision 1.21
diff -u -p -r1.21 tokenizer.c
--- lib/libedit/tokenizer.c 11 Apr 2016 21:17:29 -0000 1.21
+++ lib/libedit/tokenizer.c 5 Aug 2019 07:31:36 -0000
@@ -110,7 +110,7 @@ FUN(tok,finish)(TYPE(Tokenizer) *tok)
 TYPE(Tokenizer) *
 FUN(tok,init)(const Char *ifs)
 {
- TYPE(Tokenizer) *tok = malloc(sizeof(TYPE(Tokenizer)));
+ TYPE(Tokenizer) *tok = calloc(1, sizeof(TYPE(Tokenizer)));
 
  if (tok == NULL)
  return NULL;
@@ -121,14 +121,14 @@ FUN(tok,init)(const Char *ifs)
  }
  tok->argc = 0;
  tok->amax = AINCR;
- tok->argv = reallocarray(NULL, tok->amax, sizeof(*tok->argv));
+ tok->argv = calloc(tok->amax, sizeof(*tok->argv));
  if (tok->argv == NULL) {
  free(tok->ifs);
  free(tok);
  return NULL;
  }
  tok->argv[0] = NULL;
- tok->wspace = reallocarray(NULL, WINCR, sizeof(*tok->wspace));
+ tok->wspace = calloc(WINCR, sizeof(*tok->wspace));
  if (tok->wspace == NULL) {
  free(tok->argv);
  free(tok->ifs);
Index: lib/libedit/vi.c
===================================================================
RCS file: /cvs/src/lib/libedit/vi.c,v
retrieving revision 1.26
diff -u -p -r1.26 vi.c
--- lib/libedit/vi.c 28 Jun 2019 05:35:34 -0000 1.26
+++ lib/libedit/vi.c 5 Aug 2019 07:31:36 -0000
@@ -1021,13 +1021,13 @@ vi_histedit(EditLine *el, wint_t c __att
  return CC_ERROR;
  len = (size_t)(el->el_line.lastchar - el->el_line.buffer);
 #define TMP_BUFSIZ (EL_BUFSIZ * MB_LEN_MAX)
- cp = malloc(TMP_BUFSIZ);
+ cp = calloc(TMP_BUFSIZ, 1);
  if (cp == NULL) {
  close(fd);
  unlink(tempfile);
  return CC_ERROR;
  }
- line = reallocarray(NULL, len, sizeof(*line));
+ line = calloc(len, sizeof(*line));
  if (line == NULL) {
  close(fd);
  unlink(tempfile);

Reply | Threaded
Open this post in threaded view
|

Re: SIGSEGV in libedit

Ingo Schwarze
In reply to this post by YASUOKA Masahiko-3
Hello Yasuoka-san,

YASUOKA Masahiko wrote on Mon, Aug 05, 2019 at 04:20:41PM +0900:

> This is the diff which fixes the problem by replacing malloc by calloc.
>
> Initialize the line buffer by zero when allocation.  This fixes the
> problem a crash happens after the window size change.

Actually, only the two "b[i] = calloc(..." changes are needed.
The two "b = calloc(..." are pointless because b[] is already
explicitly and completely initialized four lines below.

Then again, even though i doubt that we still regard NetBSD as
"upstream" for libedit, we do still compare code occasionally, so
avoiding gratuitious differences still makes sense.

In this case, the "b = calloc(..." is harmless precisely because the
initializarion has absolutely no effect, being completely overwritten
four lines below.  Performance is irrelevant here because this function
is called quite rarely and certainly not in a loop.

So the diff is OK schwarze@ as is.

Thanks for tracking this down and finding the best fix.

Yours,
  Ingo


> Index: lib/libedit/terminal.c
> ===================================================================
> RCS file: /cvs/src/lib/libedit/terminal.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 terminal.c
> --- lib/libedit/terminal.c 12 Apr 2017 18:24:37 -0000 1.18
> +++ lib/libedit/terminal.c 5 Aug 2019 07:13:08 -0000
> @@ -413,11 +413,11 @@ terminal_alloc_display(EditLine *el)
>   wchar_t **b;
>   coord_t *c = &el->el_terminal.t_size;
>  
> - b = reallocarray(NULL, c->v + 1, sizeof(*b));
> + b = calloc(c->v + 1, sizeof(*b));
>   if (b == NULL)
>   goto done;
>   for (i = 0; i < c->v; i++) {
> - b[i] = reallocarray(NULL, c->h + 1, sizeof(**b));
> + b[i] = calloc(c->h + 1, sizeof(**b));
>   if (b[i] == NULL) {
>   while (--i >= 0)
>   free(b[i]);
> @@ -428,11 +428,11 @@ terminal_alloc_display(EditLine *el)
>   b[c->v] = NULL;
>   el->el_display = b;
>  
> - b = reallocarray(NULL, c->v + 1, sizeof(*b));
> + b = calloc(c->v + 1, sizeof(*b));
>   if (b == NULL)
>   goto done;
>   for (i = 0; i < c->v; i++) {
> - b[i] = reallocarray(NULL, c->h + 1, sizeof(**b));
> + b[i] = calloc(c->h + 1, sizeof(**b));
>   if (b[i] == NULL) {
>   while (--i >= 0)
>   free(b[i]);

Reply | Threaded
Open this post in threaded view
|

Re: SIGSEGV in libedit

Todd C. Miller-3
In reply to this post by YASUOKA Masahiko-3
On Mon, 05 Aug 2019 17:31:51 +0900, YASUOKA Masahiko wrote:

> The diff basically is to do the same thing which is done on the
> upstream, but it also replaces h_malloc() and tok_malloc() which the
> upstream didn't replace yet.

This only changes the reallocarray(3) calls that don't actually
reallocate.  Shouldn't the other calls that do reallocate the buffer
be converted to recallocarray(3)?

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: SIGSEGV in libedit

YASUOKA Masahiko-3
On Mon, 05 Aug 2019 07:10:01 -0600
"Todd C. Miller" <[hidden email]> wrote:
> On Mon, 05 Aug 2019 17:31:51 +0900, YASUOKA Masahiko wrote:
>
>> The diff basically is to do the same thing which is done on the
>> upstream, but it also replaces h_malloc() and tok_malloc() which the
>> upstream didn't replace yet.
>
> This only changes the reallocarray(3) calls that don't actually
> reallocate.

Yes.  el_malloc() had been used for them in the original libedit(3).

> Shouldn't the other calls that do reallocate the buffer be converted
> to recallocarray(3)?

Yes.  Actually I tried.  But I thought it's dangerous since the
current size of the buffer is not clear in some places.  So if we'll
do this, I'd like to do that separately.

Also, the upstream also didn't that yet.

In https://github.com/NetBSD/src/commit/b91b3c48e0edb116bd797586430cb426b575d717
|XXX: should fix realloc similarly.

--yasuoka