Re: Be specific about doas(1) timeout length

[Theo de Raadt]

This is likely to change, in some way.  It is left this way to get
feedback on how it is used.  Small steps. Thanks for the feedback :-)

>So people don't have to look in the source code.
>
>
>Index: doas.conf.5
>===
>RCS file: /cvs/src/usr.bin/doas/doas.conf.5,v
>retrieving revision 1.30
>diff -u -p -r1.30 doas.conf.5
>--- doas.conf.5 2 Sep 2016 18:12:30 -   1.30
>+++ doas.conf.5 5 Sep 2016 21:41:48 -
>@@ -49,7 +49,7 @@ Options are:
> The user is not required to enter a password.
> .It Ic persist
> After the user successfully authenticates, do not ask for a password
>-again for some time.
>+again for 5 minutes.
> .It Ic keepenv
> The user's environment is maintained.
> The default is to reset the environment, except for the variables
>
>

subr_tree for netinet/ip_ipsp.[ch]

[David Gwynne]

this gives us back 5k on amd64.

ok?

Index: ip_ipsp.c
===
RCS file: /cvs/src/sys/netinet/ip_ipsp.c,v
retrieving revision 1.214
diff -u -p -r1.214 ip_ipsp.c
--- ip_ipsp.c   23 May 2015 12:38:53 -  1.214
+++ ip_ipsp.c   6 Sep 2016 05:06:27 -
@@ -98,12 +98,14 @@ struct ipsec_ids_tree ipsec_ids_tree;
 struct ipsec_ids_flows ipsec_ids_flows;
 
 void ipsp_ids_timeout(void *);
-static int ipsp_ids_cmp(struct ipsec_ids *, struct ipsec_ids *);
-static int ipsp_ids_flow_cmp(struct ipsec_ids *, struct ipsec_ids *);
-RB_PROTOTYPE(ipsec_ids_tree, ipsec_ids, id_node_flow, ipsp_ids_cmp);
-RB_PROTOTYPE(ipsec_ids_flows, ipsec_ids, id_node_id, ipsp_ids_flow_cmp);
-RB_GENERATE(ipsec_ids_tree, ipsec_ids, id_node_flow, ipsp_ids_cmp);
-RB_GENERATE(ipsec_ids_flows, ipsec_ids, id_node_id, ipsp_ids_flow_cmp);
+static inline int ipsp_ids_cmp(const struct ipsec_ids *,
+const struct ipsec_ids *);
+static inline int ipsp_ids_flow_cmp(const struct ipsec_ids *,
+const struct ipsec_ids *);
+RBT_PROTOTYPE(ipsec_ids_tree, ipsec_ids, id_node_flow, ipsp_ids_cmp);
+RBT_PROTOTYPE(ipsec_ids_flows, ipsec_ids, id_node_id, ipsp_ids_flow_cmp);
+RBT_GENERATE(ipsec_ids_tree, ipsec_ids, id_node_flow, ipsp_ids_cmp);
+RBT_GENERATE(ipsec_ids_flows, ipsec_ids, id_node_id, ipsp_ids_flow_cmp);
 
 /*
  * This is the proper place to define the various encapsulation transforms.
@@ -905,7 +907,7 @@ ipsp_ids_insert(struct ipsec_ids *ids)
struct ipsec_ids *found;
u_int32_t start_flow;
 
-   found = RB_INSERT(ipsec_ids_tree, _ids_tree, ids);
+   found = RBT_INSERT(ipsec_ids_tree, _ids_tree, ids);
if (found) {
/* if refcount was zero, then timeout is running */
if (found->id_refcount++ == 0)
@@ -917,7 +919,7 @@ ipsp_ids_insert(struct ipsec_ids *ids)
ids->id_flow = start_flow = ipsec_ids_next_flow;
if (++ipsec_ids_next_flow == 0)
ipsec_ids_next_flow = 1;
-   while (RB_INSERT(ipsec_ids_flows, _ids_flows, ids) != NULL) {
+   while (RBT_INSERT(ipsec_ids_flows, _ids_flows, ids) != NULL) {
ids->id_flow = ipsec_ids_next_flow;
if (++ipsec_ids_next_flow == 0)
ipsec_ids_next_flow = 1;
@@ -939,7 +941,7 @@ ipsp_ids_lookup(u_int32_t ipsecflowinfo)
struct ipsec_idskey;
 
key.id_flow = ipsecflowinfo;
-   return RB_FIND(ipsec_ids_flows, _ids_flows, );
+   return RBT_FIND(ipsec_ids_flows, _ids_flows, );
 }
 
 /* free ids only from delayed timeout */
@@ -952,8 +954,8 @@ ipsp_ids_timeout(void *arg)
DPRINTF(("%s: ids %p count %d\n", __func__, ids, ids->id_refcount));
KASSERT(ids->id_refcount == 0);
s = splsoftnet();
-   RB_REMOVE(ipsec_ids_tree, _ids_tree, ids);
-   RB_REMOVE(ipsec_ids_flows, _ids_flows, ids);
+   RBT_REMOVE(ipsec_ids_tree, _ids_tree, ids);
+   RBT_REMOVE(ipsec_ids_flows, _ids_flows, ids);
free(ids->id_local, M_CREDENTIALS, 0);
free(ids->id_remote, M_CREDENTIALS, 0);
free(ids, M_CREDENTIALS, 0);
@@ -988,8 +990,8 @@ ipsp_id_cmp(struct ipsec_id *a, struct i
return memcmp(a + 1, b + 1, a->len);
 }
 
-static int
-ipsp_ids_cmp(struct ipsec_ids *a, struct ipsec_ids *b)
+static inline int
+ipsp_ids_cmp(const struct ipsec_ids *a, const struct ipsec_ids *b)
 {
int ret;
 
@@ -999,8 +1001,8 @@ ipsp_ids_cmp(struct ipsec_ids *a, struct
return ipsp_id_cmp(a->id_local, b->id_local);
 }
 
-static int
-ipsp_ids_flow_cmp(struct ipsec_ids *a, struct ipsec_ids *b)
+static inline int
+ipsp_ids_flow_cmp(const struct ipsec_ids *a, const struct ipsec_ids *b)
 {
if (a->id_flow > b->id_flow)
return 1;
Index: ip_ipsp.h
===
RCS file: /cvs/src/sys/netinet/ip_ipsp.h,v
retrieving revision 1.173
diff -u -p -r1.173 ip_ipsp.h
--- ip_ipsp.h   3 Dec 2015 13:12:20 -   1.173
+++ ip_ipsp.h   6 Sep 2016 05:06:27 -
@@ -166,16 +166,16 @@ struct ipsec_id {
 };
 
 struct ipsec_ids {
-   RB_ENTRY(ipsec_ids) id_node_id;
-   RB_ENTRY(ipsec_ids) id_node_flow;
+   RBT_ENTRY(ipsec_ids)id_node_id;
+   RBT_ENTRY(ipsec_ids)id_node_flow;
struct ipsec_id *id_local;
struct ipsec_id *id_remote;
u_int32_t   id_flow;
int id_refcount;
struct timeout  id_timeout;
 };
-RB_HEAD(ipsec_ids_flows, ipsec_ids);
-RB_HEAD(ipsec_ids_tree, ipsec_ids);
+RBT_HEAD(ipsec_ids_flows, ipsec_ids);
+RBT_HEAD(ipsec_ids_tree, ipsec_ids);
 
 struct ipsec_acquire {
union sockaddr_unionipa_addr;

use subr_tree in net80211

[David Gwynne]

this moves 80211 over to using the function version of red-black
trees. it gives us back the 2.5k of code that RB_GENERATE adds.

ok?

Index: ieee80211_ioctl.c
===
RCS file: /cvs/src/sys/net80211/ieee80211_ioctl.c,v
retrieving revision 1.43
diff -u -p -r1.43 ieee80211_ioctl.c
--- ieee80211_ioctl.c   31 Aug 2016 13:33:52 -  1.43
+++ ieee80211_ioctl.c   6 Sep 2016 03:46:37 -
@@ -754,7 +754,7 @@ ieee80211_ioctl(struct ifnet *ifp, u_lon
case SIOCG80211ALLNODES:
na = (struct ieee80211_nodereq_all *)data;
na->na_nodes = i = 0;
-   ni = RB_MIN(ieee80211_tree, >ic_tree);
+   ni = RBT_MIN(ieee80211_tree, >ic_tree);
while (ni && na->na_size >=
i + sizeof(struct ieee80211_nodereq)) {
ieee80211_node2req(ic, ni, );
@@ -764,7 +764,7 @@ ieee80211_ioctl(struct ifnet *ifp, u_lon
break;
i += sizeof(struct ieee80211_nodereq);
na->na_nodes++;
-   ni = RB_NEXT(ieee80211_tree, >ic_tree, ni);
+   ni = RBT_NEXT(ieee80211_tree, ni);
}
break;
case SIOCG80211FLAGS:
Index: ieee80211_node.c
===
RCS file: /cvs/src/sys/net80211/ieee80211_node.c,v
retrieving revision 1.104
diff -u -p -r1.104 ieee80211_node.c
--- ieee80211_node.c17 Aug 2016 09:42:03 -  1.104
+++ ieee80211_node.c6 Sep 2016 03:46:37 -
@@ -92,9 +92,9 @@ ieee80211_inact_timeout(void *arg)
int s;
 
s = splnet();
-   for (ni = RB_MIN(ieee80211_tree, >ic_tree);
+   for (ni = RBT_MIN(ieee80211_tree, >ic_tree);
ni != NULL; ni = next_ni) {
-   next_ni = RB_NEXT(ieee80211_tree, >ic_tree, ni);
+   next_ni = RBT_NEXT(ieee80211_tree, ni);
if (ni->ni_refcnt > 0)
continue;
if (ni->ni_inact < IEEE80211_INACT_MAX)
@@ -123,7 +123,7 @@ ieee80211_node_attach(struct ifnet *ifp)
int size;
 #endif
 
-   RB_INIT(>ic_tree);
+   RBT_INIT(ieee80211_tree, >ic_tree);
ic->ic_node_alloc = ieee80211_node_alloc;
ic->ic_node_free = ieee80211_node_free;
ic->ic_node_copy = ieee80211_node_copy;
@@ -538,7 +538,7 @@ ieee80211_end_scan(struct ifnet *ifp)
if (ic->ic_scan_count)
ic->ic_flags &= ~IEEE80211_F_ASCAN;
 
-   ni = RB_MIN(ieee80211_tree, >ic_tree);
+   ni = RBT_MIN(ieee80211_tree, >ic_tree);
 
 #ifndef IEEE80211_STA_ONLY
if (ic->ic_opmode == IEEE80211_M_HOSTAP) {
@@ -554,7 +554,7 @@ ieee80211_end_scan(struct ifnet *ifp)
 * channel from the active set.
 */
memset(occupied, 0, sizeof(occupied));
-   RB_FOREACH(ni, ieee80211_tree, >ic_tree)
+   RBT_FOREACH(ni, ieee80211_tree, >ic_tree)
setbit(occupied, ieee80211_chan2ieee(ic, ni->ni_chan));
for (i = 0; i < IEEE80211_CHAN_MAX; i++)
if (isset(ic->ic_chan_active, i) && isclr(occupied, i))
@@ -611,7 +611,7 @@ ieee80211_end_scan(struct ifnet *ifp)
selbs = NULL;
 
for (; ni != NULL; ni = nextbs) {
-   nextbs = RB_NEXT(ieee80211_tree, >ic_tree, ni);
+   nextbs = RBT_NEXT(ieee80211_tree, ni);
if (ni->ni_fails) {
/*
 * The configuration of the access points may change
@@ -807,7 +807,7 @@ ieee80211_setup_node(struct ieee80211com
timeout_set(>ni_sa_query_to, ieee80211_sa_query_timeout, ni);
 #endif
s = splnet();
-   RB_INSERT(ieee80211_tree, >ic_tree, ni);
+   RBT_INSERT(ieee80211_tree, >ic_tree, ni);
ic->ic_nnodes++;
splx(s);
 }
@@ -845,14 +845,14 @@ ieee80211_find_node(struct ieee80211com 
struct ieee80211_node *ni;
int cmp;
 
-   /* similar to RB_FIND except we compare keys, not nodes */
-   ni = RB_ROOT(>ic_tree);
+   /* similar to RBT_FIND except we compare keys, not nodes */
+   ni = RBT_ROOT(ieee80211_tree, >ic_tree);
while (ni != NULL) {
cmp = memcmp(macaddr, ni->ni_macaddr, IEEE80211_ADDR_LEN);
if (cmp < 0)
-   ni = RB_LEFT(ni, ni_node);
+   ni = RBT_LEFT(ieee80211_tree, ni);
else if (cmp > 0)
-   ni = RB_RIGHT(ni, ni_node);
+   ni = RBT_RIGHT(ieee80211_tree, ni);
else
break;
}
@@ -1112,7 +1112,7 @@ ieee80211_free_node(struct ieee80211com 
IEEE80211_AID_CLR(ni->ni_associd, ic->ic_aid_bitmap);
 #endif
ieee80211_ba_del(ni);
-   RB_REMOVE(ieee80211_tree, >ic_tree, ni);
+   RBT_REMOVE(ieee80211_tree, >ic_tree, ni);

fold pool_setipl into pool_init

[David Gwynne]

now that all the pools set an ipl we dont have to support optional ipls.

the ioff argument is and has been unused for many years, so this
replaces it with an ipl argument. cos the ipl is set on init, we
no longer need pool_setipl.

most of semantic changes in diff has been done with coccinelle using
this spatch:

@ipl@
expression pp;
expression ipl;
expression s, a, o, f, m, p;
@@
-pool_init(pp, s, a, o, f, m, p);
-pool_setipl(pp, ipl);
+pool_init(pp, s, a, ipl, f, m, p);

however, cos coccinelle sucks at formatting code, i fixed that by
hand.

i did the manpage and subr_pool.c changes myself. the splasserts
in the get and put paths get a little bit cheaper now.

ok?

Index: share/man/man9/pool.9
===
RCS file: /cvs/src/share/man/man9/pool.9,v
retrieving revision 1.51
diff -u -p -r1.51 pool.9
--- share/man/man9/pool.9   23 Nov 2015 17:53:57 -  1.51
+++ share/man/man9/pool.9   6 Sep 2016 00:35:33 -
@@ -37,7 +37,6 @@
 .Nm pool_get ,
 .Nm pool_put ,
 .Nm pool_prime ,
-.Nm pool_setipl ,
 .Nm pool_sethiwat ,
 .Nm pool_setlowat ,
 .Nm pool_sethardlimit
@@ -50,7 +49,7 @@
 .Fa "struct pool *pool"
 .Fa "size_t size"
 .Fa "u_int align"
-.Fa "u_int align_offset"
+.Fa "int ipl"
 .Fa "int flags"
 .Fa "const char *wmesg"
 .Fa "struct pool_allocator *palloc"
@@ -66,8 +65,6 @@
 .Ft int
 .Fn pool_prime "struct pool *pp" "int nitems"
 .Ft void
-.Fn pool_setipl "struct pool *pp" "int ipl"
-.Ft void
 .Fn pool_sethiwat "struct pool *pp" "int n"
 .Ft void
 .Fn pool_setlowat "struct pool *pp" "int n"
@@ -112,10 +109,12 @@ Specifies the memory address alignment o
 This argument must be a power of two.
 If zero,
 the alignment defaults to an architecture-specific natural alignment.
-.It Fa align_offset
-The offset within an item to which the
-.Fa align
-parameter applies.
+.It Fa ipl
+The interrupt protection level used to protect the pool's internals,
+and at what level at which the pool can be safely used.
+See
+.Xr spl 9
+for a list of the IPLs.
 .It Fa flags
 Specifies various flags set on the pool at creation time.
 .It Fa wmesg
@@ -266,22 +265,6 @@ Unlike
 .Fn pool_prime ,
 this function does not allocate the necessary memory up-front.
 .El
-.Ss SETTING THE PROTECTION LEVEL
-The
-.Fn pool_setipl
-function is used to specify the interrupt protection level at which the pool
-can be safely used.
-.Pp
-.Fn pool_setipl
-.Bl -tag -offset indent -width "flags"
-.It Fa pp
-The handle identifying the pool resource instance.
-.It Fa ipl
-The interrupt protection level used to protect the pool's internals.
-See
-.Xr spl 9
-for a list of the IPLs.
-.El
 .Ss SETTING HARD LIMITS
 The function
 .Fn pool_sethardlimit
@@ -299,11 +282,6 @@ already exceeds the requested hard limit
 .Ss POTENTIAL PITFALLS
 Note that undefined behaviour results when mixing the storage providing
 methods supported by the pool resource routines.
-.Pp
-The pool resource code uses a per-pool lock to protect its internal state.
-If any pool functions are called in an interrupt context,
-the caller must block all interrupts that might cause the
-code to be reentered.
 .Ss DEBUGGING
 To debug a misbehaving pool, a kernel can be compiled with the
 .Dv MALLOC_DEBUG
Index: sys/sys/pool.h
===
RCS file: /cvs/src/sys/sys/pool.h,v
retrieving revision 1.59
diff -u -p -r1.59 pool.h
--- sys/sys/pool.h  21 Apr 2016 04:09:28 -  1.59
+++ sys/sys/pool.h  6 Sep 2016 00:35:41 -
@@ -173,10 +173,9 @@ struct pool_request {
void *pr_item;
 };
 
-void   pool_init(struct pool *, size_t, u_int, u_int, int,
+void   pool_init(struct pool *, size_t, u_int, int, int,
const char *, struct pool_allocator *);
 void   pool_destroy(struct pool *);
-void   pool_setipl(struct pool *, int);
 void   pool_setlowat(struct pool *, int);
 void   pool_sethiwat(struct pool *, int);
 intpool_sethardlimit(struct pool *, u_int, const char *, int);
Index: sys/kern/subr_pool.c
===
RCS file: /cvs/src/sys/kern/subr_pool.c,v
retrieving revision 1.194
diff -u -p -r1.194 subr_pool.c
--- sys/kern/subr_pool.c15 Jan 2016 11:21:58 -  1.194
+++ sys/kern/subr_pool.c6 Sep 2016 00:35:36 -
@@ -218,14 +218,13 @@ pr_find_pagehead(struct pool *pp, void *
  * static pools that must be initialized before malloc() is available.
  */
 void
-pool_init(struct pool *pp, size_t size, u_int align, u_int ioff, int flags,
+pool_init(struct pool *pp, size_t size, u_int align, int ipl, int flags,
 const char *wchan, struct pool_allocator *palloc)
 {
int off = 0, space;
unsigned int pgsize = PAGE_SIZE, items;
 #ifdef DIAGNOSTIC
struct pool *iter;
-   KASSERT(ioff == 0);
 #endif
 
if (align == 0)
@@ 

Re: Make rc scripts use [[ instead of [

[Alexander Hall]

Ok, so I'll throw in my view too.

On September 5, 2016 1:24:17 PM GMT+02:00, Anthony Coulter 
 wrote:
>Some of the system scripts make inconsistent use of ksh-specific
>features, specifically "print" and "[[" as efficient replacements for
>"echo" and "[". This change makes the /etc/ksh.kshrc and all the rc
>scripts use "print" and "[[" exclusively.

Why are they more efficient? They're all builtins.

>
>Switching from echo to print only brought up one interesting issue:
>/etc/rc.d/mountd uses "print -n >foo" instead of "touch foo".
>The latter is arguably more transparent but I didn't change to it.

Also it does not empty the file.

I'd do
  >foo
or possibly
  :>foo

>
>Switching from [ to [[ makes a lot of things more readable; most of
>the improvements result from removing unnecessary quotation marks.
>We also don't have to test for unset variables as aggressively; a
>number of awkward [ X"$foo" = XYes ] constructions were replaced
>with [[ $foo = Yes ]] .

A POSIX shell does not need the X'es. It needs quotes though.

>
>The test [[ ${_args} = "${_args# }" ]] appears in rcctl; I left the
>quotation marks on the right-hand side because I'm afraid of breaking
>something I don't understand.

Without quotes (on the right side) the expression is subject to pattern 
matching.

>
>The rcctl script also has a lot of lines like:
>   daemon_timeout="$(svc_getdef ${_svc} timeout)"
>I think the quotation marks are unnecessary

They are in our ksh, but I don't know what POSIX says.

> but the original author
>went to a lot of trouble to escape internal quotes, e.g.
>   daemon_flags="$(eval print \"\${${_svc}_flags}\")"

Since it uses $(...), those escaped quotes are unrelated to the outer quoting. 
This is by far the best reason to abandon backticks.

>so I'll give him the benefit of the doubt. I would encourage somone
>with greater confidence to take a closer look.
>
>I replaced `backticks` with $(subshell notation) in a few places, but
>only in places where I was already changing [ to [[. I wanted to remove
>them all but I can't really justify the change in terms of either
>readability or process efficiency.

As much as I hate them I love them, because oldschool. For simple cases I think 
there is no big reason to change them although I wouldn't introduce them in new 
code.

>
>Regarding numeric changes: in ksh, [[ 0 -eq 00 ]] is true (numeric
>comparison) but [[ 0 = 00 ]] is false (string comparison). The latter
>is more readable, so I made the switch in places where the left-hand
>side is a system output, e.g. [[ $(id -u ) = 0 ]] or [[ $# > 0 ]].
>I would not expect id(1) to print the string "00" for root, nor would
>I expect the shell to print "00" for $#. But I did *not* change
>comparison operators in more complicated situations, like the bottom
>of /etc/rc which has [[ ${daemon_rtable} -eq 0 ]]. It isn't immediately
>obvious what sort of string is in ${daemon_rtable} so the numeric
>comparison seems appropriate here.

I also tend to check $# = 0 but I never consider it a numeric comparison. If I 
understand you correctly, you want to change a numeric comparison to a string 
comparison. That is a style change, and with no defined code style for shell 
scripts, that could change back and forth forever. I see no point in that.

>
>I corrected a comment at the top of /etc/rc; it said that "set +o sh"
>turned Strict Bourne shell mode off. I'm pretty sure it's actually
>turning strict mode on.

As pointed out already, you were wrong.

-o sets, +o sunsets.

>
>My system still boots with these changes, and rcctl still appears to
>work. I can't easily test all of the code paths in each of the rc
>scripts. But the changes involved here are straightforward, and you
>can validate this diff line-by-line.

While I understand your reasoning, please beware that sending diffs that are 
untested requires other to do your job, and is not a good way to make someone 
willing to look into the subject.

And the diff is far too big. Find *one* (1) *problem*, fix that, test it, and 
send a diff."

I'll end this with a reply I sent off-list regarding parts of this email:

"AFAICT, this does not fix anything, so I don't see the reason for forcibly 
changing from working sh syntax to ksh'isms just "because we can". [] is as 
valid ksh as [[]], and I believe practicing writing portable (at least as far 
as posix) shell scripts is worthwhile, unless it becomes too messy."

Note that this is not meant to being you down. Small (where possible) diffs, 
tested and justified is the way to go.

/Alexander

>
>
>Index: etc/ksh.kshrc
>===
>RCS file: /open/anoncvs/cvs/src/etc/ksh.kshrc,v
>retrieving revision 1.20
>diff -u -p -u -r1.20 ksh.kshrc
>--- etc/ksh.kshrc  18 Feb 2015 08:39:32 -  1.20
>+++ etc/ksh.kshrc  5 Sep 2016 04:03:43 -
>@@ -62,7 +62,7 @@ case "$-" in
>   case "$TERM" in
>   sun*-s)
>   # sun 

acme-client.1: Use indented display for source code

[Michael Reed]

I find that keeping prose at a different indentation level than source
code makes the man page easier to read.  Besides, it's already done
in most other man pages.



Index: acme-client.1
===
RCS file: /cvs/src/usr.sbin/acme-client/acme-client.1,v
retrieving revision 1.7
diff -u -p -r1.7 acme-client.1
--- acme-client.1   1 Sep 2016 13:42:45 -   1.7
+++ acme-client.1   5 Sep 2016 21:59:35 -
@@ -222,11 +222,11 @@ The default challenge directory
 can be served by
 .Xr httpd 8
 with this location block:
-.Bd -literal
-   location "/.well-known/acme-challenge/*" {
-   root "/acme"
-   root strip 2
-   }
+.Bd -literal -offset indent
+location "/.well-known/acme-challenge/*" {
+   root "/acme"
+   root strip 2
+}
 .Ed
 .Pp
 This way, the files placed in
@@ -261,14 +261,14 @@ web server has already been configured t
 as in the
 .Sx Challenges
 section:
-.Bd -literal
+.Bd -literal -offset indent
 # acme-client -vNn foo.com www.foo.com smtp.foo.com
 .Ed
 .Pp
 A daily
 .Xr cron 8
 job can renew the certificates:
-.Bd -literal
+.Bd -literal -offset indent
 #! /bin/sh
 
 acme-client foo.com www.foo.com smtp.foo.com

sysmerge.8: Mention PAGER behavior when undefined/empty

[Michael Reed]

As is done in other man pages.


===
RCS file: /cvs/src/usr.sbin/sysmerge/sysmerge.8,v
retrieving revision 1.78
diff -u -p -r1.78 sysmerge.8
--- sysmerge.8  2 Sep 2016 12:17:33 -   1.78
+++ sysmerge.8  5 Sep 2016 21:54:28 -
@@ -126,6 +126,11 @@ the default is
 .Xr vi 1 .
 .It Ev PAGER
 Specifies the pagination program to use.
+If
+.Ev PAGER
+is empty or not set,
+.Xr more 1
+will be used.
 .El
 .Sh FILES
 .Bl -tag -width "/var/sysmerge/xetc.tgz" -compact

Re: Make rc scripts use [[ instead of [

[Anthony Coulter]

> You don't explain why `print' is more efficient than `echo'.

Short answer: I'm an idiot who didn't realize that `echo' and `[' are
shell builtins. The only parts of my change that still seem worthwhile
to me now are the ones related to the awkward X-comparisons, but there
are few enough of those and they're handled in an idiomatic way, so I'm
not going to bother pushing it. Anybody who has real work to do can
stop reading the email now. The advantage of continuing to read is that
you get some insight into the psychology of a naive user.

Long answer (written partly in a pointless attempt to save face, and
partly to show appreciation to Jeremie's non-condescending response to
what turned out to be a stupid suggestion):

While I vaguely remember reading long ago that `echo' and `[' were
shell builtins, I'm much more strongly aware that /bin/echo and /bin/[
are also available as separate executables. Their man pages don't
mention that they have builtin ksh implementations, and I'm very aware
that `[[' parses arguments differently from `[', e.g.

$ foo="two words"
$ [[ $foo = "two words" ]] && echo True || echo False
True
$ [ $foo = "two words" ] && echo True || echo False
ksh: [: words: unexpected operator/operand
False

which I interpreted as further evidence that `[' and `[[' were
fundamentally different in implementation. Of course, this is wrong,
as "type [" and "type echo" prove. And I remember now that /bin/echo
and /bin/[ are available as separate executables for POSIX reasons,
and that their man pages don't cover the behavior of ksh because they
*aren't* ksh. The point of all of this is that I made an assumption
that seemed reasonable and while I checked some things in the manual,
there were some things I didn't even think to check because they seemed
so obvious. Oops. That happened in another way:

> Nope, ''set -o sh'' sets strict mode on, ''set +o sh'' disables it.
I checked to make sure that "sh" stood for strict mode and not
unstrict mode (since I couldn't understand how that comment could be
so flagrantly wrong), but it never occurred to me to check that `-'
enables something and `+' disables it. Oops again.

In any event, my main goal was to keep us from spawning needless
processes, and my first draft of the change was pretty much just
replacing `[' with `[[' and `echo' with `print.' But since I already
had to change things like [ "$foo" ] to [[ -n "$foo" ]] it seemed
wasteful not to take advantage of the improved argument-parsing of the
`[[' operator and remove the quotation marks, to get [[ -n $foo ]].
But if I'm doing it in some places, I should do it in all places. So
a lot of quotation marks went away to take advantage of the improved
parsing characteristics of `[['.

Some of my other "readability improvements" felt a little awkward and I
couldn't find any reference to shell scripts in style(9) (which I *did*
check before mailing in the proposed change), so I was careful to make
them only in places where I was already breaking stuff. Replacing ``
with $() was an example of that---I left backticks in place in yppasswdd
when I didn't already have a "legitimate" reason to touch the line, and
then I made a point to state explicitly what I had done to see if anyone
would comment on it. Which you did. (I won't try to rationalize my
change to numeric comparisons, since as you say it was a personal
preference, and it was also fairly dubious.)

There's one more specific point I want to make while I'm here about
the awkward X-comparisons: if you search for the phrase "A common
mistake" in the ksh manual, you'll find an example of a comparison
that fails when a variable is unset. Oddly, it only works when the
"if" keyword is used; the "[ foo ] && do_something" construct does not
have any problems.

So, this is the last you'll hear from me on this proposal---it was a
bad idea from the start, and if I had typed "whence -v [" from the
beginning I wouldn't have bothered. But overall I think my
misunderstanding was reasonable, and my "needless churn" would have
been better-received if the underlying reason for the change had been
valid.

And finally, I would again like to thank you (Jeremie) for a thorough
and non-condescending response to a stupid idea. You assumed that I was
sensible enough to read your reviewi and taught me something about the
quote behavior of the eval operator---I hope this (otherwise pointless)
email validates your faith that not all people with stupid ideas are
themselves stupid. And sometimes we read the man pages and we still
miss important things!

Regards,
Anthony

Be specific about doas(1) timeout length

[Michael Reed]

So people don't have to look in the source code.


Index: doas.conf.5
===
RCS file: /cvs/src/usr.bin/doas/doas.conf.5,v
retrieving revision 1.30
diff -u -p -r1.30 doas.conf.5
--- doas.conf.5 2 Sep 2016 18:12:30 -   1.30
+++ doas.conf.5 5 Sep 2016 21:41:48 -
@@ -49,7 +49,7 @@ Options are:
 The user is not required to enter a password.
 .It Ic persist
 After the user successfully authenticates, do not ask for a password
-again for some time.
+again for 5 minutes.
 .It Ic keepenv
 The user's environment is maintained.
 The default is to reset the environment, except for the variables

Re: Make rc scripts use [[ instead of [

[Jeremie Courreges-Anglas]

Hi,

Anthony Coulter  writes:

> Some of the system scripts make inconsistent use of ksh-specific
> features, specifically "print" and "[[" as efficient replacements for
> "echo" and "[". This change makes the /etc/ksh.kshrc and all the rc
> scripts use "print" and "[[" exclusively.

You don't explain why `print' is more efficient than `echo'.

> Switching from echo to print only brought up one interesting issue:
> /etc/rc.d/mountd uses "print -n >foo" instead of "touch foo".
> The latter is arguably more transparent but I didn't change to it.
>
> Switching from [ to [[ makes a lot of things more readable; most of
> the improvements result from removing unnecessary quotation marks.

I'm not sure I see how it makes things more readable; less bytes on the
screen?  Most contexts where variables are involved should use quotes
anyway.  So I am not sure this is a good thing in the end.

> We also don't have to test for unset variables as aggressively; a
> number of awkward [ X"$foo" = XYes ] constructions were replaced
> with [[ $foo = Yes ]] .

This is not related to quote removals.  AFAIK ksh handles [ "$foo" = Yes ]
properly, the "X"s aren't necessary.  They were probably added because
of horror stories about ancient shells.

> The test [[ ${_args} = "${_args# }" ]] appears in rcctl; I left the
> quotation marks on the right-hand side because I'm afraid of breaking
> something I don't understand.

FWIW I can't see why removing the quotes would break anything here.

> The rcctl script also has a lot of lines like:
>   daemon_timeout="$(svc_getdef ${_svc} timeout)"
> I think the quotation marks are unnecessary but the original author
> went to a lot of trouble to escape internal quotes, e.g.
>   daemon_flags="$(eval print \"\${${_svc}_flags}\")"

The "internal" quotes are escaped so that they are still present after
the `eval' pass, not because the whole command substitution itself is
surrounded by double quotes.

var='ab'; eval print "\$var"; eval print \"\$var\"

> so I'll give him the benefit of the doubt. I would encourage somone
> with greater confidence to take a closer look.
>
> I replaced `backticks` with $(subshell notation) in a few places, but
> only in places where I was already changing [ to [[. I wanted to remove
> them all but I can't really justify the change in terms of either
> readability or process efficiency.

While I prefer using $() in my scripts, this looks like needless
churn.  The diff doesn't simplify a single instance of nested ``-style
command substitutions.

> Regarding numeric changes: in ksh, [[ 0 -eq 00 ]] is true (numeric
> comparison) but [[ 0 = 00 ]] is false (string comparison). The latter
> is more readable,

This sounds like a personal preference.  I prefer to use numeric
operators when dealing with numbers.

> so I made the switch in places where the left-hand
> side is a system output, e.g. [[ $(id -u ) = 0 ]] or [[ $# > 0 ]].
> I would not expect id(1) to print the string "00" for root, nor would
> I expect the shell to print "00" for $#. But I did *not* change
> comparison operators in more complicated situations, like the bottom
> of /etc/rc which has [[ ${daemon_rtable} -eq 0 ]]. It isn't immediately
> obvious what sort of string is in ${daemon_rtable} so the numeric
> comparison seems appropriate here.
>
> I corrected a comment at the top of /etc/rc; it said that "set +o sh"
> turned Strict Bourne shell mode off. I'm pretty sure it's actually
> turning strict mode on.

Nope, ''set -o sh'' sets strict mode on, ''set +o sh'' disables it.

[...]

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: Make rc scripts use [[ instead of [

[ludovic coues]

2016-09-05 20:11 GMT+02:00  :
> The fact that one particular system still boots is not "good enough here".
>
> Kind regards,
> Anton
>

That's why he share his work. So other people can try on other system.

-- 

Cordialement, Coues Ludovic
+336 148 743 42

Make rc scripts use [[ instead of [

[Anthony Coulter]

Some of the system scripts make inconsistent use of ksh-specific
features, specifically "print" and "[[" as efficient replacements for
"echo" and "[". This change makes the /etc/ksh.kshrc and all the rc
scripts use "print" and "[[" exclusively.

Switching from echo to print only brought up one interesting issue:
/etc/rc.d/mountd uses "print -n >foo" instead of "touch foo".
The latter is arguably more transparent but I didn't change to it.

Switching from [ to [[ makes a lot of things more readable; most of
the improvements result from removing unnecessary quotation marks.
We also don't have to test for unset variables as aggressively; a
number of awkward [ X"$foo" = XYes ] constructions were replaced
with [[ $foo = Yes ]] .

The test [[ ${_args} = "${_args# }" ]] appears in rcctl; I left the
quotation marks on the right-hand side because I'm afraid of breaking
something I don't understand.

The rcctl script also has a lot of lines like:
daemon_timeout="$(svc_getdef ${_svc} timeout)"
I think the quotation marks are unnecessary but the original author
went to a lot of trouble to escape internal quotes, e.g.
daemon_flags="$(eval print \"\${${_svc}_flags}\")"
so I'll give him the benefit of the doubt. I would encourage somone
with greater confidence to take a closer look.

I replaced `backticks` with $(subshell notation) in a few places, but
only in places where I was already changing [ to [[. I wanted to remove
them all but I can't really justify the change in terms of either
readability or process efficiency.

Regarding numeric changes: in ksh, [[ 0 -eq 00 ]] is true (numeric
comparison) but [[ 0 = 00 ]] is false (string comparison). The latter
is more readable, so I made the switch in places where the left-hand
side is a system output, e.g. [[ $(id -u ) = 0 ]] or [[ $# > 0 ]].
I would not expect id(1) to print the string "00" for root, nor would
I expect the shell to print "00" for $#. But I did *not* change
comparison operators in more complicated situations, like the bottom
of /etc/rc which has [[ ${daemon_rtable} -eq 0 ]]. It isn't immediately
obvious what sort of string is in ${daemon_rtable} so the numeric
comparison seems appropriate here.

I corrected a comment at the top of /etc/rc; it said that "set +o sh"
turned Strict Bourne shell mode off. I'm pretty sure it's actually
turning strict mode on.

My system still boots with these changes, and rcctl still appears to
work. I can't easily test all of the code paths in each of the rc
scripts. But the changes involved here are straightforward, and you
can validate this diff line-by-line.


Index: etc/ksh.kshrc
===
RCS file: /open/anoncvs/cvs/src/etc/ksh.kshrc,v
retrieving revision 1.20
diff -u -p -u -r1.20 ksh.kshrc
--- etc/ksh.kshrc   18 Feb 2015 08:39:32 -  1.20
+++ etc/ksh.kshrc   5 Sep 2016 04:03:43 -
@@ -62,7 +62,7 @@ case "$-" in
case "$TERM" in
sun*-s)
# sun console with status line
-   if [ "$tty" != "$console" ]; then
+   if [[ $tty != $console ]]; then
# ilabel
ILS='\033]L'; ILE='\033\\'
# window title bar
@@ -81,7 +81,7 @@ case "$-" in
*)  ;;
esac
# do we want window decorations?
-   if [ "$ILS" ]; then
+   if [[ -n $ILS ]]; then
function ilabel { print -n "${ILS}$*${ILE}">/dev/tty; }
function label { print -n "${WLS}$*${WLE}">/dev/tty; }
 
@@ -136,14 +136,14 @@ function no_path {
 }
 # if $1 exists and is not in path, append it
 function add_path {
-  [ -d ${1:-.} ] && no_path $* && eval ${2:-PATH}="\$${2:-PATH}:$1"
+  [[ -d ${1:-.} ]] && no_path $* && eval ${2:-PATH}="\$${2:-PATH}:$1"
 }
 # if $1 exists and is not in path, prepend it
 function pre_path {
-  [ -d ${1:-.} ] && no_path $* && eval ${2:-PATH}="$1:\$${2:-PATH}"
+  [[ -d ${1:-.} ]] && no_path $* && eval ${2:-PATH}="$1:\$${2:-PATH}"
 }
 # if $1 is in path, remove it
 function del_path {
-  no_path $* || eval ${2:-PATH}=`eval echo :'$'${2:-PATH}: |
+  no_path $* || eval ${2:-PATH}=`eval print :'$'${2:-PATH}: |
 sed -e "s;:$1:;:;g" -e "s;^:;;" -e "s;:\$;;"`
 }
Index: etc/rc
===
RCS file: /open/anoncvs/cvs/src/etc/rc,v
retrieving revision 1.486
diff -u -p -u -r1.486 rc
--- etc/rc  10 Jul 2016 09:08:18 -  1.486
+++ etc/rc  4 Sep 2016 14:26:10 -
@@ -4,7 +4,7 @@
 # Output and error are redirected to console by init, and the console is the
 # controlling terminal.
 
-# Turn off Strict Bourne shell.
+# Turn on Strict Bourne shell.
 set +o sh
 
 # Subroutines (have to come first).
@@ -137,14 +137,14 @@ make_keys() {
local _iked_pub=/etc/iked/local.pub
 
if [[ ! -f $_isakmpd_key ]]; then
-   echo -n "openssl: generating isakmpd/iked RSA keys... "
+   print -n "openssl: 

whitespace in /etc/rc

[Rob Pierce]

Index: rc
===
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.486
diff -u -p -r1.486 rc
--- rc  10 Jul 2016 09:08:18 -  1.486
+++ rc  5 Sep 2016 15:45:09 -
@@ -490,7 +490,6 @@ echo clearing /tmp
 # rc.securelevel did not specifically set -1 or 2, so select the default: 1.
 (($(sysctl -n kern.securelevel) == 0)) && sysctl kern.securelevel=1
 
-
 # Patch /etc/motd.
 if [[ ! -f /etc/motd ]]; then
install -c -o root -g wheel -m 664 /dev/null /etc/motd

mg: goto end of para @ end of buffer problem

[Mark Lumsden]

Currently in mg, if you have a paragraph:

123
456

With the cursor on either the 4, 5 or 6 and no newline after the '6',
and then execute forward-paragraph (M-}), the cursor sits still and
does not move to the end of second line (after the 6), which is in
effect the end of parapraph. This diff fixes that behaviour. ok?

-lum

Index: paragraph.c
===
RCS file: /cvs/src/usr.bin/mg/paragraph.c,v
retrieving revision 1.44
diff -u -p -u -p -r1.44 paragraph.c
--- paragraph.c 14 Apr 2016 17:05:32 -  1.44
+++ paragraph.c 5 Sep 2016 14:34:45 -
@@ -108,13 +108,13 @@ do_gotoeop(int f, int n, int *i)
curwp->w_dotp = lforw(curwp->w_dotp);
curwp->w_dotline++;
 
-   /* do not continue after end of buffer */
-   if (lforw(curwp->w_dotp) == curbp->b_headp) {
-   gotoeol(FFRAND, 1);
-   curwp->w_rflag |= WFMOVE;
-   return (FALSE);
-   }
}
+   }
+   /* do not continue after end of buffer */
+   if (lforw(curwp->w_dotp) == curbp->b_headp) {
+   gotoeol(FFRAND, 1);
+   curwp->w_rflag |= WFMOVE;
+   return (FALSE);
}
 
/* force screen update */

Re: [PATCH] Callback-based interface to libtls

[busterb]

Hey, the typedef came in handy :) Ok bcook@

> On Sep 5, 2016, at 11:52 AM, Bob Beck  wrote:
> 
> I am in agreement in principle, but please coordinate with bcook@ and/or 
> jsing@ who were possibly doing
> some related adjustments. 
> 
> 
> 
>> On Mon, Sep 5, 2016 at 4:44 AM, Ted Unangst  wrote:
>> Bob Beck wrote:
>> > >
>> > > Agreed, I was also a bit unclear on payload at first (though it grew on
>> > > me over time, so I didn't change it). Here's an update with the
>> > > parameter renamed and better documented.
>> > >
>> > > ok?
>> >
>> > Yeah. I'm good with this
>> >
>> > IMO get it in so we can tweak it in tree.
>> 
>> first tweak: the context has a type: struct tls *, so use it.
>> 
>> Index: tls.h
>> ===
>> RCS file: /cvs/src/lib/libtls/tls.h,v
>> retrieving revision 1.37
>> diff -u -p -r1.37 tls.h
>> --- tls.h   4 Sep 2016 14:15:44 -   1.37
>> +++ tls.h   5 Sep 2016 10:42:50 -
>> @@ -44,9 +44,9 @@ extern "C" {
>>  struct tls;
>>  struct tls_config;
>> 
>> -typedef ssize_t (*tls_read_cb)(void *_ctx, void *_buf, size_t _buflen,
>> +typedef ssize_t (*tls_read_cb)(struct tls *_ctx, void *_buf, size_t _buflen,
>>  void *_cb_arg);
>> -typedef ssize_t (*tls_write_cb)(void *_ctx, const void *_buf,
>> +typedef ssize_t (*tls_write_cb)(struct tls *_ctx, const void *_buf,
>>  size_t _buflen, void *_cb_arg);
>> 
>>  int tls_init(void);
>> Index: tls_init.3
>> ===
>> RCS file: /cvs/src/lib/libtls/tls_init.3,v
>> retrieving revision 1.71
>> diff -u -p -r1.71 tls_init.3
>> --- tls_init.3  4 Sep 2016 16:37:18 -   1.71
>> +++ tls_init.3  5 Sep 2016 10:43:43 -
>> @@ -189,13 +189,13 @@
>>  .Ft "int"
>>  .Fn tls_connect_socket "struct tls *ctx" "int s" "const char *servername"
>>  .Ft "int"
>> -.Fn tls_connect_cbs "struct tls *ctx" "ssize_t (*tls_read_cb)(void *ctx, 
>> void *buf, size_t buflen, void *cb_arg)" "ssize_t (*tls_write_cb)(void *ctx, 
>> const void *buf, size_t buflen, void *cb_arg)" "void *cb_arg" "const char 
>> *servername"
>> +.Fn tls_connect_cbs "struct tls *ctx" "ssize_t (*tls_read_cb)(struct tls 
>> *ctx, void *buf, size_t buflen, void *cb_arg)" "ssize_t 
>> (*tls_write_cb)(struct tls *ctx, const void *buf, size_t buflen, void 
>> *cb_arg)" "void *cb_arg" "const char *servername"
>>  .Ft "int"
>>  .Fn tls_accept_fds "struct tls *tls" "struct tls **cctx" "int fd_read" "int 
>> fd_write"
>>  .Ft "int"
>>  .Fn tls_accept_socket "struct tls *tls" "struct tls **cctx" "int socket"
>>  .Ft "int"
>> -.Fn tls_accept_cbs "struct tls *ctx" "struct tls **cctx" "ssize_t 
>> (*tls_read_cb)(void *ctx, void *buf, size_t buflen, void *cb_arg)" "ssize_t 
>> (*tls_write_cb)(void *ctx, const void *buf, size_t buflen, void *cb_arg)" 
>> "void *cb_arg"
>> +.Fn tls_accept_cbs "struct tls *ctx" "struct tls **cctx" "ssize_t 
>> (*tls_read_cb)(struct *ctx, void *buf, size_t buflen, void *cb_arg)" 
>> "ssize_t (*tls_write_cb)(struct tls *ctx, const void *buf, size_t buflen, 
>> void *cb_arg)" "void *cb_arg"
>>  .Ft "int"
>>  .Fn tls_handshake "struct tls *ctx"
>>  .Ft "ssize_t"
> 

Re: Remove /usr/share/misc/eqnchar

[Todd C. Miller]

On Sun, 04 Sep 2016 20:40:47 -0600, "Anthony J. Bentley" wrote:

> eqnchar is a collection of eqn(7) definitions to create mathematical
> symbols by constructing them from other characters. Creating circled
> plus with O, a backspace, and a plus, for example. The results are
> quite ugly in both mandoc and groff if it even works at all.
> 
> Nothing in base (or even anywhere?) uses these weird macros. Anyone
> doing mathematical typesetting will be much better served by reading
> through mandoc_char(7) or the Unicode Mathematical Operators block.

OK.

 - todd

Re: Sync getopt with getopt

[Todd C. Miller]

On Mon, 05 Sep 2016 04:02:47 -0400, "Ted Unangst" wrote:

> Todd C. Miller wrote:
> > On Sun, 04 Sep 2016 11:58:23 -0600, "Anthony J. Bentley" wrote:
> > 
> > > This brings /usr/share/misc/getopt in sync with the example in getopt(3).
> > 
> > OK, though I wonder if anyone actually looks at this file?
> 
> i think it's better to delete it.

Works for me.

 - todd

Re: Bridge broken in 6.0?

[Theo de Raadt]

>I am curious though - is dhclient really the right place to fix this?  I
>might use some other dhcp client (dhcpcd in ports for example) or some
>other application that uses BPF.  Should every userland program using BPF
>have to worry whether or not it is breaking bridging?

Various solutions are being discussed, so far all have downsides.
For now, the situation must remain because changing it back would
impact ongoing work.

Re: Bridge broken in 6.0?

[Aaron Riekenberg]

Thanks for the explanation.

I am curious though - is dhclient really the right place to fix this?  I
might use some other dhcp client (dhcpcd in ports for example) or some
other application that uses BPF.  Should every userland program using BPF
have to worry whether or not it is breaking bridging?

On Sun, Sep 4, 2016 at 9:12 AM, Ted Unangst  wrote:

> Aaron Riekenberg wrote:
> > Based on Martin's hint I tried an experiment - I configured em0
> statically
> > instead of using dhcp.  This fixes my problem.
> >
> > So to summarize: in 6.0 running dhclient on a bridge-member interface
> > breaks dhcp traffic passing through the bridge.  This worked in 5.9.
> Also
> > the FAQ bridge example uses DHCP on a member interface:
> > http://www.openbsd.org/faq/faq6.html#Bridge
>
> So everybody understands what's happening behind the scenes and why this
> is a
> new problem...
>
> Long ago, a flag was added to BPF filters to drop matching packets.
> dhclient
> sets this flag on the filter it uses to listen to dhcp packets, which has
> the
> (intended) effect of causing dhclient to eat the packet so nobody else sees
> it.
>
> Between 5.9 and 6.0, some code in the network stack was shuffled about and
> this changed the order in which some operations were performed. In
> particular,
> now BPF sees packets before bridge forwards them, instead of after.
> Therefore,
> if dhclient eats the packet, it won't be forwarded.
>
> dhclient was modified to only match packets intended for *this* machine by
> specifying a filter for the local machine's MAC address. However, there are
> buggy dhcp servers that always send packets to the broadcast address. The
> dhclient filter change was reverted and it once again eats all dhcp
> packets.
>
>

Re: [PATCH] Callback-based interface to libtls

[Tobias Pape]

Hi all

thank you for including the feature.
I'm testing with my usecase and will report
back if strange things happen.

Thanks again and best regards
-Tobias

On 04.09.2016, at 12:28, Bob Beck  wrote:

> 
> On Sun, Sep 04, 2016 at 05:26:24AM -0500, Brent Cook wrote:
>> On Sun, Sep 04, 2016 at 05:57:54AM -0400, Ted Unangst wrote:
>>> Brent Cook wrote:
 @@ -246,14 +252,18 @@ An already existing socket can be upgrad
 .Fn tls_connect_socket .
 Alternatively, a secure connection can be established over a pair of 
 existing
 file descriptors by calling
 -.Fn tls_connect_fds .
 +.Fn tls_connect_fds . Using
 +.Fn tls_connect_cbs , read and write callbacks can be specified to handle 
 the
 +actual data transfer.
>>> 
>>> I think we need just a wee bit more documentation. payload is not the 
>>> clearest
>>> name. It sounds like connection data. I think cookie? Or cbarg? Is it
>>> necessary to pass the tls context to the callback? I think that's unusual.
>>> 
>>> read callback should be more like:
>>> 
>>> ssize_t (*read_cb)(void *buf, size_t buflen, void *cbarg);
>> 
>> Agreed, I was also a bit unclear on payload at first (though it grew on
>> me over time, so I didn't change it). Here's an update with the
>> parameter renamed and better documented.
>> 
>> ok?
> 
> Yeah. I'm good with this
> 
> IMO get it in so we can tweak it in tree. 
> 
> ok beck@
> 
> and don't forget to bump all the minors of all the things
> 
> 
>> 
>> Index: Makefile
>> ===
>> RCS file: /cvs/src/lib/libtls/Makefile,v
>> retrieving revision 1.23
>> diff -u -p -u -p -r1.23 Makefile
>> --- Makefile 30 Mar 2016 06:38:43 -  1.23
>> +++ Makefile 4 Sep 2016 10:23:42 -
>> @@ -13,6 +13,7 @@ LDADD+= -L${BSDOBJDIR}/lib/libssl/ssl -l
>> HDRS=tls.h
>> 
>> SRCS=tls.c \
>> +tls_bio_cb.c \
>>  tls_client.c \
>>  tls_config.c \
>>  tls_conninfo.c \
>> Index: shlib_version
>> ===
>> RCS file: /cvs/src/lib/libtls/shlib_version,v
>> retrieving revision 1.20
>> diff -u -p -u -p -r1.20 shlib_version
>> --- shlib_version31 Aug 2016 23:05:30 -  1.20
>> +++ shlib_version4 Sep 2016 10:23:42 -
>> @@ -1,2 +1,2 @@
>> major=11
>> -minor=3
>> +minor=4
>> Index: tls.c
>> ===
>> RCS file: /cvs/src/lib/libtls/tls.c,v
>> retrieving revision 1.48
>> diff -u -p -u -p -r1.48 tls.c
>> --- tls.c22 Aug 2016 17:12:35 -  1.48
>> +++ tls.c4 Sep 2016 10:23:42 -
>> @@ -424,6 +424,10 @@ tls_reset(struct tls *ctx)
>>  tls_sni_ctx_free(sni);
>>  }
>>  ctx->sni_ctx = NULL;
>> +
>> +ctx->read_cb = NULL;
>> +ctx->write_cb = NULL;
>> +ctx->cb_arg = NULL;
>> }
>> 
>> int
>> Index: tls.h
>> ===
>> RCS file: /cvs/src/lib/libtls/tls.h,v
>> retrieving revision 1.35
>> diff -u -p -u -p -r1.35 tls.h
>> --- tls.h22 Aug 2016 14:58:26 -  1.35
>> +++ tls.h4 Sep 2016 10:23:42 -
>> @@ -44,6 +44,11 @@ extern "C" {
>> struct tls;
>> struct tls_config;
>> 
>> +typedef ssize_t (*tls_read_cb)(void *_ctx, void *_buf, size_t _buflen,
>> +void *_cb_arg);
>> +typedef ssize_t (*tls_write_cb)(void *_ctx, const void *_buf,
>> +size_t _buflen, void *_cb_arg);
>> +
>> int tls_init(void);
>> 
>> const char *tls_config_error(struct tls_config *_config);
>> @@ -102,12 +107,16 @@ void tls_free(struct tls *_ctx);
>> int tls_accept_fds(struct tls *_ctx, struct tls **_cctx, int _fd_read,
>> int _fd_write);
>> int tls_accept_socket(struct tls *_ctx, struct tls **_cctx, int _socket);
>> +int tls_accept_cbs(struct tls *_ctx, struct tls **_cctx,
>> +tls_read_cb _read_cb, tls_write_cb _write_cb, void *_cb_arg);
>> int tls_connect(struct tls *_ctx, const char *_host, const char *_port);
>> int tls_connect_fds(struct tls *_ctx, int _fd_read, int _fd_write,
>> const char *_servername);
>> int tls_connect_servername(struct tls *_ctx, const char *_host,
>> const char *_port, const char *_servername);
>> int tls_connect_socket(struct tls *_ctx, int _s, const char *_servername);
>> +int tls_connect_cbs(struct tls *_ctx, tls_read_cb _read_cb,
>> +tls_write_cb _write_cb, void *_cb_arg, const char *_servername);
>> int tls_handshake(struct tls *_ctx);
>> ssize_t tls_read(struct tls *_ctx, void *_buf, size_t _buflen);
>> ssize_t tls_write(struct tls *_ctx, const void *_buf, size_t _buflen);
>> Index: tls_bio_cb.c
>> ===
>> RCS file: tls_bio_cb.c
>> diff -N tls_bio_cb.c
>> --- /dev/null1 Jan 1970 00:00:00 -
>> +++ tls_bio_cb.c 4 Sep 2016 10:23:42 -
>> @@ -0,0 +1,224 @@
>> +/* $ID$ */
>> +/*
>> + * Copyright (c) 2016 Tobias Pape 
>> + *
>> + * Permission to use, 

Re: [PATCH] Callback-based interface to libtls

[Bob Beck]

I am in agreement in principle, but please coordinate with bcook@ and/or
jsing@ who were possibly doing
some related adjustments.



On Mon, Sep 5, 2016 at 4:44 AM, Ted Unangst  wrote:

> Bob Beck wrote:
> > >
> > > Agreed, I was also a bit unclear on payload at first (though it grew on
> > > me over time, so I didn't change it). Here's an update with the
> > > parameter renamed and better documented.
> > >
> > > ok?
> >
> > Yeah. I'm good with this
> >
> > IMO get it in so we can tweak it in tree.
>
> first tweak: the context has a type: struct tls *, so use it.
>
> Index: tls.h
> ===
> RCS file: /cvs/src/lib/libtls/tls.h,v
> retrieving revision 1.37
> diff -u -p -r1.37 tls.h
> --- tls.h   4 Sep 2016 14:15:44 -   1.37
> +++ tls.h   5 Sep 2016 10:42:50 -
> @@ -44,9 +44,9 @@ extern "C" {
>  struct tls;
>  struct tls_config;
>
> -typedef ssize_t (*tls_read_cb)(void *_ctx, void *_buf, size_t _buflen,
> +typedef ssize_t (*tls_read_cb)(struct tls *_ctx, void *_buf, size_t
> _buflen,
>  void *_cb_arg);
> -typedef ssize_t (*tls_write_cb)(void *_ctx, const void *_buf,
> +typedef ssize_t (*tls_write_cb)(struct tls *_ctx, const void *_buf,
>  size_t _buflen, void *_cb_arg);
>
>  int tls_init(void);
> Index: tls_init.3
> ===
> RCS file: /cvs/src/lib/libtls/tls_init.3,v
> retrieving revision 1.71
> diff -u -p -r1.71 tls_init.3
> --- tls_init.3  4 Sep 2016 16:37:18 -   1.71
> +++ tls_init.3  5 Sep 2016 10:43:43 -
> @@ -189,13 +189,13 @@
>  .Ft "int"
>  .Fn tls_connect_socket "struct tls *ctx" "int s" "const char *servername"
>  .Ft "int"
> -.Fn tls_connect_cbs "struct tls *ctx" "ssize_t (*tls_read_cb)(void *ctx,
> void *buf, size_t buflen, void *cb_arg)" "ssize_t (*tls_write_cb)(void
> *ctx, const void *buf, size_t buflen, void *cb_arg)" "void *cb_arg" "const
> char *servername"
> +.Fn tls_connect_cbs "struct tls *ctx" "ssize_t (*tls_read_cb)(struct tls
> *ctx, void *buf, size_t buflen, void *cb_arg)" "ssize_t
> (*tls_write_cb)(struct tls *ctx, const void *buf, size_t buflen, void
> *cb_arg)" "void *cb_arg" "const char *servername"
>  .Ft "int"
>  .Fn tls_accept_fds "struct tls *tls" "struct tls **cctx" "int fd_read"
> "int fd_write"
>  .Ft "int"
>  .Fn tls_accept_socket "struct tls *tls" "struct tls **cctx" "int socket"
>  .Ft "int"
> -.Fn tls_accept_cbs "struct tls *ctx" "struct tls **cctx" "ssize_t
> (*tls_read_cb)(void *ctx, void *buf, size_t buflen, void *cb_arg)" "ssize_t
> (*tls_write_cb)(void *ctx, const void *buf, size_t buflen, void *cb_arg)"
> "void *cb_arg"
> +.Fn tls_accept_cbs "struct tls *ctx" "struct tls **cctx" "ssize_t
> (*tls_read_cb)(struct *ctx, void *buf, size_t buflen, void *cb_arg)"
> "ssize_t (*tls_write_cb)(struct tls *ctx, const void *buf, size_t buflen,
> void *cb_arg)" "void *cb_arg"
>  .Ft "int"
>  .Fn tls_handshake "struct tls *ctx"
>  .Ft "ssize_t"
>

Re: [PATCH] Callback-based interface to libtls

[Ted Unangst]

Bob Beck wrote:
> > 
> > Agreed, I was also a bit unclear on payload at first (though it grew on
> > me over time, so I didn't change it). Here's an update with the
> > parameter renamed and better documented.
> > 
> > ok?
> 
> Yeah. I'm good with this
> 
> IMO get it in so we can tweak it in tree. 

first tweak: the context has a type: struct tls *, so use it.

Index: tls.h
===
RCS file: /cvs/src/lib/libtls/tls.h,v
retrieving revision 1.37
diff -u -p -r1.37 tls.h
--- tls.h   4 Sep 2016 14:15:44 -   1.37
+++ tls.h   5 Sep 2016 10:42:50 -
@@ -44,9 +44,9 @@ extern "C" {
 struct tls;
 struct tls_config;
 
-typedef ssize_t (*tls_read_cb)(void *_ctx, void *_buf, size_t _buflen,
+typedef ssize_t (*tls_read_cb)(struct tls *_ctx, void *_buf, size_t _buflen,
 void *_cb_arg);
-typedef ssize_t (*tls_write_cb)(void *_ctx, const void *_buf,
+typedef ssize_t (*tls_write_cb)(struct tls *_ctx, const void *_buf,
 size_t _buflen, void *_cb_arg);
 
 int tls_init(void);
Index: tls_init.3
===
RCS file: /cvs/src/lib/libtls/tls_init.3,v
retrieving revision 1.71
diff -u -p -r1.71 tls_init.3
--- tls_init.3  4 Sep 2016 16:37:18 -   1.71
+++ tls_init.3  5 Sep 2016 10:43:43 -
@@ -189,13 +189,13 @@
 .Ft "int"
 .Fn tls_connect_socket "struct tls *ctx" "int s" "const char *servername"
 .Ft "int"
-.Fn tls_connect_cbs "struct tls *ctx" "ssize_t (*tls_read_cb)(void *ctx, void 
*buf, size_t buflen, void *cb_arg)" "ssize_t (*tls_write_cb)(void *ctx, const 
void *buf, size_t buflen, void *cb_arg)" "void *cb_arg" "const char *servername"
+.Fn tls_connect_cbs "struct tls *ctx" "ssize_t (*tls_read_cb)(struct tls *ctx, 
void *buf, size_t buflen, void *cb_arg)" "ssize_t (*tls_write_cb)(struct tls 
*ctx, const void *buf, size_t buflen, void *cb_arg)" "void *cb_arg" "const char 
*servername"
 .Ft "int"
 .Fn tls_accept_fds "struct tls *tls" "struct tls **cctx" "int fd_read" "int 
fd_write"
 .Ft "int"
 .Fn tls_accept_socket "struct tls *tls" "struct tls **cctx" "int socket"
 .Ft "int"
-.Fn tls_accept_cbs "struct tls *ctx" "struct tls **cctx" "ssize_t 
(*tls_read_cb)(void *ctx, void *buf, size_t buflen, void *cb_arg)" "ssize_t 
(*tls_write_cb)(void *ctx, const void *buf, size_t buflen, void *cb_arg)" "void 
*cb_arg"
+.Fn tls_accept_cbs "struct tls *ctx" "struct tls **cctx" "ssize_t 
(*tls_read_cb)(struct *ctx, void *buf, size_t buflen, void *cb_arg)" "ssize_t 
(*tls_write_cb)(struct tls *ctx, const void *buf, size_t buflen, void *cb_arg)" 
"void *cb_arg"
 .Ft "int"
 .Fn tls_handshake "struct tls *ctx"
 .Ft "ssize_t"

memmove in memcpy

[Ted Unangst]

It seems we're sticking with the C memcpy for a while (which does the bounds
check and logging) but now we're missing out on the potential asm speedup.
Let's try the best of both worlds by having the C memcpy call into memmove.
Yes, it'll do another direction test, but then it will go zip zoom fast.
On certain somewhat popular architectures it can skip the alignment checks,
for instance.


Index: string/memcpy.c
===
RCS file: /cvs/src/lib/libc/string/memcpy.c,v
retrieving revision 1.2
diff -u -p -r1.2 memcpy.c
--- string/memcpy.c 31 Aug 2015 02:53:57 -  1.2
+++ string/memcpy.c 5 Sep 2016 10:05:50 -
@@ -36,26 +36,14 @@
 #include 
 
 /*
- * sizeof(word) MUST BE A POWER OF TWO
- * SO THAT wmask BELOW IS ALL ONES
- */
-typedeflong word;  /* "word" used for optimal copy speed */
-
-#definewsize   sizeof(word)
-#definewmask   (wsize - 1)
-
-/*
  * Copy a block of memory, not handling overlap.
  */
 void *
-memcpy(void *dst0, const void *src0, size_t length)
+memcpy(void *dst, const void *src, size_t length)
 {
-   char *dst = dst0;
-   const char *src = src0;
-   size_t t;
 
if (length == 0 || dst == src)  /* nothing to do */
-   goto done;
+   return dst;
 
if ((dst < src && dst + length > src) ||
(src < dst && src + length > dst)) {
@@ -65,36 +53,7 @@ memcpy(void *dst0, const void *src0, siz
abort();
}
 
-   /*
-* Macros: loop-t-times; and loop-t-times, t>0
-*/
-#defineTLOOP(s) if (t) TLOOP1(s)
-#defineTLOOP1(s) do { s; } while (--t)
+   return memmove(dst, src, length);
 
-   /*
-* Copy forward.
-*/
-   t = (long)src;  /* only need low bits */
-   if ((t | (long)dst) & wmask) {
-   /*
-* Try to align operands.  This cannot be done
-* unless the low bits match.
-*/
-   if ((t ^ (long)dst) & wmask || length < wsize)
-   t = length;
-   else
-   t = wsize - (t & wmask);
-   length -= t;
-   TLOOP1(*dst++ = *src++);
-   }
-   /*
-* Copy whole words, then mop up any trailing bytes.
-*/
-   t = length / wsize;
-   TLOOP(*(word *)dst = *(word *)src; src += wsize; dst += wsize);
-   t = length & wmask;
-   TLOOP(*dst++ = *src++);
-done:
-   return (dst0);
 }
 DEF_STRONG(memcpy);

Re: Remove /usr/share/misc/eqnchar

[Ingo Schwarze]

Hi Anthony,

Anthony J. Bentley wrote on Sun, Sep 04, 2016 at 08:40:47PM -0600:

> eqnchar is a collection of eqn(7) definitions to create mathematical
> symbols by constructing them from other characters. Creating circled
> plus with O, a backspace, and a plus, for example. The results are
> quite ugly in both mandoc and groff if it even works at all.
> 
> Nothing in base (or even anywhere?) uses these weird macros. Anyone
> doing mathematical typesetting will be much better served by reading
> through mandoc_char(7) or the Unicode Mathematical Operators block.

Indeed, this is quite definitely junk.

> ok?

OK schwarze@
  Ingo


> Index: Makefile
> ===
> RCS file: /cvs/src/share/misc/Makefile,v
> retrieving revision 1.13
> diff -u -p -r1.13 Makefile
> --- Makefile  25 Aug 2014 14:29:49 -  1.13
> +++ Makefile  5 Sep 2016 02:15:15 -
> @@ -1,7 +1,7 @@
>  #$OpenBSD: Makefile,v 1.13 2014/08/25 14:29:49 reyk Exp $
>  #from: @(#)Makefile  5.13 (Berkeley) 5/7/91
>  
> -FILES=   airport ascii birthtoken countrycodes eqnchar getopt \
> +FILES=   airport ascii birthtoken countrycodes getopt \
>   inter.phone license.template mdoc.template mime.types \
>   na.phone operator scsi_modes usb_hid_usages usb_hid_usages \
>   zipcodes 
> Index: eqnchar
> ===
> RCS file: eqnchar
> diff -N eqnchar
> --- eqnchar   18 Oct 1995 08:44:44 -  1.1.1.1
> +++ /dev/null 1 Jan 1970 00:00:00 -
> @@ -1,90 +0,0 @@
> -.EQ
> -tdefine ciplus % "\o'\(pl\(ci'" %
> -ndefine ciplus % O+ %
> -tdefine citimes % "\o'\(mu\(ci'" %
> -ndefine citimes % Ox %
> -tdefine =wig % 
> "\(eq\h'-\w'\(eq'u-\w'\s-2\(ap'u/2u'\v'-.4m'\s-2\z\(ap\(ap\s+2\v'.4m'\h'\w'\(eq'u-\w'\s-2\(ap'u/2u'"
>  %
> -ndefine =wig % ="~" %
> -tdefine bigstar % "\o'\(pl\(mu'" %
> -ndefine bigstar % X|- %
> -tdefine =dot % "\z\(eq\v'-.6m'\h'.2m'\s+2.\s-2\v'.6m'\h'.1m'" %
> -ndefine =dot % = dot %
> -tdefine orsign % "\s-2\v'-.15m'\z\e\e\h'-.05m'\z\(sl\(sl\v'.15m'\s+2" %
> -ndefine orsign % \e/ %
> -tdefine andsign % "\s-2\v'-.15m'\z\(sl\(sl\h'-.05m'\z\e\e\v'.15m'\s+2" %
> -ndefine andsign % /\e %
> -tdefine =del % "\v'.3m'\z=\v'-.6m'\h'.3m'\s-1\(*D\s+1\v'.3m'" %
> -ndefine =del % = to DELTA %
> -tdefine oppA % 
> "\s-2\v'-.15m'\z\e\e\h'-.05m'\z\(sl\(sl\v'-.15m'\h'-.75m'\z-\z-\h'.2m'\z-\z-\v'.3m'\h'.4m'\s+2"
>  %
> -ndefine oppA % V- %
> -tdefine oppE 
> %"\s-3\v'.2m'\z\(em\v'-.5m'\z\(em\v'-.5m'\z\(em\v'.55m'\h'.9m'\z\(br\z\(br\v'.25m'\s+3"
>  %
> -ndefine oppE % E/ %
> -tdefine incl % 
> "\s-1\z\(or\h'-.1m'\v'-.45m'\z\(em\v'.7m'\z\(em\v'.2m'\(em\v'-.45m'\s+1" %
> -ndefine incl % C_ %
> -tdefine nomem % "\o'\(mo\(sl'" %
> -ndefine nomem % C-/ %
> -tdefine angstrom % "\fR\zA\v'-.3m'\h'.2m'\(de\v'.3m'\fP\h'.2m'" %
> -ndefine angstrom % A to o %
> -tdefine star %{ roman "\v'.5m'\s+3*\s-3\v'-.5m'"}%
> -ndefine star % * %
> -tdefine || % \(or\(or %
> -tdefine  -ndefine  -tdefine >wig % "\z>\v'.4m'\(ap\v'-.4m'" %
> -ndefine >wig %{ > from "~" }%
> -tdefine langle % "\s-3\b'\(sl\e'\s0" %
> -ndefine langle %<%
> -tdefine rangle % "\s-3\b'\e\(sl'\s0" %
> -ndefine rangle %>%
> -tdefine hbar % "\zh\v'-.6m'\h'.05m'\(ru\v'.6m'" %
> -ndefine hbar % h\u-\d %
> -ndefine ppd % _| %
> -tdefine ppd % "\o'\(ru\s-2\(or\s+2'" %
> -tdefine <-> % "\o'\(<-\(->'" %
> -ndefine <-> % "<-->" %
> -tdefine <=> % "\s-2\z<\v'.05m'\h'.2m'\z=\h'.55m'=\h'-.6m'\v'-.05m'>\s+2" %
> -ndefine <=> % "<=>" %
> -tdefine |< % "\o'<\(or'" %
> -ndefine |< % <| %
> -tdefine |> % "\o'>\(or'" %
> -ndefine |> % |> %
> -tdefine ang % "\v'-.15m'\z\s-2\(sl\s+2\v'.15m'\(ru" %
> -ndefine ang % /_ %
> -tdefine rang % "\z\(or\h'.15m'\(ru" %
> -ndefine rang % L %
> -tdefine 3dot % "\v'-.8m'\z.\v'.5m'\z.\v'.5m'.\v'-.2m'" %
> -ndefine 3dot % .\u.\u.\d\d %
> -tdefine thf % ".\v'-.5m'.\v'.5m'." %
> -ndefine thf % ..\u.\d %
> -tdefine quarter % roman \(14 %
> -ndefine quarter % 1/4 %
> -tdefine 3quarter % roman \(34 %
> -ndefine 3quarter % 3/4 %
> -tdefine degree % \(de %
> -ndefine degree % nothing sup o %
> -tdefine square % \(sq %
> -ndefine square % [] %
> -tdefine circle % \(ci %
> -ndefine circle % O %
> -tdefine blot % "\fB\(sq\fP" %
> -ndefine blot % HIX %
> -tdefine bullet % \(bu %
> -ndefine bullet % oxe %
> -tdefine -wig % "\(~=" %
> -ndefine -wig % - to "~" %
> -tdefine wig % \(ap %
> -ndefine wig % "~" %
> -tdefine prop % \(pt %
> -ndefine prop % oc %
> -tdefine empty % \(es %
> -ndefine empty % O/ %
> -tdefine member % \(mo %
> -ndefine member % C- %
> -tdefine cup % \(cu %
> -ndefine cup % U %
> -define cap % \(ca %
> -define subset % \(sb %
> -define supset % \(sp %
> -define !subset % \(ib %
> -define !supset % \(ip %
> -.EN

Re: Drop main() prototype

[Marc Espie]

On Mon, Sep 05, 2016 at 04:04:23AM -0400, Ted Unangst wrote:
> Sevan Janiyan wrote:
> > Hello,
> > Attached patches remove the main() prototype from
> > src/{sbin,usr.bin,usb.sbin}
> 
> yes!

I'm 100% certain I added those prototypes because some version of gcc
with the appropriate warning flags would flag main as missing a prototype.

If that era is now gone, I'm all too happy for it to vanish.

Re: mount(8): strlen + malloc + snprintf == asprintf

[Marc Espie]

Are you retarded ?

Go study the source code.

Re: Sync getopt with getopt

[Ingo Schwarze]

Hi,

Anthony J. Bentley wrote on Mon, Sep 05, 2016 at 02:05:57AM -0600:
> "Ted Unangst" writes:
>> Todd C. Miller wrote:
>>> On Sun, 04 Sep 2016 11:58:23 -0600, "Anthony J. Bentley" wrote:

 This brings /usr/share/misc/getopt in sync with the example in getopt(3).

>>> OK, though I wonder if anyone actually looks at this file?

>> i think it's better to delete it.

> Works for me.

Anthony, please do delete it.
OK schwarze@ for that.

If people want examples, they should look at real production code that
is being maintained, not at dummy files that everybody forgets about.

In some cases, an example in the manual page may be OK.
But not in such a stale random file in some shady corner.

Yours,
  Ingo

Re: mount(8): strlen + malloc + snprintf == asprintf

[Tom Cosgrove]

>>> Ali H. Fardan  5-Sep-16 09:09 >>>
>
> On 2016-09-05 11:03, Tom Cosgrove wrote:
> :
> > It does allocate the correct buffer size.  It's got all the
> > information it needs to do that with the format string and the
> > parameters.  Then it returns the buffer address via the `ret'
> > argument.
> >
> > If you don't believe us, read the source code and tell us where we
> > are wrong.
> > 
> > Tom
>
> then that patch does weaken security, the buffer can overflow.

asprintf() allocates the buffer, of the size it needs.  It can't overflow.
It makes no change to security.

The patch is fine - you'll notice it's already been committed.

Tom

Re: mount(8): strlen + malloc + snprintf == asprintf

[Martijn van Duren]

On 09/05/16 10:06, Ali H. Fardan wrote:
> On 2016-09-05 11:04, Otto Moerbeek wrote:
>> On Mon, Sep 05, 2016 at 10:47:06AM +0300, Ali H. Fardan wrote:
>>
>>> On 2016-09-05 10:44, David Gwynne wrote:
> On 5 Sep 2016, at 17:39, Ali H. Fardan  wrote:
>
> and why is he telling me this? I just said if the destination is a
> pointer to char, how would a function automagically allocate a size
> for it?

 its not a pointer to a char, its a pointer to a char pointer:

 as per the man page:

  int
  asprintf(char **ret, const char *format, ...);

 dlg

>>>
>>> Still doesn't mean that it can automagically allocate a correct
>>> buffer size.
>>
>> Yes it does.
>>
>> Arguing about this doesn't help anybody. Go study some C.
>>
>>  -Otto
> 
> You got no explanation for your argument.
> 
http://man.openbsd.org/asprintf
/usr/src/lib/libc/stdio/asprintf.c
/usr/src/lib/libc/stdio/vfprintf.c

Come back when you find the bug.
Patches welcome.

Re: mount(8): strlen + malloc + snprintf == asprintf

[Ali H. Fardan]

On 2016-09-05 11:03, Tom Cosgrove wrote:

Ali H. Fardan  5-Sep-16 08:47 >>>


On 2016-09-05 10:44, David Gwynne wrote:
>> On 5 Sep 2016, at 17:39, Ali H. Fardan  wrote:
>>
>> and why is he telling me this? I just said if the destination is a
>> pointer to char, how would a function automagically allocate a size
>> for it?
>
> its not a pointer to a char, its a pointer to a char pointer:
>
> as per the man page:
>
>  int
>  asprintf(char **ret, const char *format, ...);
>
> dlg

Still doesn't mean that it can automagically allocate a correct
buffer size.


It does allocate the correct buffer size.  It's got all the information 
it
needs to do that with the format string and the parameters.  Then it 
returns

the buffer address via the `ret' argument.

If you don't believe us, read the source code and tell us where we are 
wrong.


Tom


then that patch does weaken security, the buffer can overflow.

Re: mount(8): strlen + malloc + snprintf == asprintf

[Tom Cosgrove]

>>> Ali H. Fardan  5-Sep-16 08:47 >>>
>
> On 2016-09-05 10:44, David Gwynne wrote:
> >> On 5 Sep 2016, at 17:39, Ali H. Fardan  wrote:
> >> 
> >> and why is he telling me this? I just said if the destination is a
> >> pointer to char, how would a function automagically allocate a size
> >> for it?
> > 
> > its not a pointer to a char, its a pointer to a char pointer:
> > 
> > as per the man page:
> > 
> >  int
> >  asprintf(char **ret, const char *format, ...);
> > 
> > dlg
>
> Still doesn't mean that it can automagically allocate a correct
> buffer size.

It does allocate the correct buffer size.  It's got all the information it
needs to do that with the format string and the parameters.  Then it returns
the buffer address via the `ret' argument.

If you don't believe us, read the source code and tell us where we are wrong.

Tom

Re: mount(8): strlen + malloc + snprintf == asprintf

[Janne Johansson]

The 3 lines of code it replaced could, so why would you not believe
asprintf() couldn't ?


2016-09-05 9:47 GMT+02:00 Ali H. Fardan :

> On 2016-09-05 10:44, David Gwynne wrote:
>
>> On 5 Sep 2016, at 17:39, Ali H. Fardan  wrote:
>>>
>>> and why is he telling me this? I just said if the destination is a
>>> pointer to char, how would a function automagically allocate a size
>>> for it?
>>>
>>
>> its not a pointer to a char, its a pointer to a char pointer:
>>
>> as per the man page:
>>
>>  int
>>  asprintf(char **ret, const char *format, ...);
>>
>> dlg
>>
>>
> Still doesn't mean that it can automagically allocate a correct
> buffer size.
>
>


-- 
May the most significant bit of your life be positive.

Re: mount(8): strlen + malloc + snprintf == asprintf

[Ali H. Fardan]

On 2016-09-05 11:04, Otto Moerbeek wrote:

On Mon, Sep 05, 2016 at 10:47:06AM +0300, Ali H. Fardan wrote:


On 2016-09-05 10:44, David Gwynne wrote:
> > On 5 Sep 2016, at 17:39, Ali H. Fardan  wrote:
> >
> > and why is he telling me this? I just said if the destination is a
> > pointer to char, how would a function automagically allocate a size
> > for it?
>
> its not a pointer to a char, its a pointer to a char pointer:
>
> as per the man page:
>
>  int
>  asprintf(char **ret, const char *format, ...);
>
> dlg
>

Still doesn't mean that it can automagically allocate a correct
buffer size.


Yes it does.

Arguing about this doesn't help anybody. Go study some C.

-Otto


You got no explanation for your argument.

Re: Sync getopt with getopt

[Anthony J. Bentley]

"Ted Unangst" writes:
> Todd C. Miller wrote:
> > On Sun, 04 Sep 2016 11:58:23 -0600, "Anthony J. Bentley" wrote:
> > 
> > > This brings /usr/share/misc/getopt in sync with the example in getopt(3).
> > 
> > OK, though I wonder if anyone actually looks at this file?
> 
> i think it's better to delete it.

Works for me.

Re: Drop main() prototype

[Ted Unangst]

Sevan Janiyan wrote:
> Hello,
> Attached patches remove the main() prototype from
> src/{sbin,usr.bin,usb.sbin}

yes!

Re: mount(8): strlen + malloc + snprintf == asprintf

[Otto Moerbeek]

On Mon, Sep 05, 2016 at 10:47:06AM +0300, Ali H. Fardan wrote:

> On 2016-09-05 10:44, David Gwynne wrote:
> > > On 5 Sep 2016, at 17:39, Ali H. Fardan  wrote:
> > > 
> > > and why is he telling me this? I just said if the destination is a
> > > pointer to char, how would a function automagically allocate a size
> > > for it?
> > 
> > its not a pointer to a char, its a pointer to a char pointer:
> > 
> > as per the man page:
> > 
> >  int
> >  asprintf(char **ret, const char *format, ...);
> > 
> > dlg
> > 
> 
> Still doesn't mean that it can automagically allocate a correct
> buffer size.

Yes it does.

Arguing about this doesn't help anybody. Go study some C.

-Otto

Re: Sync getopt with getopt

[Ted Unangst]

Todd C. Miller wrote:
> On Sun, 04 Sep 2016 11:58:23 -0600, "Anthony J. Bentley" wrote:
> 
> > This brings /usr/share/misc/getopt in sync with the example in getopt(3).
> 
> OK, though I wonder if anyone actually looks at this file?

i think it's better to delete it.

Re: mount(8): strlen + malloc + snprintf == asprintf

[Ali H. Fardan]

On 2016-09-05 10:44, David Gwynne wrote:

On 5 Sep 2016, at 17:39, Ali H. Fardan  wrote:

and why is he telling me this? I just said if the destination is a
pointer to char, how would a function automagically allocate a size
for it?


its not a pointer to a char, its a pointer to a char pointer:

as per the man page:

 int
 asprintf(char **ret, const char *format, ...);

dlg



Still doesn't mean that it can automagically allocate a correct
buffer size.

Re: mount(8): strlen + malloc + snprintf == asprintf

[David Gwynne]

> On 5 Sep 2016, at 17:39, Ali H. Fardan  wrote:
> 
> and why is he telling me this? I just said if the destination is a
> pointer to char, how would a function automagically allocate a size
> for it?

its not a pointer to a char, its a pointer to a char pointer:

as per the man page:

 int
 asprintf(char **ret, const char *format, ...);

dlg

> 
>  Original Message 
> Subject: Re: mount(8): strlen + malloc + snprintf == asprintf
> Date: 2016-09-05 10:36
> From: "Michael W. Bombardieri" 
> To: "Ali H. Fardan" , Otto Moerbeek 
> Cc: David Gwynne , tech , 
> owner-t...@openbsd.org
> 
> FWIW the reply seemed like a proper statement to me.
> 
> The manual page for asprintf() doesn't explain its internals. Do you expect 
> someone to give you a summary of asprintf() internals? I don't see why they 
> should.
> 
> On 2016-09-05 3:15 PM, Ali H. Fardan wrote:
>> On 2016-09-05 08:52, Otto Moerbeek wrote:
>>> On Mon, Sep 05, 2016 at 08:05:40AM +0300, Ali H. Fardan wrote:
 On 2016-09-05 08:01, David Gwynne wrote:
 > > On 5 Sep 2016, at 12:13, Ali H. Fardan  wrote:
 > >
 > > You can't specify a buffer size in asprintf() therefore, it is not
 > > secure,
 > > you can see that snprintf() does write to the `i` bytes to the buffer
 >
 > asprintf allocates the memory it needs to write to, unlike snprintf
 > which requires a preallocated buffer.
 when the destination is a pointer to a char, and the passed argument is a
 memory address, how is it supposed to determine the correct buffer size?
 Raiz
>>> asprintf uses the internals of the printf family of functions. Look in
>>> src/lib/libc/stdio for all the details.
>>>-Otto
>> If you can read my statement and reply with a proper statement,
>> I'd appreciate it.
>> Raiz
> 

Fwd: Re: mount(8): strlen + malloc + snprintf == asprintf

[Ali H. Fardan]

and why is he telling me this? I just said if the destination is a
pointer to char, how would a function automagically allocate a size
for it?

 Original Message 
Subject: Re: mount(8): strlen + malloc + snprintf == asprintf
Date: 2016-09-05 10:36
From: "Michael W. Bombardieri" 
To: "Ali H. Fardan" , Otto Moerbeek 
Cc: David Gwynne , tech , 
owner-t...@openbsd.org


FWIW the reply seemed like a proper statement to me.

The manual page for asprintf() doesn't explain its internals. Do you 
expect someone to give you a summary of asprintf() internals? I don't 
see why they should.


On 2016-09-05 3:15 PM, Ali H. Fardan wrote:

On 2016-09-05 08:52, Otto Moerbeek wrote:

On Mon, Sep 05, 2016 at 08:05:40AM +0300, Ali H. Fardan wrote:


On 2016-09-05 08:01, David Gwynne wrote:
> > On 5 Sep 2016, at 12:13, Ali H. Fardan  wrote:
> >
> > You can't specify a buffer size in asprintf() therefore, it is not
> > secure,
> > you can see that snprintf() does write to the `i` bytes to the buffer
>
> asprintf allocates the memory it needs to write to, unlike snprintf
> which requires a preallocated buffer.

when the destination is a pointer to a char, and the passed argument 
is a
memory address, how is it supposed to determine the correct buffer 
size?


Raiz


asprintf uses the internals of the printf family of functions. Look in
src/lib/libc/stdio for all the details.

-Otto


If you can read my statement and reply with a proper statement,
I'd appreciate it.

Raiz

Re: mount(8): strlen + malloc + snprintf == asprintf

[Ted Unangst]

Ali H. Fardan wrote:
> If you can read my statement and reply with a proper statement,
> I'd appreciate it.

You are wrong.

Re: mount(8): strlen + malloc + snprintf == asprintf

[Ali H. Fardan]

On 2016-09-05 08:52, Otto Moerbeek wrote:

On Mon, Sep 05, 2016 at 08:05:40AM +0300, Ali H. Fardan wrote:


On 2016-09-05 08:01, David Gwynne wrote:
> > On 5 Sep 2016, at 12:13, Ali H. Fardan  wrote:
> >
> > You can't specify a buffer size in asprintf() therefore, it is not
> > secure,
> > you can see that snprintf() does write to the `i` bytes to the buffer
>
> asprintf allocates the memory it needs to write to, unlike snprintf
> which requires a preallocated buffer.

when the destination is a pointer to a char, and the passed argument 
is a
memory address, how is it supposed to determine the correct buffer 
size?


Raiz


asprintf uses the internals of the printf family of functions. Look in
src/lib/libc/stdio for all the details.

-Otto


If you can read my statement and reply with a proper statement,
I'd appreciate it.

Raiz

manpage for new rbtree code

[David Gwynne]

put it in and shine it in the tree?

Index: Makefile
===
RCS file: /cvs/src/share/man/man9/Makefile,v
retrieving revision 1.279
diff -u -p -r1.279 Makefile
--- Makefile2 Sep 2016 17:36:54 -   1.279
+++ Makefile5 Sep 2016 07:07:47 -
@@ -26,6 +26,7 @@ MAN=  aml_evalnode.9 atomic_add_int.9 ato
namei.9 \
panic.9 pci_conf_read.9 pci_intr_map.9 pfind.9 physio.9 pmap.9 \
pool.9 ppsratecheck.9 printf.9 psignal.9 \
+   RBT_INIT.9 \
radio.9 arc4random.9 rasops.9 ratecheck.9 refcnt_init.9 resettodr.9 \
rssadapt.9 route.9 rt_ifa_add.9 rt_timer_add.9 rtalloc.9 rtable_add.9 \
rtlabel_id2name.9 rtrequest.9 rwlock.9 SipHash24.9 sensor_attach.9 \
Index: RBT_INIT.9
===
RCS file: RBT_INIT.9
diff -N RBT_INIT.9
--- /dev/null   1 Jan 1970 00:00:00 -
+++ RBT_INIT.9  5 Sep 2016 07:07:47 -
@@ -0,0 +1,497 @@
+.\"$OpenBSD$
+.\"
+.\" * Copyright 2002 Niels Provos 
+.\" * All rights reserved.
+.\" *
+.\" * Redistribution and use in source and binary forms, with or without
+.\" * modification, are permitted provided that the following conditions
+.\" * are met:
+.\" * 1. Redistributions of source code must retain the above copyright
+.\" *notice, this list of conditions and the following disclaimer.
+.\" * 2. Redistributions in binary form must reproduce the above copyright
+.\" *notice, this list of conditions and the following disclaimer in the
+.\" *documentation and/or other materials provided with the distribution.
+.\" *
+.\" * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
+.\" * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+.\" * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
+.\" * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
+.\" * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+.\" * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+.\" * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+.\" * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+.\" * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+.\" * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+.\" */
+.\"
+.\" Copyright (c) 2016 David Gwynne 
+.\"
+.\" Permission to use, copy, modify, and distribute this software for any
+.\" purpose with or without fee is hereby granted, provided that the above
+.\" copyright notice and this permission notice appear in all copies.
+.\"
+.\" THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+.\" WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+.\" MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+.\" ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+.\" WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+.\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+.\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+.\"
+.Dd $Mdocdate: September 11 2015 $
+.Dt RBT_INIT 9
+.Os
+.Sh NAME
+.Nm RBT_HEAD ,
+.Nm RBT_ENTRY ,
+.Nm RBT_PROTOTYPE ,
+.Nm RBT_GENERATE ,
+.Nm RBT_INITIALIZER ,
+.Nm RBT_INIT ,
+.Nm RBT_INSERT ,
+.Nm RBT_REMOVE ,
+.Nm RBT_FIND ,
+.Nm RBT_NFIND ,
+.Nm RBT_EMPTY ,
+.Nm RBT_ROOT ,
+.Nm RBT_MIN ,
+.Nm RBT_MAX ,
+.Nm RBT_NEXT ,
+.Nm RBT_PREV ,
+.Nm RBT_LEFT ,
+.Nm RBT_RIGHT ,
+.Nm RBT_PARENT ,
+.Nm RBT_FOREACH ,
+.Nm RBT_FOREACH_SAFE ,
+.Nm RBT_FOREACH_REVERSE ,
+.Nm RBT_FOREACH_REVERSE_SAFE
+.Nd Kernel red-black trees
+.Sh SYNOPSIS
+.In sys/tree.h
+.Fn RBT_HEAD "NAME" "TYPE"
+.Fn RBT_ENTRY "TYPE"
+.Fo RBT_PROTOTYPE
+.Fa "NAME"
+.Fa "TYPE"
+.Fa "ENTRY"
+.Fa "int (*compare)(const struct TYPE *, const struct TYPE *)"
+.Fc
+.Fo RBT_GENERATE
+.Fa "NAME"
+.Fa "TYPE"
+.Fa "ENTRY"
+.Fa "int (*compare)(const struct TYPE *, const struct TYPE *)"
+.Fc
+.Ft struct NAME
+.Fn RBT_INITIALIZER "struct NAME *self"
+.Ft void
+.Fn RBT_INIT "NAME" "struct NAME *rbt"
+.Ft struct TYPE *
+.Fn RBT_INSERT "NAME" "struct NAME *rbt" "struct TYPE *elm"
+.Ft struct TYPE *
+.Fn RBT_REMOVE "NAME" "struct NAME *rbt" "struct TYPE *elm"
+.Ft struct TYPE *
+.Fn RBT_FIND "NAME" "struct NAME *rbt" "const struct TYPE *key"
+.Ft struct TYPE *
+.Fn RBT_NFIND "NAME" "struct NAME *rbt" "const struct TYPE *key"
+.Ft int
+.Fn RBT_EMPTY "NAME" "struct NAME *rbt"
+.Ft struct TYPE *
+.Fn RBT_ROOT "NAME" "struct NAME *rbt"
+.Ft struct TYPE *
+.Fn RBT_MIN "NAME" "struct NAME *rbt"
+.Ft struct TYPE *
+.Fn RBT_MAX "NAME" "struct NAME *rbt"
+.Ft struct TYPE *
+.Fn RBT_NEXT "NAME" "struct NAME *rbt"
+.Ft struct TYPE *
+.Fn RBT_PREV "NAME" "struct NAME *rbt"
+.Ft struct TYPE *
+.Fn RBT_LEFT "NAME" "struct NAME *rbt"
+.Ft struct TYPE *
+.Fn RBT_RIGHT