From: Otto Moerbeek Subject: Re: vi(1), adding sizeof() to malloc when using CHAR_T To: Walter Alejandro Iglesias Cc: tech@openbsd.org Date: Mon, 20 Apr 2026 12:53:00 +0200 On Mon, Apr 20, 2026 at 12:17:16PM +0200, Walter Alejandro Iglesias wrote: > Hunting the cause of this bug in our vi(1): > > https://marc.info/?l=openbsd-misc&m=177649923622612&w=2 > > I compiled nvi2 from ports on linux to be able to use valgrind. The > output at bottom shows a bunch "Invalid write" that happens after > removing the '* sizeof(CHAR_T)' from MALLOC_RET (which is already added > in this version) in ex_g_setup() (ex/ex_global.c:172). The diff below > adds that same multiplication in other places where CHAR_T is used. > > I don't know if this affects OpenBSD's malloc in the same manner, but I > guess it doesn't hurt to add it: For OpenBSD CHAR_T is uchar_t, which is 1 by definition. So I'm not sure this makes sense as long as we keep it that way. -Otto > > > Index: common/util.c > =================================================================== > RCS file: /cvs/src/usr.bin/vi/common/util.c,v > diff -u -p -u -p -r1.17 util.c > --- common/util.c 11 Jul 2018 06:39:23 -0000 1.17 > +++ common/util.c 7 Apr 2026 16:27:43 -0000 > @@ -103,7 +103,7 @@ v_strdup(SCR *sp, const CHAR_T *str, siz > { > CHAR_T *copy; > > - MALLOC(sp, copy, len + 1); > + MALLOC(sp, copy, (len + 1) * sizeof(CHAR_T)); > if (copy == NULL) > return (NULL); > memcpy(copy, str, len * sizeof(CHAR_T)); > Index: ex/ex_at.c > =================================================================== > RCS file: /cvs/src/usr.bin/vi/ex/ex_at.c,v > diff -u -p -u -p -r1.14 ex_at.c > --- ex/ex_at.c 27 May 2016 09:18:12 -0000 1.14 > +++ ex/ex_at.c 7 Apr 2026 16:27:43 -0000 > @@ -105,7 +105,7 @@ ex_at(SCR *sp, EXCMD *cmdp) > len += tp->len + 1; > } > > - MALLOC_RET(sp, ecp->cp, len * 2); > + MALLOC_RET(sp, ecp->cp, len * 2 * sizeof(CHAR_T)); > ecp->o_cp = ecp->cp; > ecp->o_clen = len; > ecp->cp[len] = '\0'; > Index: ex/ex_global.c > =================================================================== > RCS file: /cvs/src/usr.bin/vi/ex/ex_global.c,v > diff -u -p -u -p -r1.17 ex_global.c > --- ex/ex_global.c 27 May 2016 09:18:12 -0000 1.17 > +++ ex/ex_global.c 7 Apr 2026 16:27:43 -0000 > @@ -170,7 +170,7 @@ usage: ex_emsg(sp, cmdp->cmd->usage, EX > len = 1; > } > > - MALLOC_RET(sp, ecp->cp, len * 2); > + MALLOC_RET(sp, ecp->cp, len * 2 * sizeof(CHAR_T)); > ecp->o_cp = ecp->cp; > ecp->o_clen = len; > memcpy(ecp->cp + len, p, len); > > > > ==12304== Memcheck, a memory error detector > ==12304== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al. > ==12304== Using Valgrind-3.24.0 and LibVEX; rerun with -h for copyright info > ==12304== Command: nvi2/nvi -e foo > ==12304== Parent PID: 8077 > ==12304== > ==12304== Invalid write of size 2 > ==12304== at 0x48525C3: memmove (vg_replace_strmem.c:1414) > ==12304== by 0x141D27: ex_g_setup (ex_global.c:175) > ==12304== by 0x1417E4: ex_global (ex_global.c:40) > ==12304== by 0x135B14: ex_cmd (ex.c:1386) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== Address 0x4b3bf18 is 4 bytes after a block of size 4 alloc'd > ==12304== at 0x4844818: malloc (vg_replace_malloc.c:446) > ==12304== by 0x141C97: ex_g_setup (ex_global.c:172) > ==12304== by 0x1417E4: ex_global (ex_global.c:40) > ==12304== by 0x135B14: ex_cmd (ex.c:1386) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== > ==12304== Invalid read of size 8 > ==12304== at 0x48526E1: memmove (vg_replace_strmem.c:1414) > ==12304== by 0x13772B: ex_load (ex.c:2160) > ==12304== by 0x132C52: ex_cmd (ex.c:322) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== Address 0x4b3bf18 is 4 bytes after a block of size 4 alloc'd > ==12304== at 0x4844818: malloc (vg_replace_malloc.c:446) > ==12304== by 0x141C97: ex_g_setup (ex_global.c:172) > ==12304== by 0x1417E4: ex_global (ex_global.c:40) > ==12304== by 0x135B14: ex_cmd (ex.c:1386) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== > ==12304== Invalid write of size 8 > ==12304== at 0x48526E4: memmove (vg_replace_strmem.c:1414) > ==12304== by 0x13772B: ex_load (ex.c:2160) > ==12304== by 0x132C52: ex_cmd (ex.c:322) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== Address 0x4b3bf10 is 0 bytes inside a block of size 4 alloc'd > ==12304== at 0x4844818: malloc (vg_replace_malloc.c:446) > ==12304== by 0x141C97: ex_g_setup (ex_global.c:172) > ==12304== by 0x1417E4: ex_global (ex_global.c:40) > ==12304== by 0x135B14: ex_cmd (ex.c:1386) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== > ==12304== Invalid read of size 4 > ==12304== at 0x132E27: ex_cmd (ex.c:391) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== Address 0x4b3bf14 is 0 bytes after a block of size 4 alloc'd > ==12304== at 0x4844818: malloc (vg_replace_malloc.c:446) > ==12304== by 0x141C97: ex_g_setup (ex_global.c:172) > ==12304== by 0x1417E4: ex_global (ex_global.c:40) > ==12304== by 0x135B14: ex_cmd (ex.c:1386) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== > ==12304== Conditional jump or move depends on uninitialised value(s) > ==12304== at 0x4851B9E: bcmp (vg_replace_strmem.c:1233) > ==12304== by 0x137C2F: ex_comm_search (ex.c:2283) > ==12304== by 0x133072: ex_cmd (ex.c:454) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== > ==12304== Invalid read of size 1 > ==12304== at 0x4851B69: bcmp (vg_replace_strmem.c:1233) > ==12304== by 0x137C2F: ex_comm_search (ex.c:2283) > ==12304== by 0x133072: ex_cmd (ex.c:454) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== Address 0x4b3bf14 is 0 bytes after a block of size 4 alloc'd > ==12304== at 0x4844818: malloc (vg_replace_malloc.c:446) > ==12304== by 0x141C97: ex_g_setup (ex_global.c:172) > ==12304== by 0x1417E4: ex_global (ex_global.c:40) > ==12304== by 0x135B14: ex_cmd (ex.c:1386) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== > ==12304== Invalid read of size 8 > ==12304== at 0x48526E1: memmove (vg_replace_strmem.c:1414) > ==12304== by 0x13772B: ex_load (ex.c:2160) > ==12304== by 0x132C52: ex_cmd (ex.c:322) > ==12304== by 0x1571C4: v_ex (v_ex.c:401) > ==12304== by 0x169E37: vi (vi.c:226) > ==12304== by 0x154152: ex_visual (ex_visual.c:136) > ==12304== by 0x135B14: ex_cmd (ex.c:1386) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== Address 0x4b3bf18 is 4 bytes after a block of size 4 alloc'd > ==12304== at 0x4844818: malloc (vg_replace_malloc.c:446) > ==12304== by 0x141C97: ex_g_setup (ex_global.c:172) > ==12304== by 0x1417E4: ex_global (ex_global.c:40) > ==12304== by 0x135B14: ex_cmd (ex.c:1386) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== > ==12304== Invalid write of size 8 > ==12304== at 0x48526E4: memmove (vg_replace_strmem.c:1414) > ==12304== by 0x13772B: ex_load (ex.c:2160) > ==12304== by 0x132C52: ex_cmd (ex.c:322) > ==12304== by 0x1571C4: v_ex (v_ex.c:401) > ==12304== by 0x169E37: vi (vi.c:226) > ==12304== by 0x154152: ex_visual (ex_visual.c:136) > ==12304== by 0x135B14: ex_cmd (ex.c:1386) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== Address 0x4b3bf10 is 0 bytes inside a block of size 4 alloc'd > ==12304== at 0x4844818: malloc (vg_replace_malloc.c:446) > ==12304== by 0x141C97: ex_g_setup (ex_global.c:172) > ==12304== by 0x1417E4: ex_global (ex_global.c:40) > ==12304== by 0x135B14: ex_cmd (ex.c:1386) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== > ==12304== Invalid read of size 4 > ==12304== at 0x132E27: ex_cmd (ex.c:391) > ==12304== by 0x1571C4: v_ex (v_ex.c:401) > ==12304== by 0x169E37: vi (vi.c:226) > ==12304== by 0x154152: ex_visual (ex_visual.c:136) > ==12304== by 0x135B14: ex_cmd (ex.c:1386) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== Address 0x4b3bf14 is 0 bytes after a block of size 4 alloc'd > ==12304== at 0x4844818: malloc (vg_replace_malloc.c:446) > ==12304== by 0x141C97: ex_g_setup (ex_global.c:172) > ==12304== by 0x1417E4: ex_global (ex_global.c:40) > ==12304== by 0x135B14: ex_cmd (ex.c:1386) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== > ==12304== Conditional jump or move depends on uninitialised value(s) > ==12304== at 0x4851B9E: bcmp (vg_replace_strmem.c:1233) > ==12304== by 0x137C2F: ex_comm_search (ex.c:2283) > ==12304== by 0x133072: ex_cmd (ex.c:454) > ==12304== by 0x1571C4: v_ex (v_ex.c:401) > ==12304== by 0x169E37: vi (vi.c:226) > ==12304== by 0x154152: ex_visual (ex_visual.c:136) > ==12304== by 0x135B14: ex_cmd (ex.c:1386) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== > ==12304== Invalid read of size 1 > ==12304== at 0x4851B69: bcmp (vg_replace_strmem.c:1233) > ==12304== by 0x137C2F: ex_comm_search (ex.c:2283) > ==12304== by 0x133072: ex_cmd (ex.c:454) > ==12304== by 0x1571C4: v_ex (v_ex.c:401) > ==12304== by 0x169E37: vi (vi.c:226) > ==12304== by 0x154152: ex_visual (ex_visual.c:136) > ==12304== by 0x135B14: ex_cmd (ex.c:1386) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > ==12304== Address 0x4b3bf14 is 0 bytes after a block of size 4 alloc'd > ==12304== at 0x4844818: malloc (vg_replace_malloc.c:446) > ==12304== by 0x141C97: ex_g_setup (ex_global.c:172) > ==12304== by 0x1417E4: ex_global (ex_global.c:40) > ==12304== by 0x135B14: ex_cmd (ex.c:1386) > ==12304== by 0x132655: ex (ex.c:127) > ==12304== by 0x124993: editor (main.c:402) > ==12304== by 0x115766: main (cl_main.c:115) > > > > -- > Walter >