Re: xclock patch

[Sebastien Marie]

On Fri, May 06, 2016 at 01:29:11PM -0500, Edgar Pettijohn wrote:
> > On May 6, 2016, at 12:13 PM, Matthieu Herrb  wrote:
> > 
> >> On Tue, May 03, 2016 at 11:40:31AM -0500, Edgar Pettijohn wrote:
> >> I'll look at it this evening.
> > 
> > Ping...
> > 
> 
> Sorry. I tried to get it to reproduce and couldn't. Looked through the code 
> and couldn't find anything. 
> 

if you still have the xclock.core file (and the same xclock binary), you
could get a backtrace using gdb.


else, if it isn't in xclock code, it could be a open(2) call in some
X11R6 library.

One possibility is error code path: in xterm, the commit message for
justifying "rpath" is: for X11 error ("X Error of failed request: ...")
which read at least /usr/X11R6/share/X11/XErrorDB.

Maybe we should identify correctly these open(2) call in X11R6 libraries ?
-- 
Sebastien Marie

Re: current trunk is not accepting tun / link0

[sven falempin]

On Fri, May 6, 2016 at 3:27 PM, Stuart Henderson 
wrote:

> On 2016/05/06 15:19, sven falempin wrote:
> > Claudio Jeker : 10 years ago
> > ...
> > trunk(4) works only over ethernet devices (more precisely IEEE802 based
> > interfaces). This includes wireless devices but neither of gif, gre or
> > pppoe. tun(4) in layer 2 mode works while a "normal" tun(4) will not.
> >
> > Looks like it s broken on layer 2 tun.
>
> Layer 2 tun has been replaced. See the release notes and upgrade guide.
>

Does that mean link0 is completly useless now ? (because i can ifconfig
tapX link0)

Also looks like openvpn does not UP the tap interface.

Anyone checked ?

-- 
-
() ascii ribbon campaign - against html e-mail
/\

Re: current trunk is not accepting tun / link0

[sven falempin]

On Fri, May 6, 2016 at 3:27 PM, Stuart Henderson 
wrote:

> On 2016/05/06 15:19, sven falempin wrote:
> > Claudio Jeker : 10 years ago
> > ...
> > trunk(4) works only over ethernet devices (more precisely IEEE802 based
> > interfaces). This includes wireless devices but neither of gif, gre or
> > pppoe. tun(4) in layer 2 mode works while a "normal" tun(4) will not.
> >
> > Looks like it s broken on layer 2 tun.
>
> Layer 2 tun has been replaced. See the release notes and upgrade guide.
>

yes jut notice the <

Split up tun(4) into tun(4) and tap(4).

>

Sorry

-- 
-
() ascii ribbon campaign - against html e-mail
/\

Re: dhclient: use /dev/bpf

[Alexander Hall]

On May 6, 2016 8:17:50 PM GMT+02:00, Stuart Henderson  
wrote:
>On 2016/05/06 19:35, Martin Natano wrote:
>> On Thu, May 05, 2016 at 09:30:43PM +0100, Stuart Henderson wrote:
>> > On 2016/05/02 09:34, Martin Natano wrote:
>> > > Diff below simplifies the device open path and removes an
>explanation
>> > > about bpf device nodes from the manpage.
>> > 
>> > There's a problem with this. If someone is doing an "untar sets"
>> > upgrade (which is not _supported_ but is pretty much necessary in
>> > some situations) and relies on dhclient for network to get back
>> > in to the system, they lose.
>> > 
>> > Since they won't have a new MAKEDEV at the right time, I think
>> > our options are either "change dhclient back to using /dev/bpf0"
>> > or "tell them to run mknod manually before updating".
>> 
>> Adding this information to current.html seems like the way to go to
>me.
>> I see you and tb@ have already done so (with instructions to run
>MAKEDEV
>> after unpacking the sets), thanks!
>
>What we added to current.html is good enough for things like
>pflogd etc., but it doesn't go far enough for the dhclient case.
>In many cases (especially when people move from 5.9 to 6.0)
>you won't be able to run MAKEDEV until *after* you've rebooted
>due to pledge/syscall changes.

Heh, indeed. I was severely bitten by that in an upgrade when I, as part of my 
normal online upgrade procedure, ran MAKEDEV before reboot, which succeeded in 
removing the old devices but failed to create the new ones... 

My upgrade script got upgraded after that. :-)

/Alexander 

>
>Personally, my preferred course of action would be to switch to
>looking for /dev/bpf0 for now (bpf0 only: *not* revert to the old
>loop, that's not necessary), and then switch to /dev/bpf
>immediately after 6.0 is released. The recent few version
>upgrades have been relatively painful, with good reason, but
>this is something where we can easily reduce the pain with
>minimal downside.

Re: [ksh] [patch] Make "$@" POSIX-compliant with empty IFS

[Martijn Dekker]

Op 24-03-16 om 04:04 schreef Theo Buehler:

On Fri, Mar 04, 2016 at 11:29:38AM +0100, Dmitrij D. Czarkoff wrote:

Martijn Dekker said:

So this patch makes quoted "$@" act according to the standard even when
IFS is empty. Quoted "$*" is unchanged. For the unspecified (not
standardised) cases of unquoted $@ and $*, this patch makes ksh act like
AT&T ksh93, bash, zsh and (d)ash, which seems safest from a
compatibility point of view.


This makes sense to me, and changes seem reasonable.  I've re-generated
diff against -current.  Anyone willing to OK it?


I think this is ok.

However, since this patch is rather subtle, it would be nice if someone
more experienced could look this over as well. Note that this appears
to have been in the snapshots for a while.


Now it's out of the snapshots again, but it wasn't applied. Has it been 
rejected for some reason?


If not, is there anyone more experienced who could look this over? (See 
below.)



@@ -420,6 +446,7 @@ expand(char *cp,/* input word */
if (f&DOBLANK)
doblank++;
st = st->prev;
+   word = quote || (!*x.str) ? IFS_WORD : 
IFS_IWS;


This line would need some wrapping. While I'm here: the parens are a bit
strange. Since the precedence here is not completely obvious anyway, I
think they are more confusing than helpful. Why not

word = (quote || !*x.str) ? IFS_WORD : 
IFS_IWS;

or even

if (quote || !*x.str)
word = IFS_WORD;
else
word = IFS_IWS;



I agree. The latter version is much easier to read. Here is a new diff 
against -current with it applied.


Thanks,

- M.

Index: bin/ksh/eval.c
===
RCS file: /cvs/src/bin/ksh/eval.c,v
retrieving revision 1.50
diff -u -p -r1.50 eval.c
--- bin/ksh/eval.c	5 Mar 2016 12:30:17 -	1.50
+++ bin/ksh/eval.c	6 May 2016 20:05:57 -
@@ -47,6 +47,8 @@ typedef struct Expand {
 #define IFS_WORD	0	/* word has chars (or quotes) */
 #define IFS_WS		1	/* have seen IFS white-space */
 #define IFS_NWS		2	/* have seen IFS non-white-space */
+#define IFS_IWS		3	/* beginning of word, ignore IFS white-space */
+#define IFS_QUOTE	4	/* beg.w/quote, becomes IFS_WORD unless "$@" */
 
 static	int	varsub(Expand *, char *, char *, int *, int *);
 static	int	comsub(Expand *, char *);
@@ -215,7 +217,17 @@ expand(char *cp,	/* input word */
 c = *sp++;
 break;
 			case OQUOTE:
-word = IFS_WORD;
+switch (word) {
+case IFS_QUOTE:
+	/* """something */
+	word = IFS_WORD;
+	break;
+case IFS_WORD:
+	break;
+default:
+	word = IFS_QUOTE;
+	break;
+}
 tilde_ok = 0;
 quote = 1;
 continue;
@@ -295,6 +307,8 @@ expand(char *cp,	/* input word */
 if (f&DOBLANK)
 	doblank++;
 tilde_ok = 0;
+if (word == IFS_QUOTE && type != XNULLSUB)
+	word = IFS_WORD;
 if (type == XBASE) {	/* expand? */
 	if (!st->next) {
 		SubType *newst;
@@ -356,6 +370,11 @@ expand(char *cp,	/* input word */
 		f |= DOTEMP_;
 		/* FALLTHROUGH */
 	default:
+		/* '-' '+' '?' */
+		if (quote)
+			word = IFS_WORD;
+		else if (dp == Xstring(ds, dp))
+			word = IFS_IWS;
 		/* Enable tilde expansion */
 		tilde_ok = 1;
 		f |= DOTILDE;
@@ -385,10 +404,17 @@ expand(char *cp,	/* input word */
 	 */
 	x.str = trimsub(str_val(st->var),
 		dp, st->stype);
-	if (x.str[0] != '\0' || st->quote)
+	if (x.str[0] != '\0') {
+		word = IFS_IWS;
 		type = XSUB;
-	else
+	} else if (quote) {
+		word = IFS_WORD;
+		type = XSUB;
+	} else {
+		if (dp == Xstring(ds, dp))
+			word = IFS_IWS;
 		type = XNULLSUB;
+	}
 	if (f&DOBLANK)
 		doblank++;
 	st = st->prev;
@@ -420,6 +446,10 @@ expand(char *cp,	/* input word */
 	if (f&DOBLANK)
 		doblank++;
 	st = st->prev;
+	if (quote || !*x.str)
+		word = IFS_WORD;
+	else
+		word = IFS_IWS;
 	continue;
 case '?':
 {
@@ -461,12 +491,8 @@ expand(char *cp,	/* input word */
 			type = XBASE;
 			if (f&DOBLANK) {
 doblank--;
-/* not really correct: x=; "$x$@" should
- * generate a null argument and
- * set A; "${@:+}" shouldn't.
- */
-if (dp == Xstring(ds, dp))
-	word = IFS_WS;
+if (dp == Xstring(ds, dp) && word != IFS_WORD)
+	word = IFS_IWS;
 			}
 			continue;
 
@@ -501,7 +527,12 @@ expand(char *cp,	/* input word */
 if (c == 0) {
 	if (quote && !x.split)
 		continue;
+	if (!quote && word == IFS_WS)
+		continue;
+	/* this is so we don't terminate */
 	c = ' ';
+	/* now force-emit a word */
+	goto emit_word

Re: httpd: patch for portability asprintf use

[Todd C. Miller]

On Fri, 06 May 2016 18:53:28 +0200, Hiltjo Posthuma wrote:

> It is not an issue, but I thought it was not intended because the
> asprintf(3) documentation said:
> 
> "The value of ret in this situation is
> implementation-dependent (on OpenBSD, ret will be set to the null
> pointer, >but this behavior should not be relied upon<)."

Some time ago there was talk of changing the documented value of
ret on error in glibc, but it doesn't seem to have happened.

In light of that, it is probably best to explicitly NULL out the
value as per your patch.

 - todd

rtadvd.conf.5: document route preference flags

[Jeremie Courreges-Anglas]

Also motivated by http://marc.info/?l=openbsd-misc&m=146239072929264&w=2

RFC4191 defines route preference flags not only in Route Information
options, but also in Router Advertisement messages.  Let's try to make
this clear.  Also, try to document the possible values in a slightly
more useful way...

Comments / oks?

Index: rtadvd.conf.5
===
RCS file: /cvs/src/usr.sbin/rtadvd/rtadvd.conf.5,v
retrieving revision 1.35
diff -u -p -p -u -r1.35 rtadvd.conf.5
--- rtadvd.conf.5   21 Apr 2015 16:32:24 -  1.35
+++ rtadvd.conf.5   6 May 2016 19:25:15 -
@@ -116,6 +116,22 @@ and Bit 6
 .Li 0x40
 .Pc
 means Other stateful configuration flag bit.
+Bit 4
+.Po
+.Li 0x10
+.Pc
+and
+Bit 3
+.Po
+.Li 0x08
+.Pc
+are used to encode the route preference for the route as follows:
+.Bl -column "0x08" "High" -offset indent
+.It 0x08 Ta "High"
+.It 0x00 Ta "Medium" Pq default
+.It 0x18 Ta "Low"
+.El
+.Pp
 The default value is 0.
 .It Cm \&rltime
 (num) Router lifetime field
@@ -252,18 +268,7 @@ The default value is 64.
 .It Cm \&rtflags
 (str or num) A 8-bit flags field in route information option.
 Currently only the preference values are defined.
-The notation is same as that of the raflags field.
-Bit 4
-.Po
-.Li 0x10
-.Pc
-and
-Bit 3
-.Po
-.Li 0x08
-.Pc
-are used to encode the route preference for the route.
-The default value is 0x00, i.e., medium preference.
+The notation for those is same as that of the raflags field.
 .It Cm \&rtltime
 (num) route lifetime field in route information option.
 .Pq unit: seconds .

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

Re: current trunk is not accepting tun / link0

[Stuart Henderson]

On 2016/05/06 15:19, sven falempin wrote:
> Claudio Jeker : 10 years ago
> ...
> trunk(4) works only over ethernet devices (more precisely IEEE802 based
> interfaces). This includes wireless devices but neither of gif, gre or
> pppoe. tun(4) in layer 2 mode works while a "normal" tun(4) will not.
> 
> Looks like it s broken on layer 2 tun.

Layer 2 tun has been replaced. See the release notes and upgrade guide.

current trunk is not accepting tun / link0

[sven falempin]

Claudio Jeker : 10 years ago
...
trunk(4) works only over ethernet devices (more precisely IEEE802 based
interfaces). This includes wireless devices but neither of gif, gre or
pppoe. tun(4) in layer 2 mode works while a "normal" tun(4) will not.

Looks like it s broken on layer 2 tun.

# uname -a
OpenBSD current 5.9 GENERIC.MP#2004 amd64
[0]-[current]-[~]
# ifconfig tun1
tun1: flags=9051 mtu 1500
index 81 priority 0
groups: tun
status: active
[0]-[current]-[~]
# ifconfig trunk0 trunkport em7
ifconfig: SIOCSTRUNKPORT: Invalid argument
[1]-[current]-[~]
# ifconfig trunk0 trunkport tun1
ifconfig: SIOCSTRUNKPORT: Protocol not supported
[1]-[current]-[~]
# ifconfig em1 up
[0]-[current]-[~]
# ifconfig trunk0 trunkport em1
[0]-[current]-[~]
#



--
-
() ascii ribbon campaign - against html e-mail
/\

Re: httpd: patch for portability asprintf use

[Ted Unangst]

Theo de Raadt wrote:
> > If OpenBSD's behavior of asprintf is non-standard and everyone else is
> > doing it differently, we would probably have to apply the patch. But this
> > would also affect many other places in the tree were we rely on our
> > asprintf semantics.
> 
> Actually, we have fixed all usage cases in our tree to be portable.
> 
> I have wondered in the past whether we should set the pointer to (void
> *)-1 instead of NULL, because this NULL return is a trap.

interesting idea..

Re: httpd: patch for portability asprintf use

[Theo de Raadt]

> Theo de Raadt wrote:
> > > If OpenBSD's behavior of asprintf is non-standard and everyone else is
> > > doing it differently, we would probably have to apply the patch. But this
> > > would also affect many other places in the tree were we rely on our
> > > asprintf semantics.
> > 
> > Actually, we have fixed all usage cases in our tree to be portable.
> > 
> > I have wondered in the past whether we should set the pointer to (void
> > *)-1 instead of NULL, because this NULL return is a trap.
> 
> interesting idea..

But then we should also gaurantee the last page of memory cannot
be mapped by userland, same treatment as the NULL page.

(Not all architectures behave like i386)

Re: tcpdump: print router advertisement preference

[Jeremie Courreges-Anglas]

j...@wxcvbn.org (Jeremie Courreges-Anglas) writes:

> As defined by https://tools.ietf.org/html/rfc4191, prompted by
> http://marc.info/?l=openbsd-misc&m=146239072929264&w=2
>
> "pref" is inspired by upstream tcpdump format.
>
> Thoughts / ok?

Committed, thanks for the reviews.

What about the same thing for route info options?

Index: print-icmp6.c
===
RCS file: /cvs/src/usr.sbin/tcpdump/print-icmp6.c,v
retrieving revision 1.19
diff -u -p -p -u -r1.19 print-icmp6.c
--- print-icmp6.c   6 May 2016 17:16:24 -   1.19
+++ print-icmp6.c   6 May 2016 18:54:31 -
@@ -526,7 +526,10 @@ icmp6_opt_print(const u_char *bp, int re
const struct icmp6_opts_redirect *opr;
const struct nd_opt_mtu *opm;
const struct nd_opt_rdnss *oprd;
+   const struct nd_opt_route_info *opri;
const u_char *ep;
+   const struct in6_addr *in6p;
+   struct in6_addr in6;
int i, opts_len;
 #if 0
const struct ip6_hdr *ip;
@@ -642,7 +645,43 @@ icmp6_opt_print(const u_char *bp, int re
resid - (op->nd_opt_len << 3));
break;
case ND_OPT_ROUTE_INFO:
-   printf("(route-info: opt_len=%d)", op->nd_opt_len);
+   opri = (struct nd_opt_route_info *)op;
+   TCHECK(opri->nd_opt_rti_lifetime);
+   printf("(route-info: ");
+   memset(&in6, 0, sizeof(in6));
+   in6p = (const struct in6_addr *)(opri + 1);
+   switch (op->nd_opt_len) {
+   case 1:
+   break;
+   case 2:
+   TCHECK2(*in6p, 8);
+   memcpy(&in6, opri + 1, 8);
+   break;
+   case 3:
+   TCHECK(*in6p);
+   memcpy(&in6, opri + 1, sizeof(in6));
+   break;
+   default:
+   goto trunc;
+   }
+   printf("%s/%u, ", ip6addr_string(&in6),
+   opri->nd_opt_rti_prefixlen);
+   switch (opri->nd_opt_rti_flags & ND_RA_FLAG_RTPREF_MASK) {
+   case ND_RA_FLAG_RTPREF_HIGH:
+   printf("pref=high, ");
+   break;
+   case ND_RA_FLAG_RTPREF_MEDIUM:
+   printf("pref=medium, ");
+   break;
+   case ND_RA_FLAG_RTPREF_LOW:
+   printf("pref=low, ");
+   break;
+   case ND_RA_FLAG_RTPREF_RSV:
+   printf("pref=rsv, ");
+   break;
+   }
+   printf("lifetime=%us)",
+   (u_int32_t)ntohl(opri->nd_opt_rti_lifetime));
icmp6_opt_print((const u_char *)op + (op->nd_opt_len << 3),
resid - (op->nd_opt_len << 3));
break;

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

Re: xclock patch

[Edgar Pettijohn]

Sent from my iPhone

> On May 6, 2016, at 12:13 PM, Matthieu Herrb  wrote:
> 
>> On Tue, May 03, 2016 at 11:40:31AM -0500, Edgar Pettijohn wrote:
>> I'll look at it this evening.
> 
> Ping...
> 

Sorry. I tried to get it to reproduce and couldn't. Looked through the code and 
couldn't find anything. 

>> Sent from my iPhone
>> 
 On May 3, 2016, at 9:40 AM, Sebastien Marie  wrote:
 
 On Mon, May 02, 2016 at 05:39:36PM -0500, Edgar Pettijohn wrote:
 I saw this in /var/log/messages
 
 May  2 17:19:07 thinkpad /bsd: xclock(81091): syscall 5 "rpath"
 
 Does it mean xclock needs rpath?  If so here you go.
>>> 
>>> generally it is what it means: at some point, the program required
>>> "rpath" promise but didn't pledge it (so it was killed).
>>> 
>>> But it could be good for us to know *why* it needs it :)
>>> 
>>> Could you provide a way to reproduce it ? It could be command-line used,
>>> actions that have been done, or any others clues... a backtrace could
>>> also be very useful.
>>> 
>>> Thanks.
>>> -- 
>>> Sebastien Marie
> 
> -- 
> Matthieu Herrb

Re: dhclient: use /dev/bpf

[Stuart Henderson]

On 2016/05/06 19:35, Martin Natano wrote:
> On Thu, May 05, 2016 at 09:30:43PM +0100, Stuart Henderson wrote:
> > On 2016/05/02 09:34, Martin Natano wrote:
> > > Diff below simplifies the device open path and removes an explanation
> > > about bpf device nodes from the manpage.
> > 
> > There's a problem with this. If someone is doing an "untar sets"
> > upgrade (which is not _supported_ but is pretty much necessary in
> > some situations) and relies on dhclient for network to get back
> > in to the system, they lose.
> > 
> > Since they won't have a new MAKEDEV at the right time, I think
> > our options are either "change dhclient back to using /dev/bpf0"
> > or "tell them to run mknod manually before updating".
> 
> Adding this information to current.html seems like the way to go to me.
> I see you and tb@ have already done so (with instructions to run MAKEDEV
> after unpacking the sets), thanks!

What we added to current.html is good enough for things like
pflogd etc., but it doesn't go far enough for the dhclient case.
In many cases (especially when people move from 5.9 to 6.0)
you won't be able to run MAKEDEV until *after* you've rebooted
due to pledge/syscall changes.

Personally, my preferred course of action would be to switch to
looking for /dev/bpf0 for now (bpf0 only: *not* revert to the old
loop, that's not necessary), and then switch to /dev/bpf
immediately after 6.0 is released. The recent few version
upgrades have been relatively painful, with good reason, but
this is something where we can easily reduce the pain with
minimal downside.

Re: dhclient: use /dev/bpf

[Theo de Raadt]

> > There's a problem with this. If someone is doing an "untar sets"
> > upgrade (which is not _supported_ but is pretty much necessary in
> > some situations) and relies on dhclient for network to get back
> > in to the system, they lose.
> > 
> > Since they won't have a new MAKEDEV at the right time, I think
> > our options are either "change dhclient back to using /dev/bpf0"
> > or "tell them to run mknod manually before updating".
> 
> Adding this information to current.html seems like the way to go to me.
> I see you and tb@ have already done so (with instructions to run MAKEDEV
> after unpacking the sets), thanks!

If there is a bug in code, that's a problem.

A missing node is no real fuss here.  Day by day, fewer and fewer
people are going to be affected IF THEY FOLLOW THE RECOMMENDED
PROCEDURES OF USING UPGRADE...  Sometimes "make build && reboot"
isn't complete; we don't allow the possibility of that occurance
from slowing down process in a significant way.

Re: dhclient: use /dev/bpf

[Martin Natano]

On Thu, May 05, 2016 at 09:30:43PM +0100, Stuart Henderson wrote:
> On 2016/05/02 09:34, Martin Natano wrote:
> > Diff below simplifies the device open path and removes an explanation
> > about bpf device nodes from the manpage.
> 
> There's a problem with this. If someone is doing an "untar sets"
> upgrade (which is not _supported_ but is pretty much necessary in
> some situations) and relies on dhclient for network to get back
> in to the system, they lose.
> 
> Since they won't have a new MAKEDEV at the right time, I think
> our options are either "change dhclient back to using /dev/bpf0"
> or "tell them to run mknod manually before updating".

Adding this information to current.html seems like the way to go to me.
I see you and tb@ have already done so (with instructions to run MAKEDEV
after unpacking the sets), thanks!

natano

Re: xclock patch

[Matthieu Herrb]

On Tue, May 03, 2016 at 11:40:31AM -0500, Edgar Pettijohn wrote:
> I'll look at it this evening.
>

Ping...

> Sent from my iPhone
> 
> > On May 3, 2016, at 9:40 AM, Sebastien Marie  wrote:
> > 
> >> On Mon, May 02, 2016 at 05:39:36PM -0500, Edgar Pettijohn wrote:
> >> I saw this in /var/log/messages
> >> 
> >> May  2 17:19:07 thinkpad /bsd: xclock(81091): syscall 5 "rpath"
> >> 
> >> Does it mean xclock needs rpath?  If so here you go.
> > 
> > generally it is what it means: at some point, the program required
> > "rpath" promise but didn't pledge it (so it was killed).
> > 
> > But it could be good for us to know *why* it needs it :)
> > 
> > Could you provide a way to reproduce it ? It could be command-line used,
> > actions that have been done, or any others clues... a backtrace could
> > also be very useful.
> > 
> > Thanks.
> > -- 
> > Sebastien Marie
> > 

-- 
Matthieu Herrb


signature.asc
Description: PGP signature

Re: ip6_input: Double check for mapped v4 addresses

[Jeremie Courreges-Anglas]

Stuart Henderson  writes:

> On 2016/05/06 15:05, Martin Pieuchot wrote:
>> This look like a bad merge from 2000s.  This check is present twice in
>> ip6_input(), so let's remove the late one, ok?
>> 
>> Index: netinet6/ip6_input.c
>> ===
>> RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
>> retrieving revision 1.158
>> diff -u -p -r1.158 ip6_input.c
>> --- netinet6/ip6_input.c 19 Apr 2016 08:23:13 -  1.158
>> +++ netinet6/ip6_input.c 6 May 2016 13:01:34 -
>> @@ -600,21 +600,6 @@ ip6_input(struct mbuf *m)
>>  ip6 = mtod(m, struct ip6_hdr *);
>
> OK, but I believe you can also remove this mtod.

Hmm, can't pf_test and ip6_hopopts_input actually modify "m" between the
two checks, making the second check possibly relevant?  pf_test would
modify m according to the config controlled by the admin, but after
a quick look I'm not sure about ip6_hopopts_input...  paranoid, maybe?

>>  /*
>> - * Malicious party may be able to use IPv4 mapped addr to confuse
>> - * tcp/udp stack and bypass security checks (act as if it was from
>> - * 127.0.0.1 by using IPv6 src :::127.0.0.1).  Be cautious.
>> - *
>> - * For SIIT end node behavior, you may want to disable the check.
>> - * However, you will  become vulnerable to attacks using IPv4 mapped
>> - * source.
>> - */
>> -if (IN6_IS_ADDR_V4MAPPED(&ip6->ip6_src) ||
>> -IN6_IS_ADDR_V4MAPPED(&ip6->ip6_dst)) {
>> -ip6stat.ip6s_badscope++;
>> -goto bad;
>> -}
>> -
>> -/*
>>   * Tell launch routine the next header
>>   */
>>  ip6stat.ip6s_delivered++;
>> 
>

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

Re: httpd: patch for portability asprintf use

[Hiltjo Posthuma]

On Fri, May 6, 2016 at 6:47 PM, Todd C. Miller
 wrote:
> On Fri, 06 May 2016 17:56:16 +0200, Hiltjo Posthuma wrote:
>
>> Any OK's for this?
>>
>> Please also see my mail with subject "httpd: fix/style: unbalanced
>> va_start and va_end macros" (don't want to spam the mailinglist :)).
>
> I didn't change those before because body and hstsheader are
> initialized to NULL.  All known asprintf() implementations either
> leave the ret pointer unchanged on error or set it to NULL.
>
> So this would only be a problem on systems where asprintf() modifies
> ret on error and sets it to a non-NULL value.  Is that really an
> issue?
>

It is not an issue, but I thought it was not intended because the
asprintf(3) documentation said:

"The value of ret in this situation is
implementation-dependent (on OpenBSD, ret will be set to the null
pointer, >but this behavior should not be relied upon<)."

Kind regards,
Hiltjo

Re: httpd: patch for portability asprintf use

[Reyk Floeter]

> On 06.05.2016, at 18:36, Theo de Raadt  wrote:
> 
>> If OpenBSD's behavior of asprintf is non-standard and everyone else is
>> doing it differently, we would probably have to apply the patch. But this
>> would also affect many other places in the tree were we rely on our
>> asprintf semantics.
> 
> Actually, we have fixed all usage cases in our tree to be portable.
> 
> I have wondered in the past whether we should set the pointer to (void
> *)-1 instead of NULL, because this NULL return is a trap.
> 


I think enforcing it this way would make much sense.

I'm a candidate for such traps as I only develop C code on/for OpenBSD.

OK, this makes Hiltjo's diff a valid addition.

Reyk

Re: httpd: patch for portability asprintf use

[Todd C. Miller]

On Fri, 06 May 2016 17:56:16 +0200, Hiltjo Posthuma wrote:

> Any OK's for this?
> 
> Please also see my mail with subject "httpd: fix/style: unbalanced
> va_start and va_end macros" (don't want to spam the mailinglist :)).

I didn't change those before because body and hstsheader are
initialized to NULL.  All known asprintf() implementations either
leave the ret pointer unchanged on error or set it to NULL.

So this would only be a problem on systems where asprintf() modifies
ret on error and sets it to a non-NULL value.  Is that really an
issue?

 - todd

Re: httpd: patch for portability asprintf use

[Theo de Raadt]

> If OpenBSD's behavior of asprintf is non-standard and everyone else is
> doing it differently, we would probably have to apply the patch. But this
> would also affect many other places in the tree were we rely on our
> asprintf semantics.

Actually, we have fixed all usage cases in our tree to be portable.

I have wondered in the past whether we should set the pointer to (void
*)-1 instead of NULL, because this NULL return is a trap.

Re: httpd: patch for portability asprintf use

[Reyk Floeter]

> On 06.05.2016, at 17:56, Hiltjo Posthuma  wrote:
> 
> On Wed, Apr 27, 2016 at 2:41 PM, Hiltjo Posthuma  > wrote:
>> Hi,
>> 
>> The following patch for httpd makes sure the value of the asprintf buffer is
>> zeroed on error and not relied upon, so at the 'done' label free(body) and
>> free(hstsheader) is safe.
>> 
>> from asprintf(3):
>> 
>>"The asprintf() and vasprintf() functions return the number of 
>> characters
>>that were output to the newly allocated string (excluding the '\0').
>>A pointer to the newly allocated string is returned in ret; it
>>should be passed to free(3) to release the allocated storage when it 
>> is
>>no longer needed.  If sufficient space cannot be allocated, these
>>functions will return -1. >>The value of ret in this situation is
>>implementation-dependent (on OpenBSD, ret will be set to the null
>>pointer, but this behavior should not be relied upon)."<<
>> 
>> 
>> Index: server_http.c
>> ===
>> RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
>> retrieving revision 1.106
>> diff -u -p -r1.106 server_http.c
>> --- server_http.c   8 Mar 2016 09:33:15 -   1.106
>> +++ server_http.c   27 Apr 2016 12:01:00 -
>> @@ -826,8 +826,10 @@ server_abort_http(struct client *clt, un
>>"\n%s\n"
>>"\n"
>>"\n",
>> -   code, httperr, style, code, httperr, HTTPD_SERVERNAME)) == -1)
>> +   code, httperr, style, code, httperr, HTTPD_SERVERNAME)) == -1) {
>> +   body = NULL;
>>goto done;
>> +   }
>> 
>>if (srv_conf->flags & SRVFLAG_SERVER_HSTS) {
>>if (asprintf(&hstsheader, "Strict-Transport-Security: "
>> @@ -835,8 +837,10 @@ server_abort_http(struct client *clt, un
>>srv_conf->hsts_flags & HSTSFLAG_SUBDOMAINS ?
>>"; includeSubDomains" : "",
>>srv_conf->hsts_flags & HSTSFLAG_PRELOAD ?
>> -   "; preload" : "") == -1)
>> +   "; preload" : "") == -1) {
>> +   hstsheader = NULL;
>>goto done;
>> +   }
>>}
>> 
>>/* Add basic HTTP headers */
>> 
>> --
> 
> Any OK's for this?
> 
> Please also see my mail with subject "httpd: fix/style: unbalanced
> va_start and va_end macros" (don't want to spam the mailinglist :)).

I don't know.

It doesn't affect OpenBSD and I don't like portability code in tree.
Isn't it better to create a portable version of httpd instead?

If OpenBSD's behavior of asprintf is non-standard and everyone else is
doing it differently, we would probably have to apply the patch. But this
would also affect many other places in the tree were we rely on our
asprintf semantics.

Reyk

Re: httpd: patch for portability asprintf use

[Hiltjo Posthuma]

On Wed, Apr 27, 2016 at 2:41 PM, Hiltjo Posthuma  wrote:
> Hi,
>
> The following patch for httpd makes sure the value of the asprintf buffer is
> zeroed on error and not relied upon, so at the 'done' label free(body) and
> free(hstsheader) is safe.
>
> from asprintf(3):
>
> "The asprintf() and vasprintf() functions return the number of 
> characters
> that were output to the newly allocated string (excluding the '\0').
> A pointer to the newly allocated string is returned in ret; it
> should be passed to free(3) to release the allocated storage when it 
> is
> no longer needed.  If sufficient space cannot be allocated, these
> functions will return -1. >>The value of ret in this situation is
> implementation-dependent (on OpenBSD, ret will be set to the null
> pointer, but this behavior should not be relied upon)."<<
>
>
> Index: server_http.c
> ===
> RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
> retrieving revision 1.106
> diff -u -p -r1.106 server_http.c
> --- server_http.c   8 Mar 2016 09:33:15 -   1.106
> +++ server_http.c   27 Apr 2016 12:01:00 -
> @@ -826,8 +826,10 @@ server_abort_http(struct client *clt, un
> "\n%s\n"
> "\n"
> "\n",
> -   code, httperr, style, code, httperr, HTTPD_SERVERNAME)) == -1)
> +   code, httperr, style, code, httperr, HTTPD_SERVERNAME)) == -1) {
> +   body = NULL;
> goto done;
> +   }
>
> if (srv_conf->flags & SRVFLAG_SERVER_HSTS) {
> if (asprintf(&hstsheader, "Strict-Transport-Security: "
> @@ -835,8 +837,10 @@ server_abort_http(struct client *clt, un
> srv_conf->hsts_flags & HSTSFLAG_SUBDOMAINS ?
> "; includeSubDomains" : "",
> srv_conf->hsts_flags & HSTSFLAG_PRELOAD ?
> -   "; preload" : "") == -1)
> +   "; preload" : "") == -1) {
> +   hstsheader = NULL;
> goto done;
> +   }
> }
>
> /* Add basic HTTP headers */
>
> --

Any OK's for this?

Please also see my mail with subject "httpd: fix/style: unbalanced
va_start and va_end macros" (don't want to spam the mailinglist :)).

> Kind regards,
> Hiltjo

Re: enable shadow only pwd

[Jyri Hovila [Turvamies.fi]]

>> Feel free to investigate the ports I mentioned then.

That's exactly what I was planning to do already. Just didn't want to make any 
promises - I've got my hands so full of other OpenBSD related projects that the 
daily 24 hour limit is getting really troublesome. =P

>> Diffs to
>> ports@ please

Ok.

>> - you can test that they work using tedu's diff
>> that will avoid filling in pw_passwd in the struct,

Ok.

>> it's usually
>> just a case of s/(getpwuid|getpwnam)/\1_shadow/.

Yup.

> It's not black magic.  Anyone who knows what source code is, can help.

Indeed.

-j.

Re: changelist(5) [and mtree(8)] entry for rebound(8) config file

[Raf Czlonka]

On Fri, May 06, 2016 at 06:37:44AM BST, Raf Czlonka wrote:
> Hi all,
> 
> Thought rebound(8) config file could be added to changelist(5).
> 
> Regards,
> 
> Raf
> 
> Index: etc/changelist
> ===
> RCS file: /cvs/src/etc/changelist,v
> retrieving revision 1.112
> diff -u -p -r1.112 changelist
> --- etc/changelist20 Apr 2016 12:41:30 -  1.112
> +++ etc/changelist6 May 2016 05:31:00 -
> @@ -102,6 +102,7 @@
>  /etc/rc.local
>  /etc/rc.securelevel
>  /etc/rc.shutdown
> +/etc/rebound.conf
>  /etc/relayd.conf
>  /etc/remote
>  /etc/resolv.conf

Hi all,

If you agree to the above, then the below should probably also be added.

Regards,

Raf

Index: etc/mtree/special
===
RCS file: /cvs/src/etc/mtree/special,v
retrieving revision 1.119
diff -u -p -r1.119 special
--- etc/mtree/special   25 Apr 2016 20:38:11 -  1.119
+++ etc/mtree/special   6 May 2016 14:05:33 -
@@ -81,6 +81,7 @@ rc.conf.local type=file mode=0644 uname=
 rc.local   type=file mode=0644 uname=root gname=wheel optional
 rc.securelevel type=file mode=0644 uname=root gname=wheel optional
 rc.shutdowntype=file mode=0644 uname=root gname=wheel optional
+rebound.conf   type=file mode=0644 uname=root gname=wheel optional
 relayd.conftype=file mode=0600 uname=root gname=wheel optional
 remote type=file mode=0644 uname=root gname=wheel optional
 resolv.conftype=file mode=0644 uname=root gname=wheel optional

Re: enable shadow only pwd

[Theo de Raadt]

> On 2016/05/06 16:44, Jyri Hovila [Turvamies.fi] wrote:
> > 
> > > Anyways, it would be really nice if we can get the issues resolved
> > > before end of June, because 6.0 should have this.  It is a valuable
> > > security improvement.
> > 
> > Amen to that!
> > 
> > -j.
> 
> Feel free to investigate the ports I mentioned then. Diffs to
> ports@ please - you can test that they work using tedu's diff
> that will avoid filling in pw_passwd in the struct, it's usually
> just a case of s/(getpwuid|getpwnam)/\1_shadow/.

- 1 line change to libc
- rebuild libc
- test all the ports sthen listed
- test dependencies of those ports, also
- look for failure, help fix them

It's not black magic.  Anyone who knows what source code is, can help.
 

Re: enable shadow only pwd

[Stuart Henderson]

On 2016/05/06 16:44, Jyri Hovila [Turvamies.fi] wrote:
> 
> > Anyways, it would be really nice if we can get the issues resolved
> > before end of June, because 6.0 should have this.  It is a valuable
> > security improvement.
> 
> Amen to that!
> 
> -j.

Feel free to investigate the ports I mentioned then. Diffs to
ports@ please - you can test that they work using tedu's diff
that will avoid filling in pw_passwd in the struct, it's usually
just a case of s/(getpwuid|getpwnam)/\1_shadow/.

Re: enable shadow only pwd

[Jyri Hovila [Turvamies.fi]]

> Anyways, it would be really nice if we can get the issues resolved
> before end of June, because 6.0 should have this.  It is a valuable
> security improvement.

Amen to that!

-j.

Re: ip6_input: Double check for mapped v4 addresses

[Stuart Henderson]

On 2016/05/06 15:05, Martin Pieuchot wrote:
> This look like a bad merge from 2000s.  This check is present twice in
> ip6_input(), so let's remove the late one, ok?
> 
> Index: netinet6/ip6_input.c
> ===
> RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
> retrieving revision 1.158
> diff -u -p -r1.158 ip6_input.c
> --- netinet6/ip6_input.c  19 Apr 2016 08:23:13 -  1.158
> +++ netinet6/ip6_input.c  6 May 2016 13:01:34 -
> @@ -600,21 +600,6 @@ ip6_input(struct mbuf *m)
>   ip6 = mtod(m, struct ip6_hdr *);

OK, but I believe you can also remove this mtod.

>   /*
> -  * Malicious party may be able to use IPv4 mapped addr to confuse
> -  * tcp/udp stack and bypass security checks (act as if it was from
> -  * 127.0.0.1 by using IPv6 src :::127.0.0.1).  Be cautious.
> -  *
> -  * For SIIT end node behavior, you may want to disable the check.
> -  * However, you will  become vulnerable to attacks using IPv4 mapped
> -  * source.
> -  */
> - if (IN6_IS_ADDR_V4MAPPED(&ip6->ip6_src) ||
> - IN6_IS_ADDR_V4MAPPED(&ip6->ip6_dst)) {
> - ip6stat.ip6s_badscope++;
> - goto bad;
> - }
> -
> - /*
>* Tell launch routine the next header
>*/
>   ip6stat.ip6s_delivered++;
> 

Re: enable shadow only pwd

[Theo de Raadt]

> On 2016/05/04 17:48, Ted Unangst wrote:
> > i think it's time. otherwise we'll never find the bugs.
> 
> I don't think it's time since afaicr nobody other than me has fixed
> anything for this in ports yet. They're not hard to find, try screen
> lockers for starters. Because this is using a different API than
> everyone else with shadow passwords we don't get fixes for free -
> anything using pw_passwd from getpw{nam,uid}() needs modifying.
> 
> If you'd sent this 10 days ago we could have had enough of them
> fixed at p2k16. As I won't have time to do that now, here are
> unfiltered search results from ports source (unpacked Aug 2015
> so there may be some more by now). I'll have a look and see if
> I've got any notes on which ones I already looked at.

Bummer, bad timing.

Anyways, it would be really nice if we can get the issues resolved
before end of June, because 6.0 should have this.  It is a valuable
security improvement.

ip6_input: Double check for mapped v4 addresses

[Martin Pieuchot]

This look like a bad merge from 2000s.  This check is present twice in
ip6_input(), so let's remove the late one, ok?

Index: netinet6/ip6_input.c
===
RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
retrieving revision 1.158
diff -u -p -r1.158 ip6_input.c
--- netinet6/ip6_input.c19 Apr 2016 08:23:13 -  1.158
+++ netinet6/ip6_input.c6 May 2016 13:01:34 -
@@ -600,21 +600,6 @@ ip6_input(struct mbuf *m)
ip6 = mtod(m, struct ip6_hdr *);
 
/*
-* Malicious party may be able to use IPv4 mapped addr to confuse
-* tcp/udp stack and bypass security checks (act as if it was from
-* 127.0.0.1 by using IPv6 src :::127.0.0.1).  Be cautious.
-*
-* For SIIT end node behavior, you may want to disable the check.
-* However, you will  become vulnerable to attacks using IPv4 mapped
-* source.
-*/
-   if (IN6_IS_ADDR_V4MAPPED(&ip6->ip6_src) ||
-   IN6_IS_ADDR_V4MAPPED(&ip6->ip6_dst)) {
-   ip6stat.ip6s_badscope++;
-   goto bad;
-   }
-
-   /*
 * Tell launch routine the next header
 */
ip6stat.ip6s_delivered++;

ip_forward mpath fix

[Martin Pieuchot]

Now that ip_forward() is reusing the route fetched by in_ouraddr() we
want to make sure we're fetching a possible RTF_MPATH route and not
simply picking the first one every time.

This restore the behavior of r1.274 when using mpath entries for
forwarding, ok?

Index: netinet/ip_input.c
===
RCS file: /cvs/src/sys/netinet/ip_input.c,v
retrieving revision 1.275
diff -u -p -r1.275 ip_input.c
--- netinet/ip_input.c  3 May 2016 12:19:13 -   1.275
+++ netinet/ip_input.c  6 May 2016 12:22:57 -
@@ -606,7 +606,8 @@ in_ouraddr(struct mbuf *m, struct ifnet 
sin.sin_len = sizeof(sin);
sin.sin_family = AF_INET;
sin.sin_addr = ip->ip_dst;
-   rt = rtalloc(sintosa(&sin), 0, m->m_pkthdr.ph_rtableid);
+   rt = rtalloc_mpath(sintosa(&sin), &ip->ip_src.s_addr,
+   m->m_pkthdr.ph_rtableid);
if (rtisvalid(rt)) {
if (ISSET(rt->rt_flags, RTF_LOCAL))
match = 1;

Re: remove MPSAFE from makesyscalls.sh

[Martin Pieuchot]

On 02/05/16(Mon) 12:07, Michal Mazurek wrote:
> On 10:22:58,  7.04.16, Martin Natano wrote:
> > On Wed, Apr 06, 2016 at 03:42:20PM +0200, Michal Mazurek wrote:
> > > MPSAFE is never used, and doesn't look like it's even supported (no
> > > matching SY_MPSAFE anywhere).
> > 
> > SY_MPSAFE seems to be unused since it's introduction in 2007 and it
> > doesn't have any effect; ok natano@, if no one comes up with a reason to
> > keep this.
> 
> Nearly a month later, nobody did come up with a reason to keep it.

I think it's fine to get rid of it.

Re: document NOLOCK in syscalls.master

[Martin Pieuchot]

On 02/05/16(Mon) 12:34, Michal Mazurek wrote:
> On 10:30:02,  7.04.16, Martin Natano wrote:
> > > * mention INDIR and NOLOCK in 'Fields'.
> > 
> > How about something like "one of the types listed below, or one of the
> > compatibility options defined in syscalls.conf" instead?
> 
> That's a better idea, included below but s/listed/described/.
> 
> > > * sort the list of types in 'Fields'.
> > > * sort the list of types in 'types'.
> > 
> > I think the order was more logical before (most used type first).
> 
> I wanted to verify this statement:
> 
> perl -ne 's/^(\d+\s+)// && s/\s+[{a-z4].*$// && print' kern/syscalls.master | 
> perl -pse 's/\s+/\n/g' | sort | uniq -c | sort -nr
>  211 STD
>   59 OBSOL
>   31 UNIMPL
>   19 NOLOCK
>2 INDIR
> 
> It seems NODEF and NOARGS are never used, is this right?
> 
> > > * place a dot ('.') at the end of each sentence.
> > 
> > Bike-shedding: Those are not full sentences, but only fragments. There
> > doesn't have to be a dot at the end.
> 
> I think it's better to have consistency, dot or not. Attached is a
> version without the unfortunate dots.
> 
> On 14:57:02,  6.04.16, Mike Belopuhov wrote:
> > "NOLOCK the syscall doesn't grab any locks whatsoever"
> > is a poor description IMO.  It should say it doesn't grab the
> > KERNEL_LOCK or biglock or something like that.  "Any
> > locks whatsoever" is incorrect as there might be mutexes,
> > rwlocks, etc.
> 
> How about this:
> NOLOCK  don't acquire the kernel lock when calling this syscall.

ok mpi@

> 
> 
> Index: sys/kern/syscalls.master
> ===
> RCS file: /cvs/src/sys/kern/syscalls.master,v
> retrieving revision 1.169
> diff -u -p -r1.169 syscalls.master
> --- sys/kern/syscalls.master  30 Mar 2016 07:49:11 -  1.169
> +++ sys/kern/syscalls.master  2 May 2016 10:31:12 -
> @@ -8,17 +8,18 @@
>  ;
>  ; Fields: number type [type-dependent ...]
>  ;number  system call number, must be in order
> -;typeone of STD, OBSOL, UNIMPL, NODEF, NOARGS, or one of
> -;the compatibility options defined in syscalls.conf.
> +;typeone of the types described below, or one of the compatibility
> +;options defined in syscalls.conf
>  ;
>  ; types:
> -;STD always included
> +;INDIR   included, but don't define the syscall args structure,
> +;and allow it to be "really" varargs
> +;NOARGS  included, but don't define the syscall args structure
> +;NODEF   included, but don't define the syscall number
> +;NOLOCK  don't acquire the kernel lock when calling this syscall
>  ;OBSOL   obsolete, not included in system
> +;STD always included
>  ;UNIMPL  unimplemented, not included in system
> -;NODEF   included, but don't define the syscall number
> -;NOARGS  included, but don't define the syscall args structure
> -;INDIR   included, but don't define the syscall args structure,
> -;and allow it to be "really" varargs.
>  ;
>  ; The compat options are defined in the syscalls.conf file, and the
>  ; compat option name is prefixed to the syscall name.  Other than
> 
> -- 
> Michal Mazurek
> 

tcpdump: print router advertisement preference

[Jérémie Courrèges-Anglas]

As defined by https://tools.ietf.org/html/rfc4191, prompted by
http://marc.info/?l=openbsd-misc&m=146239072929264&w=2

"pref" is inspired by upstream tcpdump format.

Thoughts / ok?

Index: print-icmp6.c
===
RCS file: /cvs/src/usr.sbin/tcpdump/print-icmp6.c,v
retrieving revision 1.18
diff -u -p -p -u -r1.18 print-icmp6.c
--- print-icmp6.c   15 Mar 2016 05:03:11 -  1.18
+++ print-icmp6.c   6 May 2016 11:55:20 -
@@ -305,8 +305,24 @@ icmp6_print(const u_char *bp, u_int leng
printf("M");
if (p->nd_ra_flags_reserved & ND_RA_FLAG_OTHER)
printf("O");
-   if (p->nd_ra_flags_reserved != 0)
-   printf(" ");
+   if (p->nd_ra_flags_reserved &
+   (ND_RA_FLAG_MANAGED|ND_RA_FLAG_OTHER))
+   printf(", ");
+   switch (p->nd_ra_flags_reserved
+   & ND_RA_FLAG_RTPREF_MASK) {
+   case ND_RA_FLAG_RTPREF_HIGH:
+   printf("pref=high, ");
+   break;
+   case ND_RA_FLAG_RTPREF_MEDIUM:
+   printf("pref=medium, ");
+   break;
+   case ND_RA_FLAG_RTPREF_LOW:
+   printf("pref=low, ");
+   break;
+   case ND_RA_FLAG_RTPREF_RSV:
+   printf("pref=rsv, ");
+   break;
+   }
printf("router_ltime=%d, ", 
ntohs(p->nd_ra_router_lifetime));
printf("reachable_time=%u, ",
(u_int32_t)ntohl(p->nd_ra_reachable));


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

Re: ld patch that greatly speeds up linking large programs with debug symbols

[Martin Pieuchot]

On 27/04/16(Wed) 21:12, Aaron Miller wrote:
> Hi All,
> 
> I was experiencing ~8 minute linking times for a large C++ application
> I have been working on when running -current on amd64. It turns out
> that the decade-old version of ld in the OpenBSD source tree has a bug
> that causes quadratic complexity for some linking operations when
> debug symbols are present. I found a patch from 2006 in a mailing list
> archive that fixes this; I adapted it to work with OpenBSD by editing
> files that are normally regenerated (I couldn't figure out how to do
> automatically regenerate them).

Do you see a difference when linking a kernel built with DWARF sections,
i.e. by setting DEBUG to -g?

> Here is the original patch:
> https://sourceware.org/ml/binutils/2006-08/msg00334.html
> 
> I have rebuilt the kernel and system with this ld and everything runs
> identically to the system linked with the unpatched ld.

Could you make sure ports bulk builds run without regression.  You can
ping aja@ and landry@ to have it tested on various architectures.

> Please test this and let me know if this could get into the tree, and
> if not, what changes I need to make. Thanks!

I'd be nice if your changes are not lost when re-generating files.  I
don't know how to check that myself but maybe somebody could comment
on that.

> Index: bfd/bfd-in2.h
> ===
> RCS file: /cvs/src/gnu/usr.bin/binutils-2.17/bfd/bfd-in2.h,v
> retrieving revision 1.4
> diff -u -p -r1.4 bfd-in2.h
> --- bfd/bfd-in2.h25 May 2015 14:56:26 -1.4
> +++ bfd/bfd-in2.h17 Apr 2016 05:14:15 -
> @@ -5068,7 +5068,7 @@ typedef struct bfd_target
> 
>/* Check if SEC has been already linked during a reloceatable or
>   final link.  */
> -  void (*_section_already_linked) (bfd *, struct bfd_section *);
> +  void (*_section_already_linked) (bfd *, struct bfd_section *,
> struct bfd_link_info *);
> 
>/* Routines to handle dynamic symbols and relocs.  */
>  #define BFD_JUMP_TABLE_DYNAMIC(NAME) \
> @@ -5128,10 +5128,10 @@ bfd_boolean bfd_link_split_section (bfd
>  #define bfd_link_split_section(abfd, sec) \
> BFD_SEND (abfd, _bfd_link_split_section, (abfd, sec))
> 
> -void bfd_section_already_linked (bfd *abfd, asection *sec);
> +void bfd_section_already_linked (bfd *abfd, asection *sec, struct
> bfd_link_info *);
> 
> -#define bfd_section_already_linked(abfd, sec) \
> -   BFD_SEND (abfd, _section_already_linked, (abfd, sec))
> +#define bfd_section_already_linked(abfd, sec, info) \
> +   BFD_SEND (abfd, _section_already_linked, (abfd, sec, info))
> 
>  /* Extracted from simple.c.  */
>  bfd_byte *bfd_simple_get_relocated_section_contents
> Index: bfd/elf-bfd.h
> ===
> RCS file: /cvs/src/gnu/usr.bin/binutils-2.17/bfd/elf-bfd.h,v
> retrieving revision 1.4
> diff -u -p -r1.4 elf-bfd.h
> --- bfd/elf-bfd.h25 Aug 2015 02:24:49 -1.4
> +++ bfd/elf-bfd.h17 Apr 2016 05:14:16 -
> @@ -1371,6 +1371,9 @@ struct elf_obj_tdata
> 
>/* Used to determine if we are creating an executable.  */
>bfd_boolean executable;
> +
> +  /* Symbol buffer.  */
> +  Elf_Internal_Sym *symbuf;
>  };
> 
>  #define elf_tdata(bfd)((bfd) -> tdata.elf_obj_data)
> @@ -1503,11 +1506,11 @@ extern bfd_boolean _bfd_elf_match_sectio
>  extern bfd_boolean bfd_elf_is_group_section
>(bfd *, const struct bfd_section *);
>  extern void _bfd_elf_section_already_linked
> -  (bfd *, struct bfd_section *);
> +  (bfd *, struct bfd_section *, struct bfd_link_info *);
>  extern void bfd_elf_set_group_contents
>(bfd *, asection *, void *);
>  extern asection *_bfd_elf_check_kept_section
> -  (asection *);
> +  (asection *, struct bfd_link_info *);
>  extern void _bfd_elf_link_just_syms
>(asection *, struct bfd_link_info *);
>  extern bfd_boolean _bfd_elf_copy_private_header_data
> @@ -1703,7 +1706,7 @@ extern bfd_boolean _bfd_elf_symbol_refs_
>(struct elf_link_hash_entry *, struct bfd_link_info *, bfd_boolean);
> 
>  extern bfd_boolean bfd_elf_match_symbols_in_sections
> -  (asection *sec1, asection *sec2);
> +  (asection *, asection *, struct bfd_link_info *);
> 
>  extern bfd_boolean _bfd_elf_setup_sections
>(bfd *);
> Index: bfd/elf.c
> ===
> RCS file: /cvs/src/gnu/usr.bin/binutils-2.17/bfd/elf.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 elf.c
> --- bfd/elf.c13 Jan 2015 20:05:43 -1.7
> +++ bfd/elf.c17 Apr 2016 05:14:18 -
> @@ -3101,7 +3101,7 @@ assign_section_numbers (bfd *abfd, struc
>   s, s->owner);
>/* Point to the kept section if it has the same
>   size as the discarded one.  */
> -  kept = _bfd_elf_check_kept_section (s);
> +  kept = _bfd_elf_check_kept_section (s, link_info);
>if (kept == NULL)
>  {
>bfd