Re: utf8 input in el_gets(3)

[Nicholas Marriott]

Go for it, IIRC there are a few new strcpy that will need care to change
to strlcpy but otherwise it should be easy enough.



On Thu, Dec 24, 2015 at 04:30:37PM -0500, Christian Heckendorf wrote:
> * Michael McConville  [24.12.2015. @16:19:03 -0500]:
> 
> > Christian Heckendorf wrote:
> > > A couple of somewhat recent changes in NetBSD's libedit permit
> > > el_gets(3) to accept multibyte characters if the locale supports
> > > it.
> > > 
> > > A notable user of this function is sftp(1) which will allow users to
> > > input multibyte characters, in filenames for example, once the diff
> > > is applied.
> > 
> > I remember someone recently mentioning that libedit is due for an
> > update. Maybe we should try to include this in a full sync.
> > 
> 
> Assuming someone isn't already working on it, I volunteer.
> 
> > > Index: eln.c
> > > ===
> > > RCS file: /cvs/src/lib/libedit/eln.c,v
> > > retrieving revision 1.4
> > > diff -u -p -r1.4 eln.c
> > > --- eln.c 20 May 2014 11:59:03 -  1.4
> > > +++ eln.c 24 Dec 2015 19:34:09 -
> > > @@ -74,9 +74,18 @@ el_gets(EditLine *el, int *nread)
> > >  {
> > >   const wchar_t *tmp;
> > >  
> > > - el->el_flags |= IGNORE_EXTCHARS;
> > > + if (!(el->el_flags & CHARSET_IS_UTF8))
> > > + el->el_flags |= IGNORE_EXTCHARS;
> > >   tmp = el_wgets(el, nread);
> > > - el->el_flags &= ~IGNORE_EXTCHARS;
> > > + if (tmp != NULL) {
> > > + size_t nwread = 0;
> > > + int i;
> > > + for (i = 0; i < *nread; i++)
> > > + nwread += ct_enc_width(tmp[i]);
> > > + *nread = (int)nwread;
> > > + }
> > > + if (!(el->el_flags & CHARSET_IS_UTF8))
> > > + el->el_flags &= ~IGNORE_EXTCHARS;
> > >   return ct_encode_string(tmp, >el_lgcyconv);
> > >  }
> > >  
> > > 
> > 
> 

Re: ksh.1: simplify SYNOPSIS a bit

[Jason McIntyre]

On Fri, Dec 25, 2015 at 01:23:06PM -0500, Michael Reed wrote:
> Literal pipe characters are easier to read and there isn't any
> difference in the output for mandoc(1) -Thtml and -Tutf8.
> 
> \*(Ba is still used elsewhere in ksh.1, although I didn't touch
> those as I'm unsure if this change is even wanted.
> 

we are moving from the idea that we should use such constructs to ingo's
assertion (see mandoc_char(7), i think) that we should avoid them. ah,
change!

so please mail a diff to fix this consistently for this page, not just
one place.

jmc

> 
> Index: ksh.1
> ===
> RCS file: /cvs/src/bin/ksh/ksh.1,v
> retrieving revision 1.171
> diff -u -p -r1.171 ksh.1
> --- ksh.1 24 Nov 2015 21:07:31 -  1.171
> +++ ksh.1 25 Dec 2015 18:18:51 -
> @@ -14,7 +14,7 @@
>  .Bk -words
>  .Op Fl +abCefhiklmnpruvXx
>  .Op Fl +o Ar option
> -.Op Fl c Ar string \*(Ba Fl s \*(Ba Ar file Op Ar argument ...
> +.Op Fl c Ar string | Fl s | Ar file Op Ar argument ...
>  .Ek
>  .Sh DESCRIPTION
>  .Nm
> 

Re: memory leak in libc

[Ingo Schwarze]

Hi Fritjof,

frit...@alokat.org wrote on Fri, Dec 25, 2015 at 11:56:33AM +0100:

> it looks like there is a memory leak in libc.
> In file "src/lib/libc/stdio/makebuf.c" line 62 malloc(3) is called,
> but never freed, when printf(3) is called.

I think that's a false positive.  The pointer to the buffer is
stored in the FILE object, specifically in fp->_bf._base,
and that is freed in fclose(3), line 54.

Or do you mean to say that _base is overwritten somewhere while
it still points to the allocated memory?  If so, where specifically?

Yours,
  Ingo


> $ valgrind --leak-check=full --show-leak-kinds=all /usr/sbin/apm
> ==29572== Memcheck, a memory error detector
> ==29572== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
> ==29572== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
> ==29572== Command: /usr/sbin/apm
> ==29572== 
> ==29572== 
> ==29572== FILE DESCRIPTORS: 4 open at exit.
> ==29572== Open file descriptor 3: /dev/apm
> ==29572==at 0x5443A2A: open (:2)
> ==29572==by 0x1099B6: ??? (in /usr/sbin/apm)
> ==29572==by 0x108FF0: ??? (in /usr/sbin/apm)
> ==29572== 
> ==29572== Open file descriptor 2:
> ==29572==
> ==29572== 
> ==29572== Open file descriptor 1:
> ==29572==
> ==29572== 
> ==29572== Open file descriptor 0:
> ==29572==
> ==29572== 
> ==29572== 
> ==29572== HEAP SUMMARY:
> ==29572== in use at exit: 65,536 bytes in 1 blocks
> ==29572==   total heap usage: 1 allocs, 0 frees, 65,536 bytes allocated
> ==29572== 
> ==29572== 65,536 bytes in 1 blocks are still reachable in loss record 1 of 1
> ==29572==at 0x5019224: malloc (in 
> /usr/local/lib/valgrind/vgpreload_memcheck-amd64-openbsd.so)
> ==29572==by 0x54AB15B: __smakebuf (makebuf.c:62)
> ==29572==by 0x54A53A9: __swsetup (wsetup.c:73)
> ==29572==by 0x548F2F7: __vfprintf (vfprintf.c:462)
> ==29572==by 0x549211D: vfprintf (vfprintf.c:267)
> ==29572==by 0x547700B: printf (printf.c:44)
> ==29572==by 0x109B8F: ??? (in /usr/sbin/apm)
> ==29572==by 0x108FF0: ??? (in /usr/sbin/apm)
> ==29572== 
> ==29572== LEAK SUMMARY:
> ==29572==definitely lost: 0 bytes in 0 blocks
> ==29572==indirectly lost: 0 bytes in 0 blocks
> ==29572==  possibly lost: 0 bytes in 0 blocks
> ==29572==still reachable: 65,536 bytes in 1 blocks
> ==29572== suppressed: 0 bytes in 0 blocks
> ==29572== 
> ==29572== For counts of detected and suppressed errors, rerun with: -v
> ==29572== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Re: memory leak in libc

[Mark Kettenis]

> Date: Fri, 25 Dec 2015 11:56:33 +0100
> From: frit...@alokat.org
> 
> Hi tech@,
> 
> it looks like there is a memory leak in libc.  In file
> "src/lib/libc/stdio/makebuf.c" line 62 malloc(3) is called, but
> never freed, when printf(3) is called.

Not really a leak.  That buffer gets freed when fclose() gets called.
However we don't do that in the cleanup handler.  I don't know why,
but the commented out code in stdio/findfp.c:_cleanup() suggests
somebody considered doing so at some point.

Re: memory leak in libc

[Ingo Schwarze]

Hi Mark,

Mark Kettenis wrote on Fri, Dec 25, 2015 at 12:30:43PM +0100:
> frit...@alokat.org wrote:

>> it looks like there is a memory leak in libc.  In file
>> "src/lib/libc/stdio/makebuf.c" line 62 malloc(3) is called, but
>> never freed, when printf(3) is called.

> Not really a leak.  That buffer gets freed when fclose() gets called.
> However we don't do that in the cleanup handler.  I don't know why,
> but the commented out code in stdio/findfp.c:_cleanup() suggests
> somebody considered doing so at some point.

Chris Torek in 1990:

  https://svnweb.freebsd.org/csrg/lib/libc/stdio/findfp.c?r1=35686=46074

In the commit message, Keith Bostic doesn't say why he considered it,
or why he decided not to.

Right before exiting, free(3) is optional, sometimes it adds clarity.
But in an atexit(3) handler, it's probably better to only do the
minimum required for correct operation, so i tend to think that
not calling fclose(3) here is actually better.

Yours,
  Ingo

ksh.1: Remove $ERRNO reference

[Michael Reed]

It's had the ``Not implemented'' notice since ksh.1's initial import
19 years ago [1], so it's probably not going to be implemented.

[1]: 
http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/bin/ksh/ksh.1?rev=1.1=text/x-cvsweb-markup



Index: ksh.1
===
RCS file: /cvs/src/bin/ksh/ksh.1,v
retrieving revision 1.171
diff -u -p -r1.171 ksh.1
--- ksh.1   24 Nov 2015 21:07:31 -  1.171
+++ ksh.1   25 Dec 2015 18:32:46 -
@@ -1386,12 +1386,6 @@ is set, it overrides
 If this parameter is found to be set after any profile files are executed, the
 expanded value is used as a shell startup file.
 It typically contains function and alias definitions.
-.It Ev ERRNO
-Integer value of the shell's
-.Va errno
-variable.
-It indicates the reason the last system call failed.
-Not yet implemented.
 .It Ev EXECSHELL
 If set, this parameter is assumed to contain the shell that is to be used to
 execute commands that

ksh.1: simplify SYNOPSIS a bit

[Michael Reed]

Literal pipe characters are easier to read and there isn't any
difference in the output for mandoc(1) -Thtml and -Tutf8.

\*(Ba is still used elsewhere in ksh.1, although I didn't touch
those as I'm unsure if this change is even wanted.


Index: ksh.1
===
RCS file: /cvs/src/bin/ksh/ksh.1,v
retrieving revision 1.171
diff -u -p -r1.171 ksh.1
--- ksh.1   24 Nov 2015 21:07:31 -  1.171
+++ ksh.1   25 Dec 2015 18:18:51 -
@@ -14,7 +14,7 @@
 .Bk -words
 .Op Fl +abCefhiklmnpruvXx
 .Op Fl +o Ar option
-.Op Fl c Ar string \*(Ba Fl s \*(Ba Ar file Op Ar argument ...
+.Op Fl c Ar string | Fl s | Ar file Op Ar argument ...
 .Ek
 .Sh DESCRIPTION
 .Nm

memory leak in libc

[fritjof]

Hi tech@,

it looks like there is a memory leak in libc.
In file "src/lib/libc/stdio/makebuf.c" line 62 malloc(3) is called, but never 
freed, when
printf(3) is called.

--F.

$ valgrind --leak-check=full --show-leak-kinds=all /usr/sbin/apm
==29572== Memcheck, a memory error detector
==29572== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==29572== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==29572== Command: /usr/sbin/apm
==29572== 
==29572== 
==29572== FILE DESCRIPTORS: 4 open at exit.
==29572== Open file descriptor 3: /dev/apm
==29572==at 0x5443A2A: open (:2)
==29572==by 0x1099B6: ??? (in /usr/sbin/apm)
==29572==by 0x108FF0: ??? (in /usr/sbin/apm)
==29572== 
==29572== Open file descriptor 2:
==29572==
==29572== 
==29572== Open file descriptor 1:
==29572==
==29572== 
==29572== Open file descriptor 0:
==29572==
==29572== 
==29572== 
==29572== HEAP SUMMARY:
==29572== in use at exit: 65,536 bytes in 1 blocks
==29572==   total heap usage: 1 allocs, 0 frees, 65,536 bytes allocated
==29572== 
==29572== 65,536 bytes in 1 blocks are still reachable in loss record 1 of 1
==29572==at 0x5019224: malloc (in 
/usr/local/lib/valgrind/vgpreload_memcheck-amd64-openbsd.so)
==29572==by 0x54AB15B: __smakebuf (makebuf.c:62)
==29572==by 0x54A53A9: __swsetup (wsetup.c:73)
==29572==by 0x548F2F7: __vfprintf (vfprintf.c:462)
==29572==by 0x549211D: vfprintf (vfprintf.c:267)
==29572==by 0x547700B: printf (printf.c:44)
==29572==by 0x109B8F: ??? (in /usr/sbin/apm)
==29572==by 0x108FF0: ??? (in /usr/sbin/apm)
==29572== 
==29572== LEAK SUMMARY:
==29572==definitely lost: 0 bytes in 0 blocks
==29572==indirectly lost: 0 bytes in 0 blocks
==29572==  possibly lost: 0 bytes in 0 blocks
==29572==still reachable: 65,536 bytes in 1 blocks
==29572== suppressed: 0 bytes in 0 blocks
==29572== 
==29572== For counts of detected and suppressed errors, rerun with: -v
==29572== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Re: allocation simplifications in yacc

[Theo Buehler]

On Thu, Dec 24, 2015 at 12:41:28PM -0500, Michael McConville wrote:
> 1. realloc acts like malloc when ptr == NULL

Why not

"#endif",
-   "if (newsize && YY_SIZE_MAX / newsize < sizeof *newss)",
-   "goto bail;",
-   "newss = yyss ? (short *)realloc(yyss, newsize * sizeof *newss) :",
-   "  (short *)malloc(newsize * sizeof *newss); /* overflow check 
above */",
+   "newss = reallocarray(yyss, newsize,  sizeof(*newss));

instead?

Note however that the commit message in which the overflow checks were
introduced says

revision 1.28
date: 2007/09/03 21:14:58;  author: deraadt;  state: Exp;  lines: +13 -4;
move back to using malloc() instead of calloc(), because the yacc
skeleton really should only call malloc/realloc/free, no other external
APIs at all.  theefore, add a pre-check for the overflow case, thus
protecting realloc too; tested mblamer, ok millert, help from kettenis

but that was long before reallocarray existed, so I'm not sure.

> 2. no need to check for NULL before free
> 
> ok?
> 
> 
> ? cscope.out
> Index: skeleton.c
> ===
> RCS file: /cvs/src/usr.bin/yacc/skeleton.c,v
> retrieving revision 1.35
> diff -u -p -r1.35 skeleton.c
> --- skeleton.c16 Mar 2014 18:38:30 -  1.35
> +++ skeleton.c24 Dec 2015 17:34:02 -
> @@ -137,16 +137,14 @@ char *body[] =
>   "#endif",
>   "if (newsize && YY_SIZE_MAX / newsize < sizeof *newss)",
>   "goto bail;",
> - "newss = yyss ? (short *)realloc(yyss, newsize * sizeof *newss) :",
> - "  (short *)malloc(newsize * sizeof *newss); /* overflow check 
> above */",
> + "newss = realloc(yyss, newsize * sizeof(*newss)); /* overflow check 
> above */",
>   "if (newss == NULL)",
>   "goto bail;",
>   "yyss = newss;",
>   "yyssp = newss + sslen;",
>   "if (newsize && YY_SIZE_MAX / newsize < sizeof *newvs)",
>   "goto bail;",
> - "newvs = yyvs ? (YYSTYPE *)realloc(yyvs, newsize * sizeof *newvs) 
> :",
> - "  (YYSTYPE *)malloc(newsize * sizeof *newvs); /* overflow check 
> above */",
> + "newvs = realloc(yyvs, newsize * sizeof(*newvs)); /* overflow check 
> above */",
>   "if (newvs == NULL)",
>   "goto bail;",
>   "yyvs = newvs;",
> @@ -155,10 +153,8 @@ char *body[] =
>   "yysslim = yyss + newsize - 1;",
>   "return 0;",
>   "bail:",
> - "if (yyss)",
> - "free(yyss);",
> - "if (yyvs)",
> - "free(yyvs);",
> + "free(yyss);",
> + "free(yyvs);",
>   "yyss = yyssp = NULL;",
>   "yyvs = yyvsp = NULL;",
>   "yystacksize = 0;",
> @@ -368,19 +364,15 @@ char *trailer[] =
>   "yyoverflow:",
>   "yyerror(\"yacc stack overflow\");",
>   "yyabort:",
> - "if (yyss)",
> - "free(yyss);",
> - "if (yyvs)",
> - "free(yyvs);",
> + "free(yyss);",
> + "free(yyvs);",
>   "yyss = yyssp = NULL;",
>   "yyvs = yyvsp = NULL;",
>   "yystacksize = 0;",
>   "return (1);",
>   "yyaccept:",
> - "if (yyss)",
> - "free(yyss);",
> - "if (yyvs)",
> - "free(yyvs);",
> + "free(yyss);",
> + "free(yyvs);",
>   "yyss = yyssp = NULL;",
>   "yyvs = yyvsp = NULL;",
>   "yystacksize = 0;",
> 

Re: bugs in printf(3)

[Ingo Schwarze]

Hi,

more is broken in printf(3).

Ingo Schwarze wrote on Fri, Dec 25, 2015 at 12:30:29AM +0100:

> Fourth file, fourth broken file.
> This is the worst bug found so far.
[...]
> When fprintf(fp, "...%ls...", ...) encounters an encoding error,
> it trashes fp BEYOND REPAIR, clearing all FILE flags, making the
> file unreadable, unwriteable, and probably breaking even more than
> that.  Calling clearerr(3) is by far insufficient to undo the
> devastating damage done.

In addition to that, the format string is parsed with mbrtowc(3),
even if PRINTF_WIDE_CHAR is *not* defined, and the failure is *not*
detected.  That is, invalid bytes in the format string (for example,
0xff in an UTF-8 locale or ISO-Latin bytes in the C locale) are
silently treated as the end of the format string, without reporting
an error, only clobbering errno.

In addition to that, __find_arguments() also returns successfully
in this case, and its return value - which can already be -1 on
malloc failure - is never checked.  This can result in access to
invalid memory.

Fix this by always checking the mbrtowc(3) return value against -1,
by failing at once as soon as that happens, and by always checking
__find_arguments() for success.

The diff below includes my previous diff, both are indeed related.

The new chunks are marked with ***NEW***.

OK?
  Ingo


Index: stdio/vfprintf.c
===
RCS file: /cvs/src/lib/libc/stdio/vfprintf.c,v
retrieving revision 1.69
diff -u -p -r1.69 vfprintf.c
--- stdio/vfprintf.c29 Sep 2015 03:19:24 -  1.69
+++ stdio/vfprintf.c25 Dec 2015 13:33:57 -
@@ -165,10 +165,8 @@ __wcsconv(wchar_t *wcsarg, int prec)
memset(, 0, sizeof(mbs));
p = wcsarg;
nbytes = wcsrtombs(NULL, (const wchar_t **), 0, );
-   if (nbytes == (size_t)-1) {
-   errno = EILSEQ;
+   if (nbytes == (size_t)-1)
return (NULL);
-   }
} else {
/*
 * Optimisation: if the output precision is small enough,
@@ -188,10 +186,8 @@ __wcsconv(wchar_t *wcsarg, int prec)
break;
nbytes += clen;
}
-   if (clen == (size_t)-1) {
-   errno = EILSEQ;
+   if (clen == (size_t)-1)
return (NULL);
-   }
}
}
if ((convbuf = malloc(nbytes + 1)) == NULL)
@@ -203,7 +199,6 @@ __wcsconv(wchar_t *wcsarg, int prec)
if ((nbytes = wcsrtombs(convbuf, (const wchar_t **),
nbytes, )) == (size_t)-1) {
free(convbuf);
-   errno = EILSEQ;
return (NULL);
}
convbuf[nbytes] = '\0';
@@ -438,7 +433,11 @@ __vfprintf(FILE *fp, const char *fmt0, *** NEW ***
int hold = nextarg; \
if (argtable == NULL) { \
argtable = statargtable; \
-   __find_arguments(fmt0, orgap, , ); 
\
+   if (__find_arguments(fmt0, orgap, , \
+   ) == -1) { \
+   ret = -1; \
+   goto error; \
+   } \
} \
nextarg = n2; \
val = GETARG(int); \
@@ -494,6 +493,10 @@ __vfprintf(FILE *fp, const char *fmt0, *** NEW ***
break;
}
}
+   if (n < 0) {
+   ret = -1;
+   goto error;
+   }
if (fmt != cp) {
ptrdiff_t m = fmt - cp;
if (m < 0 || m > INT_MAX - ret)
@@ -501,7 +504,7 @@ __vfprintf(FILE *fp, const char *fmt0, *** NEW ***
PRINT(cp, m);
ret += m;
}
-   if (n <= 0)
+   if (n == 0)
goto done;
fmt++;  /* skip over '%' */
 
@@ -564,8 +567,11 @@ reswitch:  switch (ch) { *** NEW ***
nextarg = n;
if (argtable == NULL) {
argtable = statargtable;
-   __find_arguments(fmt0, orgap,
-   , );
+   if (__find_arguments(fmt0, orgap,
+   , ) == -1) {
+   ret = -1;
+   goto error;
+   }
}
goto rflag;
}
@@ -590,8 +596,11 @@ reswitch:  switch (ch) { *** NEW ***
   

Re: [patch] Fix tput(1) capability's needed argument count

[Alessandro DE LAURENZIS]

Dear tech@ readers,

On Thu 24/12/2015 15:58, Alessandro DE LAURENZIS wrote:
[...]
> my second attempt... I understand that the previous patch was wrong.
> 
> After digging into terminfo(5) man page, I think the problem could be
> related to the if...then...else structures (%?...%t...%e...%t...%;)
> Probably we should reset the parameter counter each time we change the
> branch, proceeding "row by row" as per "infocmp -f" output.

I didn't receive any feedback so far, meaning either xterm 256-color
usage is barely used in our community or my approach in fixing tput is
completely wrong.

Nevertheless, I'm still trying to improve the patch; this time, I
realized that popcount is not actually involved into the parameter
counting process, so no need to reset it:

--- /usr/src/usr.bin/tput/tput.c.orig   Fri Jan 16 07:40:13 2015
+++ /usr/src/usr.bin/tput/tput.cFri Dec 25 14:39:32 2015
@@ -190,10 +190,10 @@
 process(char *cap, char *str, char **argv)
 {
char *cp, *s, *nargv[9];
-   int arg_need, popcount, i;
+   int arg_need, arg_need_p, popcount, i;
 
/* Count how many values we need for this capability. */
-   for (cp = str, arg_need = popcount = 0; *cp != '\0'; cp++) {
+   for (cp = str, arg_need = arg_need_p = popcount = 0; *cp != '\0'; cp++) 
{
if (*cp == '%') {
switch (*++cp) {
case '%':
@@ -224,6 +224,14 @@
case '.':
case '+':
arg_need++;
+   break;
+   case '?':
+   case 'e':
+   arg_need_p = MAXIMUM(arg_need_p, arg_need);
+   arg_need=0;
+   break;
+   case ';':
+   arg_need = MAXIMUM(arg_need_p, arg_need);
break;
default:
break;

-- 
Alessandro DE LAURENZIS
[mailto:just22@gmail.com]
LinkedIn: http://it.linkedin.com/in/delaurenzis

Re: allocation simplifications in yacc

[Theo Buehler]

On Fri, Dec 25, 2015 at 02:34:12PM +0100, Mark Kettenis wrote:
> IMNSHO the code produced by OpenBSD's yacc should be portable;
> reallocarray isn't portable.

ok, thanks for clarifying

Re: allocation simplifications in yacc

[Mark Kettenis]

> Date: Fri, 25 Dec 2015 14:17:19 +0100
> From: Theo Buehler 
> 
> On Thu, Dec 24, 2015 at 12:41:28PM -0500, Michael McConville wrote:
> > 1. realloc acts like malloc when ptr == NULL
> 
> Why not
> 
>   "#endif",
> - "if (newsize && YY_SIZE_MAX / newsize < sizeof *newss)",
> - "goto bail;",
> - "newss = yyss ? (short *)realloc(yyss, newsize * sizeof *newss) :",
> - "  (short *)malloc(newsize * sizeof *newss); /* overflow check 
> above */",
> + "newss = reallocarray(yyss, newsize,  sizeof(*newss));
> 
> instead?
> 
> Note however that the commit message in which the overflow checks were
> introduced says
> 
> revision 1.28
> date: 2007/09/03 21:14:58;  author: deraadt;  state: Exp;  lines: +13 -4;
> move back to using malloc() instead of calloc(), because the yacc
> skeleton really should only call malloc/realloc/free, no other external
> APIs at all.  theefore, add a pre-check for the overflow case, thus
> protecting realloc too; tested mblamer, ok millert, help from kettenis
> 
> but that was long before reallocarray existed, so I'm not sure.

IMNSHO the code produced by OpenBSD's yacc should be portable;
reallocarray isn't portable.

Re: strncpy->strlcpy question

[Michael McConville]

Ricardo Mestre wrote:
> I made an inspection on userland tree and there quite a few
> applications still using strncpy(3) instead of strlcpy(3). Some of
> them may never need that safety since the boundaries are always fixed,
> nevertheless since strlcpy is a drop-in replacement it doesn't hurt to
> use, plus it will always be safer than strncpy.

It does hurt a little bit, sadly, because the l variants are not
POSIX-et-al-specified and many platforms don't have them. They're easy
to add to a port's compat code, but many devs have a slight preference
for the n variants in safe, fixed-bounds situations because of the
porting headache.

Also, as always, churn.

Using these sorts of things everywhere also makes it a little harder for
people to pull functions or chunks of code, although I don't know how
much other devs care about that.

That said, there are specific tools (I'm thinking of httpd and friends,
and smtpd) that already have compat systems. In those, maybe more l
variants wouldn't hurt.

> The same question goes for strncat(3)->strlcat(3), can they just be
> swapped without causing any hiccups? Or should it be kept as is? What
> about kernel space? There's also some usage there.

IIUC, the kernel is a good place for safer but unstandardized features.
I'd be happy to review some of these.

Re: strncpy->strlcpy question

[Philip Guenther]

On Fri, Dec 25, 2015 at 8:21 PM, Ricardo Mestre  wrote:
> I made an inspection on userland tree and there quite a few applications still
> using strncpy(3) instead of strlcpy(3). Some of them may never need that 
> safety
> since the boundaries are always fixed, nevertheless since strlcpy is a drop-in
> replacement it doesn't hurt to use, plus it will always be safer than strncpy.

strlcpy() is not a "drop-in replacement" for strncpy() for _all_
circumstances.  There are conditions under which strncpy()'s behavior
is actually the expected and required.  For example, to quote
utmp(5)'s CAVEATS section:
 The strings in the utmp and lastlog structures are not normal `C' strings
 and are thus not guaranteed to be null terminated.

The practice for those fields is to use strncpy() when filling them,
both so that you don't leak garbage into bytes after the first NUL and
to permit use of the full field width.  Switching to strlcpy() would
be wrong for both reasons.

Those are true of some kernel APIs too, thus explaining why the kernel
uses strncpy() in some places.

These calls have been audited before.  While it's possible some bogus
strncpy() has crawled back in, a careful review of the context of the
usage is necessary to understand whether the code needs either
a) guaranteed NUL fill, or
b) NUL-less full field fill,
or both.

Losing the former can be a security hole if the data crosses a
security boundary!


Philip Guenther

Re: ksh.1: Remove $ERRNO reference

[Alexander Hall]

On Fri, Dec 25, 2015 at 01:36:31PM -0500, Michael Reed wrote:
> It's had the ``Not implemented'' notice since ksh.1's initial import
> 19 years ago [1], so it's probably not going to be implemented.
> 
> [1]: 
> http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/bin/ksh/ksh.1?rev=1.1=text/x-cvsweb-markup
> 

I also noticed this one just the other day. I'm ok halex@ with this.

/Alexander
 
> 
> Index: ksh.1
> ===
> RCS file: /cvs/src/bin/ksh/ksh.1,v
> retrieving revision 1.171
> diff -u -p -r1.171 ksh.1
> --- ksh.1 24 Nov 2015 21:07:31 -  1.171
> +++ ksh.1 25 Dec 2015 18:32:46 -
> @@ -1386,12 +1386,6 @@ is set, it overrides
>  If this parameter is found to be set after any profile files are executed, 
> the
>  expanded value is used as a shell startup file.
>  It typically contains function and alias definitions.
> -.It Ev ERRNO
> -Integer value of the shell's
> -.Va errno
> -variable.
> -It indicates the reason the last system call failed.
> -Not yet implemented.
>  .It Ev EXECSHELL
>  If set, this parameter is assumed to contain the shell that is to be used to
>  execute commands that
> 

strncpy->strlcpy question

[Ricardo Mestre]

Hello,

I made an inspection on userland tree and there quite a few applications still
using strncpy(3) instead of strlcpy(3). Some of them may never need that safety
since the boundaries are always fixed, nevertheless since strlcpy is a drop-in
replacement it doesn't hurt to use, plus it will always be safer than strncpy.

The same question goes for strncat(3)->strlcat(3), can they just be swapped 
without causing any hiccups? Or should it be kept as is? What about kernel 
space?
There's also some usage there.

Best regards,
Ricardo Mestre

Detect more keyboard cases when starting X

[Anthony J. Bentley]

Hi,

As X starts, it will attempt to detect features from the kbd(8)
setting--for example, us.dvorak will enable dvorak in X, and
fr.dvorak will enable French dvorak in X. However, it detects
these features with equality checks, which will fail if multiple
options are set, as in the case of us.dvorak.swapctrlcaps or
fr.dvorak.swapctrlcaps.

Instead of checking for equality, this diff instead checks if the
bits are set. Now us.dvorak.swapctrlcaps and fr.dvorak.swapctrlcaps
work, us.swapctrlcaps.iopener swaps ctrl/caps, and the ones that
already worked (de.nodead, etc) still do.

This was reported on misc@ by "Sevan / Venture37" back in June.

ok?

Index: config/wscons.c
===
RCS file: /cvs/xenocara/xserver/config/wscons.c,v
retrieving revision 1.14
diff -u -p -r1.14 wscons.c
--- config/wscons.c 15 Jan 2015 01:30:40 -  1.14
+++ config/wscons.c 25 Dec 2015 21:37:21 -
@@ -139,7 +139,7 @@ wscons_add_keyboard(void)
 break;
 }
 for (i = 0; kbdvar[i].val; i++)
-if (wsenc == kbdvar[i].val || KB_VARIANT(wsenc) == kbdvar[i].val) {
+if ((wsenc & kbdvar[i].val) == kbdvar[i].val) {
 LogMessageVerb(X_INFO, 3, "wskbd: using variant %s\n",
kbdvar[i].name);
 input_options = input_option_new(input_options,
@@ -147,7 +147,7 @@ wscons_add_keyboard(void)
 break;
 }
 for (i = 0; kbdopt[i].val; i++)
-if (KB_VARIANT(wsenc) == kbdopt[i].val) {
+if (KB_VARIANT(wsenc) & kbdopt[i].val) {
 LogMessageVerb(X_INFO, 3, "wskbd: using option %s\n",
kbdopt[i].name);
 input_options = input_option_new(input_options,

Re: mpsafe re(4)

[Jonathan Matthew]

On Sat, Dec 05, 2015 at 06:11:51PM +0100, Jonathan Matthew wrote:
> The main interesting bit here is the txeof and start loops, which previously
> operated based on the prod/cons indices and the contents of the tx queue,
> but now just uses the indices as that's the only way to get a consistent view
> of the tx queue state.
> 
> At the moment I don't think the tx ring is big enough to use IFQ_DEQUEUE
> instead of ifq_deq_begin/commit, but maybe I'm wrong about that.

dlg@ pointed out that the way the driver modifies rl_flags would be unsafe with
this diff applied, since it sets and clears bits during interrupts and ioctl
handling with no locking.  The only change it makes during interrupts is to
flip RL_FLAG_TIMERINTR, which otherwise is only used during re_init and
re_stop, so this updated diff splits it out into a separate field.

ok?


Index: dev/ic/re.c
===
RCS file: /cvs/src/sys/dev/ic/re.c,v
retrieving revision 1.187
diff -u -p -r1.187 re.c
--- dev/ic/re.c 25 Nov 2015 03:09:58 -  1.187
+++ dev/ic/re.c 26 Dec 2015 03:43:35 -
@@ -120,6 +120,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -151,7 +152,7 @@ int redebug = 0;
 
 static inline void re_set_bufaddr(struct rl_desc *, bus_addr_t);
 
-intre_encap(struct rl_softc *, struct mbuf *, int *);
+intre_encap(struct rl_softc *, struct mbuf *, struct rl_txq *, int *);
 
 intre_newbuf(struct rl_softc *);
 intre_rx_list_init(struct rl_softc *);
@@ -1448,18 +1449,14 @@ re_txeof(struct rl_softc *sc)
struct ifnet*ifp;
struct rl_txq   *txq;
uint32_ttxstat;
-   int idx, descidx, tx = 0;
+   int idx, descidx, tx_free, freed = 0;
 
ifp = >sc_arpcom.ac_if;
 
-   for (idx = sc->rl_ldata.rl_txq_considx;; idx = RL_NEXT_TXQ(sc, idx)) {
+   for (idx = sc->rl_ldata.rl_txq_considx;
+   idx != sc->rl_ldata.rl_txq_prodidx; idx = RL_NEXT_TXQ(sc, idx)) {
txq = >rl_ldata.rl_txq[idx];
 
-   if (txq->txq_mbuf == NULL) {
-   KASSERT(idx == sc->rl_ldata.rl_txq_prodidx);
-   break;
-   }
-
descidx = txq->txq_descidx;
RL_TXDESCSYNC(sc, descidx,
BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
@@ -1470,9 +1467,7 @@ re_txeof(struct rl_softc *sc)
if (txstat & RL_TDESC_CMD_OWN)
break;
 
-   tx = 1;
-   sc->rl_ldata.rl_tx_free += txq->txq_nsegs;
-   KASSERT(sc->rl_ldata.rl_tx_free <= sc->rl_ldata.rl_tx_desc_cnt);
+   freed += txq->txq_nsegs;
bus_dmamap_sync(sc->sc_dmat, txq->txq_dmamap,
0, txq->txq_dmamap->dm_mapsize, BUS_DMASYNC_POSTWRITE);
bus_dmamap_unload(sc->sc_dmat, txq->txq_dmamap);
@@ -1487,9 +1482,13 @@ re_txeof(struct rl_softc *sc)
ifp->if_opackets++;
}
 
-   sc->rl_ldata.rl_txq_considx = idx;
+   if (freed == 0)
+   return (0);
 
-   ifq_clr_oactive(>if_snd);
+   tx_free = atomic_add_int_nv(>rl_ldata.rl_tx_free, freed);
+   KASSERT(tx_free <= sc->rl_ldata.rl_tx_desc_cnt);
+
+   sc->rl_ldata.rl_txq_considx = idx;
 
/*
 * Some chips will ignore a second TX request issued while an
@@ -1498,12 +1497,14 @@ re_txeof(struct rl_softc *sc)
 * to restart the channel here to flush them out. This only
 * seems to be required with the PCIe devices.
 */
-   if (sc->rl_ldata.rl_tx_free < sc->rl_ldata.rl_tx_desc_cnt)
+   if (tx_free < sc->rl_ldata.rl_tx_desc_cnt)
CSR_WRITE_1(sc, sc->rl_txstart, RL_TXSTART_START);
-   else
+   else {
+   ifq_clr_oactive(>if_snd);
ifp->if_timer = 0;
+   }
 
-   return (tx);
+   return (1);
 }
 
 void
@@ -1566,13 +1567,15 @@ re_intr(void *arg)
}
 
if (status & RL_ISR_SYSTEM_ERR) {
+   KERNEL_LOCK();
re_init(ifp);
+   KERNEL_UNLOCK();
claimed = 1;
}
}
 
if (sc->rl_imtype == RL_IMTYPE_SIM) {
-   if ((sc->rl_flags & RL_FLAG_TIMERINTR)) {
+   if (sc->rl_timerintr) {
if ((tx | rx) == 0) {
/*
 * Nothing needs to be processed, fallback
@@ -1599,7 +1602,11 @@ re_intr(void *arg)
}
}
 
-   re_start(ifp);
+   if (!IFQ_IS_EMPTY(>if_snd)) {
+   KERNEL_LOCK();
+   re_start(ifp);
+   KERNEL_UNLOCK();
+   }
 
CSR_WRITE_2(sc, RL_IMR, sc->rl_intrs);
 
@@ -1607,7 +1614,7 @@ re_intr(void *arg)
 }
 
 int
-re_encap(struct rl_softc *sc, struct mbuf *m, int *idx)
+re_encap(struct rl_softc *sc, struct mbuf *m,