Re: tftpd(8): diff for ip path rewrite

[Sebastien Marie]

On Thu, Oct 19, 2017 at 08:58:12PM +0200, Jan Klemkow wrote:
> 
> Index: tftpd.c
> ===
> RCS file: /mount/openbsd/cvs/src/usr.sbin/tftpd/tftpd.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 tftpd.c
> --- tftpd.c   26 May 2017 17:38:46 -  1.39
> +++ tftpd.c   19 Oct 2017 18:27:24 -
> @@ -903,8 +903,17 @@ again:
>  
>   if (rwmap != NULL)
>   rewrite_map(client, filename);
> - else
> - tftp_open(client, filename);
> + else {
> + char nfilename[PATH_MAX];
> +
> + snprintf(nfilename, sizeof nfilename, "%s/%s",
> + getip(>ss), filename);

- filename has PATH_MAX length
- getip(>ss) could have NI_MAXHOST length

so nfilename could be larger than PATH_MAX (sizeof nfilename).

I assume the return of snprintf() need to be checked. if truncation
occured, a warning should be issued and nfilename discarded (just
calling tftp_open(client, filename)) ?

> +
> + if (access(nfilename, R_OK) == 0)
> + tftp_open(client, nfilename);
> + else
> + tftp_open(client, filename);
> + }
>  
>   return;
>  
> 

thanks
-- 
Sebastien Marie

Re: tftpd(8): diff for ip path rewrite

[Stuart Henderson]

On 2017/10/19 16:22, Theo de Raadt wrote:
> I am always worried by non-intuitive magic behaviour.
> 
> It may serve some obvious purposes, but for someone else it is going
> to break things.
> 
> I worry.

The IP/filename -> filename fallback method seems good enough, but
I agree with Theo.

I think it would be safer behind a flag rather than on by default.

Re: tftpd(8): diff for ip path rewrite

[Theo de Raadt]

I am always worried by non-intuitive magic behaviour.

It may serve some obvious purposes, but for someone else it is going
to break things.

I worry.

> bluhm@ suggested, that this should be the default behavior.  Thus, the
> ftpd(8) checks if a subdirectory with the client's ip address exists and
> contains the requested file.  It not, it uses the original path as
> default.  I implemented it in this diff:
> 
> Index: tftpd.8
> ===
> RCS file: /mount/openbsd/cvs/src/usr.sbin/tftpd/tftpd.8,v
> retrieving revision 1.5
> diff -u -p -r1.5 tftpd.8
> --- tftpd.8   18 Jul 2015 05:32:56 -  1.5
> +++ tftpd.8   19 Oct 2017 18:41:07 -
> @@ -78,6 +78,14 @@ and therefore this path will be ignored 
>  .Ox
>  network bootloaders access this path to harvest entropy during
>  kernel load.
> +Also,
> +.Nm
> +always tries to rewrite the requested filename with a prefix of
> +the client's IP address.
> +If the rewritten path exists
> +.Nm
> +will serve this file.
> +If not, it will serve the original filename.
>  .Pp
>  The options are as follows:
>  .Bl -tag -width Ds
> Index: tftpd.c
> ===
> RCS file: /mount/openbsd/cvs/src/usr.sbin/tftpd/tftpd.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 tftpd.c
> --- tftpd.c   26 May 2017 17:38:46 -  1.39
> +++ tftpd.c   19 Oct 2017 18:27:24 -
> @@ -903,8 +903,17 @@ again:
>  
>   if (rwmap != NULL)
>   rewrite_map(client, filename);
> - else
> - tftp_open(client, filename);
> + else {
> + char nfilename[PATH_MAX];
> +
> + snprintf(nfilename, sizeof nfilename, "%s/%s",
> + getip(>ss), filename);
> +
> + if (access(nfilename, R_OK) == 0)
> + tftp_open(client, nfilename);
> + else
> + tftp_open(client, filename);
> + }
>  
>   return;
>  
> 

Re: tftpd(8): diff for ip path rewrite

[Jan Klemkow]

On Thu, Oct 19, 2017 at 09:36:50AM +, Jeremie Courreges-Anglas wrote:
> On Wed, Oct 18 2017, Jan Klemkow  wrote:
> > On Wed, Oct 18, 2017 at 08:37:48PM +, Jason McIntyre wrote:
> >> On Wed, Oct 18, 2017 at 10:25:13PM +0200, Jan Klemkow wrote:
> >> > This diff adds an option for client IP address path prefixes to the
> >> > tftpd(8).  First, I used the -r rewrite socket for this, but...
> >> > 
> >> > If you use the rewrite socket feature, the tftpd(8) will exit with an
> >> > error when the rewrite socket is closed.  A reopen of the socket is not
> >> > possible, if its outside of the chroot directory.  And a privilege
> >> > separated tftpd(8) is a bit overkill for a stable per client path
> >> > rewrite feature.  This story led me to this change here.
> 
> I think it makes sense to support this feature without the need for an
> additional unix service.
> 
> >> > Any suggestions or objections are welcome. :-)
> 
> Do we want to provide a fallback directory so that you don't need to
> restart tftpd without -i to support unknown clients?

bluhm@ suggested, that this should be the default behavior.  Thus, the
ftpd(8) checks if a subdirectory with the client's ip address exists and
contains the requested file.  It not, it uses the original path as
default.  I implemented it in this diff:

Index: tftpd.8
===
RCS file: /mount/openbsd/cvs/src/usr.sbin/tftpd/tftpd.8,v
retrieving revision 1.5
diff -u -p -r1.5 tftpd.8
--- tftpd.8 18 Jul 2015 05:32:56 -  1.5
+++ tftpd.8 19 Oct 2017 18:41:07 -
@@ -78,6 +78,14 @@ and therefore this path will be ignored 
 .Ox
 network bootloaders access this path to harvest entropy during
 kernel load.
+Also,
+.Nm
+always tries to rewrite the requested filename with a prefix of
+the client's IP address.
+If the rewritten path exists
+.Nm
+will serve this file.
+If not, it will serve the original filename.
 .Pp
 The options are as follows:
 .Bl -tag -width Ds
Index: tftpd.c
===
RCS file: /mount/openbsd/cvs/src/usr.sbin/tftpd/tftpd.c,v
retrieving revision 1.39
diff -u -p -r1.39 tftpd.c
--- tftpd.c 26 May 2017 17:38:46 -  1.39
+++ tftpd.c 19 Oct 2017 18:27:24 -
@@ -903,8 +903,17 @@ again:
 
if (rwmap != NULL)
rewrite_map(client, filename);
-   else
-   tftp_open(client, filename);
+   else {
+   char nfilename[PATH_MAX];
+
+   snprintf(nfilename, sizeof nfilename, "%s/%s",
+   getip(>ss), filename);
+
+   if (access(nfilename, R_OK) == 0)
+   tftp_open(client, nfilename);
+   else
+   tftp_open(client, filename);
+   }
 
return;
 

Enable TCP selective acknowledgements (SACK) on all kernels

[Mike Belopuhov]

SACK has been enabled in GENERIC kernels for over a decade and it's
time to make it an official part of the TCP stack.  This grows bsd.rd
on amd64 by 8k but Theo said it's within reasonable.  OK?

diff --git sys/conf/GENERIC sys/conf/GENERIC
index 87dd069f514..cd68ae9e651 100644
--- sys/conf/GENERIC
+++ sys/conf/GENERIC
@@ -43,11 +43,10 @@ option  MSDOSFS # MS-DOS file system
 option FIFO# FIFOs; RECOMMENDED
 #optionTMPFS   # efficient memory file system
 option FUSE# FUSE
 
 option SOCKET_SPLICE   # Socket Splicing for TCP and UDP
-option TCP_SACK# Selective Acknowledgements for TCP
 option TCP_ECN # Explicit Congestion Notification for TCP
 option TCP_SIGNATURE   # TCP MD5 Signatures, for BGP routing sessions
 #optionTCP_FACK# Forward Acknowledgements for TCP
 
 option INET6   # IPv6
diff --git sys/netinet/tcp_input.c sys/netinet/tcp_input.c
index 52c206f0bf5..9951923bbdb 100644
--- sys/netinet/tcp_input.c
+++ sys/netinet/tcp_input.c
@@ -852,14 +852,12 @@ findpcb:
 */
tp->t_rcvtime = tcp_now;
if (TCPS_HAVEESTABLISHED(tp->t_state))
TCP_TIMER_ARM(tp, TCPT_KEEP, tcp_keepidle);
 
-#ifdef TCP_SACK
if (tp->sack_enable)
tcp_del_sackholes(tp, th); /* Delete stale SACK holes */
-#endif /* TCP_SACK */
 
/*
 * Process options.
 */
 #ifdef TCP_SIGNATURE
@@ -962,25 +960,23 @@ findpcb:
 */
if (tp->t_pmtud_mss_acked < acked)
tp->t_pmtud_mss_acked = acked;
 
tp->snd_una = th->th_ack;
-#if defined(TCP_SACK) || defined(TCP_ECN)
/*
 * We want snd_last to track snd_una so
 * as to avoid sequence wraparound problems
 * for very large transfers.
 */
 #ifdef TCP_ECN
if (SEQ_GT(tp->snd_una, tp->snd_last))
 #endif
tp->snd_last = tp->snd_una;
-#endif /* TCP_SACK */
-#if defined(TCP_SACK) && defined(TCP_FACK)
+#ifdef TCP_FACK
tp->snd_fack = tp->snd_una;
tp->retran_data = 0;
-#endif /* TCP_FACK */
+#endif
m_freem(m);
 
/*
 * If all outstanding data are acked, stop
 * retransmit timer, otherwise restart timer
@@ -1012,15 +1008,13 @@ findpcb:
/*
 * This is a pure, in-sequence data packet
 * with nothing on the reassembly queue and
 * we have enough buffer space to take it.
 */
-#ifdef TCP_SACK
/* Clean receiver SACK report if present */
if (tp->sack_enable && tp->rcv_numsacks)
tcp_clean_sackreport(tp);
-#endif /* TCP_SACK */
tcpstat_inc(tcps_preddat);
tp->rcv_nxt += tlen;
tcpstat_pkt(tcps_rcvpack, tcps_rcvbyte, tlen);
ND6_HINT(tp);
 
@@ -1137,19 +1131,17 @@ findpcb:
/* Reset initial window to 1 segment for retransmit */
if (tp->t_rxtshift > 0)
tp->snd_cwnd = tp->t_maxseg;
tcp_rcvseqinit(tp);
tp->t_flags |= TF_ACKNOW;
-#ifdef TCP_SACK
 /*
  * If we've sent a SACK_PERMITTED option, and the peer
  * also replied with one, then TF_SACK_PERMIT should have
  * been set in tcp_dooptions().  If it was not, disable SACKs.
  */
if (tp->sack_enable)
tp->sack_enable = tp->t_flags & TF_SACK_PERMIT;
-#endif
 #ifdef TCP_ECN
/*
 * if ECE is set but CWR is not set for SYN-ACK, or
 * both ECE and CWR are set for simultaneous open,
 * peer is ECN capable.
@@ -1569,11 +1561,11 @@ trimthenstep6:
 * to keep a constant cwnd packets in the
 * network.
 */
if (TCP_TIMER_ISARMED(tp, TCPT_REXMT) == 0)
tp->t_dupacks = 0;
-#if defined(TCP_SACK) && defined(TCP_FACK)
+#ifdef TCP_FACK
/*
 * In FACK, can enter fast rec. if the receiver
 * reports a reass. queue longer than 3 segs.
 */
else 

iked: support multiple subjectAltNames

[Patrick Wildt]

Hi,

so far, even if we look for our own cert, we only match the id against
the first subjectAltName.  This means we cannot use certificates where
we actually need a different one.  This diff changes the behaviour so
that we check all subjectAltNames of a given certificate.

ok?

Patrick

diff --git a/sbin/iked/ca.c b/sbin/iked/ca.c
index a8034411e77..543bd0b8725 100644
--- a/sbin/iked/ca.c
+++ b/sbin/iked/ca.c
@@ -65,7 +65,7 @@ intca_privkey_to_method(struct iked_id *);
 struct ibuf *
 ca_x509_serialize(X509 *);
 int ca_x509_subjectaltname_cmp(X509 *, struct iked_static_id *);
-int ca_x509_subjectaltname(X509 *cert, struct iked_id *);
+int ca_x509_subjectaltname(X509 *cert, struct iked_id *, int);
 int ca_dispatch_parent(int, struct privsep_proc *, struct imsg *);
 int ca_dispatch_ikev2(int, struct privsep_proc *, struct imsg *);
 
@@ -1400,34 +1400,31 @@ ca_x509_subjectaltname_cmp(X509 *cert, struct 
iked_static_id *id)
 {
struct iked_id   sanid;
char idstr[IKED_ID_SIZE];
-   int  ret = -1;
-
-   bzero(, sizeof(sanid));
-
-   if (ca_x509_subjectaltname(cert, ) != 0)
-   return (-1);
-
-   ikev2_print_id(, idstr, sizeof(idstr));
-
-   /* Compare id types, length and data */
-   if ((id->id_type != sanid.id_type) ||
-   ((ssize_t)ibuf_size(sanid.id_buf) !=
-   (id->id_length - id->id_offset)) ||
-   (memcmp(id->id_data + id->id_offset,
-   ibuf_data(sanid.id_buf),
-   ibuf_size(sanid.id_buf)) != 0)) {
+   int  ret = -1, lastpos = -1;
+
+   while (ca_x509_subjectaltname(cert, , lastpos++) == 0) {
+   ikev2_print_id(, idstr, sizeof(idstr));
+
+   /* Compare id types, length and data */
+   if ((id->id_type == sanid.id_type) &&
+   ((ssize_t)ibuf_size(sanid.id_buf) ==
+   (id->id_length - id->id_offset)) &&
+   (memcmp(id->id_data + id->id_offset,
+   ibuf_data(sanid.id_buf),
+   ibuf_size(sanid.id_buf)) == 0)) {
+   ret = 0;
+   break;
+   }
log_debug("%s: %s mismatched", __func__, idstr);
-   goto done;
+   bzero(, sizeof(sanid));
}
 
-   ret = 0;
- done:
ibuf_release(sanid.id_buf);
return (ret);
 }
 
 int
-ca_x509_subjectaltname(X509 *cert, struct iked_id *id)
+ca_x509_subjectaltname(X509 *cert, struct iked_id *id, int lastpos)
 {
X509_EXTENSION  *san;
uint8_t  sanhdr[4], *data;
@@ -1435,7 +1432,7 @@ ca_x509_subjectaltname(X509 *cert, struct iked_id *id)
char idstr[IKED_ID_SIZE];
 
if ((ext = X509_get_ext_by_NID(cert,
-   NID_subject_alt_name, -1)) == -1 ||
+   NID_subject_alt_name, lastpos)) == -1 ||
((san = X509_get_ext(cert, ext)) == NULL)) {
log_debug("%s: did not find subjectAltName in certificate",
__func__);

Re: cannot take the address of stdout

[Mark Kettenis]

> From: Jeremie Courreges-Anglas 
> Date: Thu, 19 Oct 2017 11:26:38 +0200
> 
> On Thu, Oct 19 2017, Jeremie Courreges-Anglas  wrote:
> > When building an updated version of ports/security/polarssl, I hit the
> > following failure:
> >
> > [219/427] /usr/ports/pobj/mbedtls-2.6.0/bin/cc  
> > -I/usr/ports/pobj/mbedtls-2.6.0/mbedtls-2.6.0/include 
> > -I/usr/ports/pobj/mbedtls-2.6.0/mbedtls-2.6.0/tests -O2 -pipe -Wall -Wextra 
> > -W -Wdeclaration-after-statement -Wwrite-strings -Wpointer-arith 
> > -Wimplicit-fallthrough -Wshadow -Wno-unused-function -DNDEBUG -MD -MT 
> > tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o
> >  -MF 
> > tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o.d
> >  -o 
> > tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o
> >-c tests/test_suite_hmac_drbg.misc.c
> > FAILED: 
> > tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o
> >  
> > /usr/ports/pobj/mbedtls-2.6.0/bin/cc  
> > -I/usr/ports/pobj/mbedtls-2.6.0/mbedtls-2.6.0/include 
> > -I/usr/ports/pobj/mbedtls-2.6.0/mbedtls-2.6.0/tests -O2 -pipe -Wall -Wextra 
> > -W -Wdeclaration-after-statement -Wwrite-strings -Wpointer-arith 
> > -Wimplicit-fallthrough -Wshadow -Wno-unused-function -DNDEBUG -MD -MT 
> > tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o
> >  -MF 
> > tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o.d
> >  -o 
> > tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o
> >-c tests/test_suite_hmac_drbg.misc.c
> > main_test.function:411:50: error: cannot take the address of an rvalue of 
> > type 'FILE *' (aka 'struct __sFILE *')
> > stdout_fd = redirect_output( , "/dev/null" );
> >  ^~~
> > main_test.function:423:56: error: cannot take the address of an rvalue of 
> > type 'FILE *' (aka 'struct __sFILE *')
> > if( !option_verbose && restore_output( , stdout_fd ) 
> > )
> >^~~
> > 2 errors generated.
> > ninja: build stopped: subcommand failed.
> >
> > And indeed, stdio.h declares stdout as follows:
> >
> >   #define   stdin   (&__sF[0])
> >   #define   stdout  (&__sF[1])
> >   #define   stderr  (&__sF[2])
> >
> > POSIX says pretty much the same as the C standard:
> >
> >   The  header shall define the following macros which shall
> >   expand to expressions of type "pointer to FILE" that point to the FILE
> >   objects associated, respectively, with the standard error, input, and
> >   output streams:
> >
> > So maybe libc should expose stdin/out/err as individual, modifiable
> > pointers?  This is what FreeBSD seems to do.
> 
> The C standard says:
> 
>   The primary use of the freopen function is to change the file
>   associated with a standard text stream (stderr, stdin, or stdout), as
>   those identifiers need not be modifiable lvalues to which the value
>   returned by the fopen function may be assigned.
> 
> so I guess that a valid way to "solve" this problem is just to fix the
> offending application.

Right.  The application is violating the standard.

Re: tftpd(8): diff for ip path rewrite

[Jeremie Courreges-Anglas]

On Wed, Oct 18 2017, Jan Klemkow  wrote:
> On Wed, Oct 18, 2017 at 08:37:48PM +, Jason McIntyre wrote:
>> On Wed, Oct 18, 2017 at 10:25:13PM +0200, Jan Klemkow wrote:
>> > This diff adds an option for client IP address path prefixes to the
>> > tftpd(8).  First, I used the -r rewrite socket for this, but...
>> > 
>> > If you use the rewrite socket feature, the tftpd(8) will exit with an
>> > error when the rewrite socket is closed.  A reopen of the socket is not
>> > possible, if its outside of the chroot directory.  And a privilege
>> > separated tftpd(8) is a bit overkill for a stable per client path
>> > rewrite feature.  This story led me to this change here.

I think it makes sense to support this feature without the need for an
additional unix service.

>> > Any suggestions or objections are welcome. :-)

Do we want to provide a fallback directory so that you don't need to
restart tftpd without -i to support unknown clients?

>> evening. some comments inline:
>
> Thanks.  Fixed diff:
>
> Index: tftpd.8
> ===
> RCS file: /mount/openbsd/cvs/src/usr.sbin/tftpd/tftpd.8,v
> retrieving revision 1.5
> diff -u -p -r1.5 tftpd.8
> --- tftpd.8   18 Jul 2015 05:32:56 -  1.5
> +++ tftpd.8   18 Oct 2017 21:12:52 -
> @@ -37,7 +37,7 @@
>  .Nd DARPA Trivial File Transfer Protocol daemon
>  .Sh SYNOPSIS
>  .Nm tftpd
> -.Op Fl 46cdv
> +.Op Fl 46cdiv
>  .Op Fl l Ar address
>  .Op Fl p Ar port
>  .Op Fl r Ar socket
> @@ -100,6 +100,11 @@ If this option is specified,
>  .Nm
>  will run in the foreground and log
>  the client IP, type of request, and filename to stderr.
> +.It Fl i
> +Use the client's IP address as a subdirectory prefix for all requested
> +filenames.
> +This option can not be combined with
> +.Fl r .
>  .It Fl l Ar address
>  Listen on the specified address.
>  By default
> @@ -126,6 +131,8 @@ before the TFTP request will continue.
>  By default
>  .Nm
>  does not use filename rewriting.
> +This option can not be combined with
> +.Fl i .
>  .It Fl v
>  Log the client IP, type of request, and filename.
>  .It Ar directory
> Index: tftpd.c
> ===
> RCS file: /mount/openbsd/cvs/src/usr.sbin/tftpd/tftpd.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 tftpd.c
> --- tftpd.c   26 May 2017 17:38:46 -  1.39
> +++ tftpd.c   18 Oct 2017 21:16:25 -
> @@ -282,7 +282,7 @@ __dead void
>  usage(void)
>  {
>   extern char *__progname;
> - fprintf(stderr, "usage: %s [-46cdv] [-l address] [-p port] [-r socket]"
> + fprintf(stderr, "usage: %s [-46cdiv] [-l address] [-p port] [-r socket]"
>   " directory\n", __progname);
>   exit(1);
>  }
> @@ -290,6 +290,7 @@ usage(void)
>  intcancreate = 0;
>  intverbose = 0;
>  intdebug = 0;
> +intiflag = 0;
>  
>  int
>  main(int argc, char *argv[])
> @@ -307,7 +308,7 @@ main(int argc, char *argv[])
>   int family = AF_UNSPEC;
>   int devnull = -1;
>  
> - while ((c = getopt(argc, argv, "46cdl:p:r:v")) != -1) {
> + while ((c = getopt(argc, argv, "46cdil:p:r:v")) != -1) {
>   switch (c) {
>   case '4':
>   family = AF_INET;
> @@ -321,6 +322,11 @@ main(int argc, char *argv[])
>   case 'd':
>   verbose = debug = 1;
>   break;
> + case 'i':
> + if (rewrite != NULL)
> + usage();
> + iflag = 1;
> + break;
>   case 'l':
>   addr = optarg;
>   break;
> @@ -328,6 +334,8 @@ main(int argc, char *argv[])
>   port = optarg;
>   break;
>   case 'r':
> + if (iflag)
> + usage();
>   rewrite = optarg;
>   break;
>   case 'v':
> @@ -903,7 +911,13 @@ again:
>  
>   if (rwmap != NULL)
>   rewrite_map(client, filename);
> - else
> + else if (iflag) {
> + char nfilename[PATH_MAX];
> +
> + snprintf(nfilename, sizeof nfilename, "%s/%s",
> + getip(>ss), filename);
> + tftp_open(client, nfilename);
> + } else
>   tftp_open(client, filename);
>  
>   return;
>

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

Re: KAME ioctl leftovers

[Alexander Bluhm]

On Wed, Oct 18, 2017 at 01:34:27PM +0200, Martin Pieuchot wrote:
> Kill ioctl(2) added with original KAME import that have never been used.
> FreeBSD also stopped supporting them in 2013.
> 
> ok?

OK bluhm@

> Index: sys/sockio.h
> ===
> RCS file: /cvs/src/sys/sys/sockio.h,v
> retrieving revision 1.70
> diff -u -p -r1.70 sockio.h
> --- sys/sockio.h  27 Jun 2017 22:18:24 -  1.70
> +++ sys/sockio.h  18 Oct 2017 11:28:25 -
> @@ -64,12 +64,6 @@
>  #define  SIOCGIFDATA _IOWR('i', 27, struct ifreq)/* get if_data 
> */
>  #define  SIOCSIFLLADDR   _IOW('i', 31, struct ifreq) /* set link 
> level addr */
>  
> -/* KAME IPv6 */
> -/* SIOCAIFALIAS? */
> -#define SIOCALIFADDR  _IOW('i', 28, struct if_laddrreq) /* add IF addr */
> -#define SIOCGLIFADDR _IOWR('i', 29, struct if_laddrreq) /* get IF addr */
> -#define SIOCDLIFADDR  _IOW('i', 30, struct if_laddrreq) /* delete IF addr */
> -
>  #define  SIOCADDMULTI _IOW('i', 49, struct ifreq)/* add m'cast 
> addr */
>  #define  SIOCDELMULTI _IOW('i', 50, struct ifreq)/* del m'cast 
> addr */
>  #define  SIOCGETVIFCNT   _IOWR('u', 51, struct sioc_vif_req)/* vif pkt 
> cnt */
> Index: netinet/in.c
> ===
> RCS file: /cvs/src/sys/netinet/in.c,v
> retrieving revision 1.140
> diff -u -p -r1.140 in.c
> --- netinet/in.c  11 Aug 2017 19:53:02 -  1.140
> +++ netinet/in.c  18 Oct 2017 11:29:04 -
> @@ -83,7 +83,6 @@
>  
>  
>  void in_socktrim(struct sockaddr_in *);
> -int in_lifaddr_ioctl(u_long, caddr_t, struct ifnet *, int);
>  
>  void in_purgeaddr(struct ifaddr *);
>  int in_addhost(struct in_ifaddr *, struct sockaddr_in *);
> @@ -182,9 +181,6 @@ in_nam2sin(const struct mbuf *nam, struc
>   return 0;
>  }
>  
> -/*
> - * Generic internet control operations (ioctl's).
> - */
>  int
>  in_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp)
>  {
> @@ -194,25 +190,13 @@ in_control(struct socket *so, u_long cmd
>   if ((so->so_state & SS_PRIV) != 0)
>   privileged++;
>  
> - switch (cmd) {
>  #ifdef MROUTING
> + switch (cmd) {
>   case SIOCGETVIFCNT:
>   case SIOCGETSGCNT:
>   return (mrt_ioctl(so, cmd, data));
> -#endif /* MROUTING */
> - case SIOCALIFADDR:
> - case SIOCDLIFADDR:
> - if (!privileged)
> - return (EPERM);
> - /* FALLTHROUGH */
> - case SIOCGLIFADDR:
> - if (ifp == NULL)
> - return (EINVAL);
> - return in_lifaddr_ioctl(cmd, data, ifp, privileged);
> - default:
> - if (ifp == NULL)
> - return (EOPNOTSUPP);
>   }
> +#endif /* MROUTING */
>  
>   return (in_ioctl(cmd, data, ifp, privileged));
>  }
> @@ -228,6 +212,9 @@ in_ioctl(u_long cmd, caddr_t data, struc
>   int error;
>   int newifaddr;
>  
> + if (ifp == NULL)
> + return (EOPNOTSUPP);
> +
>   NET_ASSERT_LOCKED();
>  
>   TAILQ_FOREACH(ifa, >if_addrlist, ifa_list) {
> @@ -413,187 +400,6 @@ in_ioctl(u_long cmd, caddr_t data, struc
>   }
>   return (0);
>  }
> -
> -/*
> - * SIOC[GAD]LIFADDR.
> - *   SIOCGLIFADDR: get first address. (???)
> - *   SIOCGLIFADDR with IFLR_PREFIX:
> - *   get first address that matches the specified prefix.
> - *   SIOCALIFADDR: add the specified address.
> - *   SIOCALIFADDR with IFLR_PREFIX:
> - *   EINVAL since we can't deduce hostid part of the address.
> - *   SIOCDLIFADDR: delete the specified address.
> - *   SIOCDLIFADDR with IFLR_PREFIX:
> - *   delete the first address that matches the specified prefix.
> - * return values:
> - *   EINVAL on invalid parameters
> - *   EADDRNOTAVAIL on prefix match failed/specified address not found
> - *   other values may be returned from in_ioctl()
> - */
> -int
> -in_lifaddr_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
> -{
> - struct if_laddrreq *iflr = (struct if_laddrreq *)data;
> - struct ifaddr *ifa;
> - struct sockaddr *sa;
> -
> - /* sanity checks */
> - if (!data || !ifp) {
> - panic("invalid argument to in_lifaddr_ioctl");
> - /*NOTRECHED*/
> - }
> -
> - switch (cmd) {
> - case SIOCGLIFADDR:
> - /* address must be specified on GET with IFLR_PREFIX */
> - if ((iflr->flags & IFLR_PREFIX) == 0)
> - break;
> - /*FALLTHROUGH*/
> - case SIOCALIFADDR:
> - case SIOCDLIFADDR:
> - /* address must be specified on ADD and DELETE */
> - sa = sstosa(>addr);
> - if (sa->sa_family != AF_INET)
> - return EINVAL;
> - if (sa->sa_len != sizeof(struct sockaddr_in))
> - return EINVAL;
> - /* XXX need improvement */
> - sa = 

Re: cannot take the address of stdout

[Jeremie Courreges-Anglas]

On Thu, Oct 19 2017, Jeremie Courreges-Anglas  wrote:
> When building an updated version of ports/security/polarssl, I hit the
> following failure:
>
> [219/427] /usr/ports/pobj/mbedtls-2.6.0/bin/cc  
> -I/usr/ports/pobj/mbedtls-2.6.0/mbedtls-2.6.0/include 
> -I/usr/ports/pobj/mbedtls-2.6.0/mbedtls-2.6.0/tests -O2 -pipe -Wall -Wextra 
> -W -Wdeclaration-after-statement -Wwrite-strings -Wpointer-arith 
> -Wimplicit-fallthrough -Wshadow -Wno-unused-function -DNDEBUG -MD -MT 
> tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o 
> -MF 
> tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o.d
>  -o 
> tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o  
>  -c tests/test_suite_hmac_drbg.misc.c
> FAILED: 
> tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o 
> /usr/ports/pobj/mbedtls-2.6.0/bin/cc  
> -I/usr/ports/pobj/mbedtls-2.6.0/mbedtls-2.6.0/include 
> -I/usr/ports/pobj/mbedtls-2.6.0/mbedtls-2.6.0/tests -O2 -pipe -Wall -Wextra 
> -W -Wdeclaration-after-statement -Wwrite-strings -Wpointer-arith 
> -Wimplicit-fallthrough -Wshadow -Wno-unused-function -DNDEBUG -MD -MT 
> tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o 
> -MF 
> tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o.d
>  -o 
> tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o  
>  -c tests/test_suite_hmac_drbg.misc.c
> main_test.function:411:50: error: cannot take the address of an rvalue of 
> type 'FILE *' (aka 'struct __sFILE *')
> stdout_fd = redirect_output( , "/dev/null" );
>  ^~~
> main_test.function:423:56: error: cannot take the address of an rvalue of 
> type 'FILE *' (aka 'struct __sFILE *')
> if( !option_verbose && restore_output( , stdout_fd ) )
>^~~
> 2 errors generated.
> ninja: build stopped: subcommand failed.
>
> And indeed, stdio.h declares stdout as follows:
>
>   #define stdin   (&__sF[0])
>   #define stdout  (&__sF[1])
>   #define stderr  (&__sF[2])
>
> POSIX says pretty much the same as the C standard:
>
>   The  header shall define the following macros which shall
>   expand to expressions of type "pointer to FILE" that point to the FILE
>   objects associated, respectively, with the standard error, input, and
>   output streams:
>
> So maybe libc should expose stdin/out/err as individual, modifiable
> pointers?  This is what FreeBSD seems to do.

The C standard says:

  The primary use of the freopen function is to change the file
  associated with a standard text stream (stderr, stdin, or stdout), as
  those identifiers need not be modifiable lvalues to which the value
  returned by the fopen function may be assigned.

so I guess that a valid way to "solve" this problem is just to fix the
offending application.

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

Re: if_ioctl is not NULL

[Alexander Bluhm]

On Wed, Oct 18, 2017 at 01:42:57PM +0200, Martin Pieuchot wrote:
> All are drivers provides it and if_attach() now asserts that it is not
> NULL.
> 
> Let's get rid of those checks, ok?

OK bluhm@

> Index: netinet/in.c
> ===
> RCS file: /cvs/src/sys/netinet/in.c,v
> retrieving revision 1.140
> diff -u -p -r1.140 in.c
> --- netinet/in.c  11 Aug 2017 19:53:02 -  1.140
> +++ netinet/in.c  18 Oct 2017 11:40:41 -
> @@ -406,8 +406,6 @@ in_ioctl(u_long cmd, caddr_t data, struc
>   break;
>  
>   default:
> - if (ifp->if_ioctl == NULL)
> - return (EOPNOTSUPP);
>   error = ((*ifp->if_ioctl)(ifp, cmd, data));
>   return (error);
>   }
> @@ -826,9 +824,6 @@ in_addmulti(struct in_addr *ap, struct i
>*/
>   ++inm->inm_refcnt;
>   } else {
> - if (ifp->if_ioctl == NULL)
> - return (NULL);
> -
>   /*
>* New address; allocate a new multicast record
>* and link it into the interface's multicast list.
> Index: netinet6/in6.c
> ===
> RCS file: /cvs/src/sys/netinet6/in6.c,v
> retrieving revision 1.212
> diff -u -p -r1.212 in6.c
> --- netinet6/in6.c16 Oct 2017 13:40:58 -  1.212
> +++ netinet6/in6.c18 Oct 2017 11:40:03 -
> @@ -489,8 +489,6 @@ in6_ioctl(u_long cmd, caddr_t data, stru
>   break;
>  
>   default:
> - if (ifp->if_ioctl == NULL)
> - return (EOPNOTSUPP);
>   error = ((*ifp->if_ioctl)(ifp, cmd, data));
>   return (error);
>   }
> @@ -1247,11 +1245,6 @@ in6_addmulti(struct in6_addr *maddr6, st
>*/
>   in6m->in6m_refcnt++;
>   } else {
> - if (ifp->if_ioctl == NULL) {
> - *errorp = ENXIO; /* XXX: appropriate? */
> - return (NULL);
> - }
> -
>   /*
>* New address; allocate a new multicast record
>* and link it into the interface's multicast list.

Re: ksh: remove the deprecated emacs-usemeta option

[Jeremie Courreges-Anglas]

On Thu, Oct 19 2017, Sebastian Benoit  wrote:
> Jeremie Courreges-Anglas(j...@wxcvbn.org) on 2017.10.18 22:16:48 +0200:
>> 
>> This would make ''set -o emacs-usemeta'' a fatal error, which means that
>> subsequent lines in your kshrc will not be run.  I think people were
>> given enough time to cope with this (6.2 users get a warning).
>> 
>> ok?
>
> no please wait until 6.3 is out. Is it in the way of something else?

It's already been committed.  If you or someone else want to keep it for
one additional release, I have no objection.

>> 
>> 
>> Index: misc.c
>> ===
>> RCS file: /d/cvs/src/bin/ksh/misc.c,v
>> retrieving revision 1.59
>> diff -u -p -r1.59 misc.c
>> --- misc.c   30 Aug 2017 17:15:36 -  1.59
>> +++ misc.c   18 Oct 2017 19:41:04 -
>> @@ -129,7 +129,6 @@ const struct option options[] = {
>>  { "csh-history",  0,OF_ANY }, /* non-standard */
>>  #ifdef EMACS
>>  { "emacs",0,OF_ANY },
>> -{ "emacs-usemeta",  0,  OF_ANY }, /* XXX delete after 6.2 */
>>  #endif
>>  { "errexit",'e',OF_ANY },
>>  #ifdef EMACS
>> @@ -185,13 +184,8 @@ option(const char *n)
>>  int i;
>>  
>>  for (i = 0; i < NELEM(options); i++)
>> -if (options[i].name && strcmp(options[i].name, n) == 0) {
>> -#ifdef EMACS
>> -if (i == FEMACSUSEMETA)
>> -warningf(true, "%s: deprecated option", n);
>> -#endif
>> +if (options[i].name && strcmp(options[i].name, n) == 0)
>>  return i;
>> -}
>>  
>>  return -1;
>>  }
>> @@ -232,10 +226,6 @@ printoptions(int verbose)
>>  shprintf("Current option settings\n");
>>  
>>  for (i = n = oi.opt_width = 0; i < NELEM(options); i++) {
>> -#ifdef EMACS
>> -if (i == FEMACSUSEMETA)
>> -continue;
>> -#endif
>>  if (options[i].name) {
>>  len = strlen(options[i].name);
>>  oi.opts[n].name = options[i].name;
>> @@ -250,10 +240,6 @@ printoptions(int verbose)
>>  /* short version ala ksh93 */
>>  shprintf("set");
>>  for (i = 0; i < NELEM(options); i++) {
>> -#ifdef EMACS
>> -if (i == FEMACSUSEMETA)
>> -continue;
>> -#endif
>>  if (options[i].name)
>>  shprintf(" %co %s",
>>   Flag(i) ? '-' : '+',
>> Index: sh.h
>> ===
>> RCS file: /d/cvs/src/bin/ksh/sh.h,v
>> retrieving revision 1.64
>> diff -u -p -r1.64 sh.h
>> --- sh.h 3 Sep 2017 11:52:01 -   1.64
>> +++ sh.h 18 Oct 2017 16:12:59 -
>> @@ -139,7 +139,6 @@ enum sh_flag {
>>  FCSHHISTORY,/* csh-style history enabled */
>>  #ifdef EMACS
>>  FEMACS, /* emacs command editing */
>> -FEMACSUSEMETA,  /* XXX delete after 6.2 */
>>  #endif
>>  FERREXIT,   /* -e: quit on error */
>>  #ifdef EMACS
>> 
>> 
>> -- 
>> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
>> 
>

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

Re: ksh: skip long history lines

[Jeremie Courreges-Anglas]

On Wed, Oct 18 2017, Ori Bernstein  wrote:
> On Thu, 19 Oct 2017 00:22:51 +0200
> Jeremie Courreges-Anglas  wrote:
>
>> Those variables are static so that error messages are only printed once
>> in the shell lifetime.  history reload happens each time the shell
>> detects that the histfile has been modified, which can happen often if
>> like me you have multiple interactives shells running at the same time.
>  
> Makes sense, although I had been thinking that it could lead to lost
> warnings when changing histfile; I'd hope that a corrupt histfile isn't
> a persistent condition.

That's a fair point, I think I understand your previous proposal better.

Warning only once per histfile would need something like a file-global
variable - that we should reset in hist_init.  This sounds like extra
quirkiness that would make the code more complicated for a limited
benefit.  In the end, it might eat some of the precious 24 lines of your
terminal, but you can also just fix the offending lines.

> In any case, this looks pretty good to me.

Thanks for the feedback.

>> > Maybe hoist this outside of the loop.
>> 
>> I don't see a reason for this.

(Now I see.)

[...]

Updated diff.  The logic has changed, hopefully giving more accurate
warning messages.

Feedback / oks welcome.


Index: history.c
===
RCS file: /d/cvs/src/bin/ksh/history.c,v
retrieving revision 1.72
diff -u -p -p -u -r1.72 history.c
--- history.c   18 Oct 2017 15:41:25 -  1.72
+++ history.c   19 Oct 2017 07:56:05 -
@@ -740,23 +740,38 @@ static void
 history_load(Source *s)
 {
char*p, encoded[LINE + 1], line[LINE + 1];
+   int  toolongseen = 0;
 
rewind(histfh);
+   line_co = 1;
 
/* just read it all; will auto resize history upon next command */
-   for (line_co = 1; ; line_co++) {
-   p = fgets(encoded, sizeof(encoded), histfh);
-   if (p == NULL || feof(histfh) || ferror(histfh))
-   break;
+   while (fgets(encoded, sizeof(encoded), histfh)) {
if ((p = strchr(encoded, '\n')) == NULL) {
-   bi_errorf("history file is corrupt");
-   return;
+   /* discard overlong line */
+   do {
+   /* maybe a missing trailing newline? */
+   if (strlen(encoded) != sizeof(encoded) - 1) {
+   bi_errorf("history file is corrupt");
+   return;
+   }
+   } while (fgets(encoded, sizeof(encoded), histfh)
+   && strchr(encoded, '\n') == NULL);
+
+   if (!toolongseen) {
+   toolongseen = 1;
+   bi_errorf("ignored history line(s) longer than"
+   " %d bytes", LINE);
+   }
+
+   continue;
}
*p = '\0';
s->line = line_co;
s->cmd_offset = line_co;
strunvis(line, encoded);
histsave(line_co, line, 0);
+   line_co++;
}
 
history_write();

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

Re: ksh: extra rewind call

[Jeremie Courreges-Anglas]

On Thu, Oct 19 2017, Sebastian Benoit  wrote:
> Jeremie Courreges-Anglas(j...@wxcvbn.org) on 2017.10.19 00:36:00 +0200:
>> 
>> This call was added along with the magic check, but it not actually
>> needed: history_load already calls rewind(3).  I feel like the magic
>> check should also be in history_load(), so that a ksh process can
>> recover at runtime from a binary->plaintext history file migration.
>> Worth the trouble?
>
> unsure, without extra work (backport) only people who upgrade from 6.1 (and
> earlier) to 6.3 or snap from sometime in summer to now would benefit, right?

I took a look and it seems a bit more involved than I initially thought,
so I just abandoned the idea.

>> ok for the diff below?
>
> yes ok benno

Thanks,

>
>> Index: history.c
>> ===
>> RCS file: /d/cvs/src/bin/ksh/history.c,v
>> retrieving revision 1.72
>> diff -u -p -p -u -r1.72 history.c
>> --- history.c18 Oct 2017 15:41:25 -  1.72
>> +++ history.c18 Oct 2017 22:25:23 -
>> @@ -799,8 +799,6 @@ hist_init(Source *s)
>>  return;
>>  }
>>  
>> -rewind(histfh);
>> -
>>  history_load(s);
>>  
>>  history_lock(LOCK_UN);
>> 
>> 
>> -- 
>> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
>> 
>

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

Re: ksh: remove the deprecated emacs-usemeta option

[Sebastian Benoit]

Jeremie Courreges-Anglas(j...@wxcvbn.org) on 2017.10.18 22:16:48 +0200:
> 
> This would make ''set -o emacs-usemeta'' a fatal error, which means that
> subsequent lines in your kshrc will not be run.  I think people were
> given enough time to cope with this (6.2 users get a warning).
> 
> ok?

no please wait until 6.3 is out. Is it in the way of something else?

> 
> 
> Index: misc.c
> ===
> RCS file: /d/cvs/src/bin/ksh/misc.c,v
> retrieving revision 1.59
> diff -u -p -r1.59 misc.c
> --- misc.c30 Aug 2017 17:15:36 -  1.59
> +++ misc.c18 Oct 2017 19:41:04 -
> @@ -129,7 +129,6 @@ const struct option options[] = {
>   { "csh-history",  0,OF_ANY }, /* non-standard */
>  #ifdef EMACS
>   { "emacs",0,OF_ANY },
> - { "emacs-usemeta",  0,  OF_ANY }, /* XXX delete after 6.2 */
>  #endif
>   { "errexit",'e',OF_ANY },
>  #ifdef EMACS
> @@ -185,13 +184,8 @@ option(const char *n)
>   int i;
>  
>   for (i = 0; i < NELEM(options); i++)
> - if (options[i].name && strcmp(options[i].name, n) == 0) {
> -#ifdef EMACS
> - if (i == FEMACSUSEMETA)
> - warningf(true, "%s: deprecated option", n);
> -#endif
> + if (options[i].name && strcmp(options[i].name, n) == 0)
>   return i;
> - }
>  
>   return -1;
>  }
> @@ -232,10 +226,6 @@ printoptions(int verbose)
>   shprintf("Current option settings\n");
>  
>   for (i = n = oi.opt_width = 0; i < NELEM(options); i++) {
> -#ifdef EMACS
> - if (i == FEMACSUSEMETA)
> - continue;
> -#endif
>   if (options[i].name) {
>   len = strlen(options[i].name);
>   oi.opts[n].name = options[i].name;
> @@ -250,10 +240,6 @@ printoptions(int verbose)
>   /* short version ala ksh93 */
>   shprintf("set");
>   for (i = 0; i < NELEM(options); i++) {
> -#ifdef EMACS
> - if (i == FEMACSUSEMETA)
> - continue;
> -#endif
>   if (options[i].name)
>   shprintf(" %co %s",
>Flag(i) ? '-' : '+',
> Index: sh.h
> ===
> RCS file: /d/cvs/src/bin/ksh/sh.h,v
> retrieving revision 1.64
> diff -u -p -r1.64 sh.h
> --- sh.h  3 Sep 2017 11:52:01 -   1.64
> +++ sh.h  18 Oct 2017 16:12:59 -
> @@ -139,7 +139,6 @@ enum sh_flag {
>   FCSHHISTORY,/* csh-style history enabled */
>  #ifdef EMACS
>   FEMACS, /* emacs command editing */
> - FEMACSUSEMETA,  /* XXX delete after 6.2 */
>  #endif
>   FERREXIT,   /* -e: quit on error */
>  #ifdef EMACS
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 

Re: ksh: skip long history lines

[Sebastian Benoit]

Jeremie Courreges-Anglas(j...@wxcvbn.org) on 2017.10.19 00:22:51 +0200:
> 
> Hi Ori,
> 
> thanks for your feedback.  Reply and updated diff below,
> 
> On Wed, Oct 18 2017, Ori Bernstein  wrote:
> > On Wed, 18 Oct 2017 22:33:51 +0200
> > Jeremie Courreges-Anglas  wrote:
> >
> >> 
> >> It would be nice to support arbitrarily long lines, but a first step
> >> would be to skip them gracefuly.
> >> 
> >> The code modifies the loop condition: no tests against ferror(3)/feof(3)
> >> are performed any more (I don't see their point).
> >> 
> >> We could print a warning on ferror(), though, but that would be another
> >> diff.
> >> 
> >> ok?

o

> >> 
> >> 
> >> Index: history.c
> >> ===
> >> RCS file: /d/cvs/src/bin/ksh/history.c,v
> >> retrieving revision 1.72
> >> diff -u -p -p -u -r1.72 history.c
> >> --- history.c  18 Oct 2017 15:41:25 -  1.72
> >> +++ history.c  18 Oct 2017 20:18:24 -
> >> @@ -739,24 +739,45 @@ history_close(void)
> >>  static void
> >>  history_load(Source *s)
> >>  {
> >> -  char*p, encoded[LINE + 1], line[LINE + 1];
> >> +  char*nl, *p, encoded[LINE + 1], line[LINE + 1];
> >>  
> >>rewind(histfh);
> >> +  line_co = 1;
> >>  
> >>/* just read it all; will auto resize history upon next command */
> >> -  for (line_co = 1; ; line_co++) {
> >> -  p = fgets(encoded, sizeof(encoded), histfh);
> >> -  if (p == NULL || feof(histfh) || ferror(histfh))
> >> -  break;
> >> -  if ((p = strchr(encoded, '\n')) == NULL) {
> >> -  bi_errorf("history file is corrupt");
> >> -  return;
> >> +  for (p = fgets(encoded, sizeof(encoded), histfh); p != NULL;
> >> +   p = fgets(encoded, sizeof(encoded), histfh)) {
> >
> > The redundant calls prevent this code from scanning as well as it
> > could for me. Perhaps:
> >
> > for(;;) {
> > if ((p = fgets(...) == NULL)
> > break;
> >
> > Just a minor cosmetic nitpick, it's not a big deal either way.
> 
> I must admit that the code is needlessly redundant, but I prefer to have
> the loop condition inside... the loop condition.
> 
> >> +  if ((nl = strchr(encoded, '\n')) == NULL) {
> >> +  static int corrupt, toolong;
> >
> > I'm not sure why these need to be static. I'm not an expert on
> > the code, but it looks like histsave can be called multiple
> > times as the shell runs, which triggers a history reload.
> > Keeping this state seems like it would be wrong.
> 
> Those variables are static so that error messages are only printed once
> in the shell lifetime.  history reload happens each time the shell
> detects that the histfile has been modified, which can happen often if
> like me you have multiple interactives shells running at the same time.
> 
> > Maybe hoist this outside of the loop.
> 
> I don't see a reason for this.
> 
> >> +  /* no trailing newline? */
> >> +  if (strlen(p) != sizeof(encoded) - 1) {
> >> +  if (!corrupt) {
> >> +  corrupt = 1;
> >> +  bi_errorf("history file is corrupt");
> >> +  }
> >> +  return;
> >
> > Why does `corrupt` need to be recorded at all? There's a
> > return right after it.
> 
> Also to limit the amount of crap printed on screen, though maybe we
> should warn every time in this case.  Here's an updated diff that does
> so and hopefully makes the intents clearer.  It also adds a comment
> suggested by anton@
> 
> If your histfile ends with an overlong line which is also not terminated
> by a newline, the do...while loop would silently ignore this last line.
> I think that's acceptable, but YMMV.
> 
> 
> Index: history.c
> ===
> RCS file: /d/cvs/src/bin/ksh/history.c,v
> retrieving revision 1.72
> diff -u -p -p -u -r1.72 history.c
> --- history.c 18 Oct 2017 15:41:25 -  1.72
> +++ history.c 18 Oct 2017 22:16:43 -
> @@ -739,24 +739,42 @@ history_close(void)
>  static void
>  history_load(Source *s)
>  {
> - char*p, encoded[LINE + 1], line[LINE + 1];
> + char*nl, *p, encoded[LINE + 1], line[LINE + 1];
>  
>   rewind(histfh);
> + line_co = 1;
>  
>   /* just read it all; will auto resize history upon next command */
> - for (line_co = 1; ; line_co++) {
> - p = fgets(encoded, sizeof(encoded), histfh);
> - if (p == NULL || feof(histfh) || ferror(histfh))
> - break;
> - if ((p = strchr(encoded, '\n')) == NULL) {
> - bi_errorf("history file is corrupt");
> - return;
> + while ((p = fgets(encoded, sizeof(encoded), histfh)) != NULL) {
> + if ((nl = 

Re: ksh: extra rewind call

[Sebastian Benoit]

Jeremie Courreges-Anglas(j...@wxcvbn.org) on 2017.10.19 00:36:00 +0200:
> 
> This call was added along with the magic check, but it not actually
> needed: history_load already calls rewind(3).  I feel like the magic
> check should also be in history_load(), so that a ksh process can
> recover at runtime from a binary->plaintext history file migration.
> Worth the trouble?

unsure, without extra work (backport) only people who upgrade from 6.1 (and
earlier) to 6.3 or snap from sometime in summer to now would benefit, right?

> ok for the diff below?

yes ok benno


> Index: history.c
> ===
> RCS file: /d/cvs/src/bin/ksh/history.c,v
> retrieving revision 1.72
> diff -u -p -p -u -r1.72 history.c
> --- history.c 18 Oct 2017 15:41:25 -  1.72
> +++ history.c 18 Oct 2017 22:25:23 -
> @@ -799,8 +799,6 @@ hist_init(Source *s)
>   return;
>   }
>  
> - rewind(histfh);
> -
>   history_load(s);
>  
>   history_lock(LOCK_UN);
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 

Re: fix memory handling in acme-client config parser

[Sebastian Benoit]

sure ok benno@

Jonathan Gray(j...@jsg.id.au) on 2017.10.19 16:33:35 +1100:
> Use after free and a memory leak.
> 
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/acme-client/parse.y,v
> retrieving revision 1.17
> diff -u -p -U4 -r1.17 parse.y
> --- parse.y   23 Mar 2017 12:59:32 -  1.17
> +++ parse.y   19 Oct 2017 04:50:29 -
> @@ -224,10 +224,10 @@ domain  : DOMAIN STRING {
>   char *s;
>   if ((s = strdup($2)) == NULL)
>   err(EXIT_FAILURE, "strdup");
>   if (!domain_valid(s)) {
> - free(s);
>   yyerror("%s: bad domain syntax", s);
> + free(s);
>   YYERROR;
>   }
>   if ((domain = conf_new_domain(conf, s)) == NULL) {
>   free(s);
> @@ -335,8 +335,9 @@ domainoptsl   : ALTERNATIVE NAMES '{' altn
>   if ((s = strdup($3)) == NULL)
>   err(EXIT_FAILURE, "strdup");
>   if (authority_find(conf, s) == NULL) {
>   yyerror("use: unknown authority");
> + free(s);
>   YYERROR;
>   }
>   domain->auth = s;
>   }
> 

Re: uninitialised variable crashes in bgpd config parser

[Sebastian Benoit]

ok benno@

Jonathan Gray(j...@jsg.id.au) on 2017.10.19 16:34:35 +1100:
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
> retrieving revision 1.315
> diff -u -p -r1.315 parse.y
> --- parse.y   21 Aug 2017 14:41:22 -  1.315
> +++ parse.y   19 Oct 2017 05:32:03 -
> @@ -3181,7 +3181,7 @@ parseextcommunity(struct filter_extcommu
>   switch (type) {
>   case -1:
>   if ((p = strchr(s, ':')) == NULL) {
> - yyerror("Bad ext-community %s is %s", s, errstr);
> + yyerror("Bad ext-community %s", s);
>   return (-1);
>   }
>   *p++ = '\0';
> @@ -3239,7 +3239,7 @@ parseextcommunity(struct filter_extcommu
>   else if (strcmp(s, "not-found") == 0)
>   c->data.ext_opaq = EXT_COMMUNITY_OVS_NOTFOUND;
>   else {
> - yyerror("Bad ext-community %s is %s", s, errstr);
> + yyerror("Bad ext-community %s", s);
>   return (-1);
>   }
>   break;
> 

cannot take the address of stdout

[Jeremie Courreges-Anglas]

When building an updated version of ports/security/polarssl, I hit the
following failure:

[219/427] /usr/ports/pobj/mbedtls-2.6.0/bin/cc  
-I/usr/ports/pobj/mbedtls-2.6.0/mbedtls-2.6.0/include 
-I/usr/ports/pobj/mbedtls-2.6.0/mbedtls-2.6.0/tests -O2 -pipe -Wall -Wextra -W 
-Wdeclaration-after-statement -Wwrite-strings -Wpointer-arith 
-Wimplicit-fallthrough -Wshadow -Wno-unused-function -DNDEBUG -MD -MT 
tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o 
-MF 
tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o.d 
-o tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o 
  -c tests/test_suite_hmac_drbg.misc.c
FAILED: 
tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o 
/usr/ports/pobj/mbedtls-2.6.0/bin/cc  
-I/usr/ports/pobj/mbedtls-2.6.0/mbedtls-2.6.0/include 
-I/usr/ports/pobj/mbedtls-2.6.0/mbedtls-2.6.0/tests -O2 -pipe -Wall -Wextra -W 
-Wdeclaration-after-statement -Wwrite-strings -Wpointer-arith 
-Wimplicit-fallthrough -Wshadow -Wno-unused-function -DNDEBUG -MD -MT 
tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o 
-MF 
tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o.d 
-o tests/CMakeFiles/test_suite_hmac_drbg.misc.dir/test_suite_hmac_drbg.misc.c.o 
  -c tests/test_suite_hmac_drbg.misc.c
main_test.function:411:50: error: cannot take the address of an rvalue of type 
'FILE *' (aka 'struct __sFILE *')
stdout_fd = redirect_output( , "/dev/null" );
 ^~~
main_test.function:423:56: error: cannot take the address of an rvalue of type 
'FILE *' (aka 'struct __sFILE *')
if( !option_verbose && restore_output( , stdout_fd ) )
   ^~~
2 errors generated.
ninja: build stopped: subcommand failed.

And indeed, stdio.h declares stdout as follows:

  #define   stdin   (&__sF[0])
  #define   stdout  (&__sF[1])
  #define   stderr  (&__sF[2])

POSIX says pretty much the same as the C standard:

  The  header shall define the following macros which shall
  expand to expressions of type "pointer to FILE" that point to the FILE
  objects associated, respectively, with the standard error, input, and
  output streams:

So maybe libc should expose stdin/out/err as individual, modifiable
pointers?  This is what FreeBSD seems to do.

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

Re: Httpd support for internal redirects.

[Gregory Edigarov]

as a, well, co-author of some earlier attempt on this, the developers 
just do not interested in this.
so we are left on our own to maintain this. as a side note my diff is 
more functional then your's
as it provides a way to test  the existence of the filesystem object 
before rewrite happen.





On 19.10.17 00:39, Ori Bernstein wrote:

Pinging this patch.

On Tue, 10 Oct 2017 21:31:20 -0700
Ori Bernstein  wrote:


My website generator is a little stupid at times. It generates
files with .html suffixes, but urls without them.

I worked around this with some redirects, but it never felt
quite right doing an extra round trip. Therefore, I added
internal redirects, processing the rewrite before responding to
an http request.

This introduces new syntax to the config file, allowing you to
do:

location match "^(/foo/bar/[%w]+)$" {
rewrite-to "/baz/%1.html"
}

Because we don't know what the paths should be relative
to, all paths rewritten must be absolute.

It seems like someone else may find it useful[1], so
I'm submitting it. I've been running a slightly older
version of this on https://myrlang.org for the last
day or two, and it's been uneventful. The difference
is that the syntax used to piggy back off the "block"
action => 'block internal return 302 "path"'.

This doesn't currently support chained rewrites. I think
that it wouldn't be hard to add if it's needed.

Ok?

[1] https://github.com/reyk/httpd/issues/27
==


diff --git usr.sbin/httpd/config.c usr.sbin/httpd/config.c
index 3c31c3d4cd3..7d2982af7b9 100644
--- usr.sbin/httpd/config.c
+++ usr.sbin/httpd/config.c
@@ -448,7 +448,7 @@ config_getserver_config(struct httpd *env, struct server 
*srv,
sizeof(srv_conf->errorlog));
}
  
-		f = SRVFLAG_BLOCK|SRVFLAG_NO_BLOCK;

+   f = SRVFLAG_BLOCK|SRVFLAG_NO_BLOCK|SRVFLAG_REWRITE;
if ((srv_conf->flags & f) == 0) {
free(srv_conf->return_uri);
srv_conf->flags |= parent->flags & f;
diff --git usr.sbin/httpd/httpd.conf.5 usr.sbin/httpd/httpd.conf.5
index a3c97629de3..3a00a750537 100644
--- usr.sbin/httpd/httpd.conf.5
+++ usr.sbin/httpd/httpd.conf.5
@@ -454,6 +454,14 @@ instead of the log files.
  Disable any previous
  .Ic block
  in a location.
+.It Ic rewrite-to Ar path
+The current request path is rewritten to
+.Ar  path .
+using the same macro expansions as
+.Cm block return
+rules. After macros are substituted, the resulting paths must be
+absolute, beginning with a slash.  Rewriting is not done
+recursively.
  .It Ic root Ar option
  Configure the document root and options for the request path.
  Valid options are:
diff --git usr.sbin/httpd/httpd.h usr.sbin/httpd/httpd.h
index 05cbb8e3550..477115ec92d 100644
--- usr.sbin/httpd/httpd.h
+++ usr.sbin/httpd/httpd.h
@@ -394,6 +394,7 @@ SPLAY_HEAD(client_tree, client);
  #define SRVFLAG_SERVER_MATCH  0x0020
  #define SRVFLAG_SERVER_HSTS   0x0040
  #define SRVFLAG_DEFAULT_TYPE  0x0080
+#define SRVFLAG_REWRITE0x0100
  
  #define SRVFLAG_BITS			\

"\10\01INDEX\02NO_INDEX\03AUTO_INDEX\04NO_AUTO_INDEX" \
diff --git usr.sbin/httpd/parse.y usr.sbin/httpd/parse.y
index fcf1938c42d..4072ee5b532 100644
--- usr.sbin/httpd/parse.y
+++ usr.sbin/httpd/parse.y
@@ -134,7 +134,7 @@ typedef struct {
  %tokenLISTEN LOCATION LOG LOGDIR MATCH MAXIMUM NO NODELAY OCSP ON 
PORT PREFORK
  %tokenPROTOCOLS REQUESTS ROOT SACK SERVER SOCKET STRIP STYLE SYSLOG 
TCP TICKET
  %tokenTIMEOUT TLS TYPE TYPES HSTS MAXAGE SUBDOMAINS DEFAULT PRELOAD 
REQUEST
-%token ERROR INCLUDE AUTHENTICATE WITH BLOCK DROP RETURN PASS
+%token ERROR INCLUDE AUTHENTICATE WITH BLOCK DROP RETURN REWRITE PASS
  %tokenSTRING
  %tokenNUMBER
  %type   port
@@ -986,6 +986,11 @@ filter : block RETURN NUMBER optstring {
srv_conf->return_uri_len = strlen($4) + 1;
}
}
+   | REWRITE STRING {
+   srv_conf->flags |= SRVFLAG_REWRITE;
+   srv_conf->return_uri = $2;
+   srv_conf->return_uri_len = strlen($2) + 1;
+   }
| block DROP{
/* No return code, silently drop the connection */
srv_conf->return_code = 0;
@@ -1255,6 +1260,7 @@ lookup(char *s)
{ "request",  REQUEST },
{ "requests", REQUESTS },
{ "return",   RETURN },
+   { "rewrite-to",   REWRITE },
{ "root", ROOT },
{ "sack", SACK },
{ "server",   SERVER },
diff --git usr.sbin/httpd/server_http.c usr.sbin/httpd/server_http.c
index 

Re: fix memory handling in acme-client config parser

[Florian Obser]

OK florian@

On Thu, Oct 19, 2017 at 05:33:35AM +, Jonathan Gray wrote:
> Use after free and a memory leak.
> 
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/acme-client/parse.y,v
> retrieving revision 1.17
> diff -u -p -U4 -r1.17 parse.y
> --- parse.y   23 Mar 2017 12:59:32 -  1.17
> +++ parse.y   19 Oct 2017 04:50:29 -
> @@ -224,10 +224,10 @@ domain  : DOMAIN STRING {
>   char *s;
>   if ((s = strdup($2)) == NULL)
>   err(EXIT_FAILURE, "strdup");
>   if (!domain_valid(s)) {
> - free(s);
>   yyerror("%s: bad domain syntax", s);
> + free(s);
>   YYERROR;
>   }
>   if ((domain = conf_new_domain(conf, s)) == NULL) {
>   free(s);
> @@ -335,8 +335,9 @@ domainoptsl   : ALTERNATIVE NAMES '{' altn
>   if ((s = strdup($3)) == NULL)
>   err(EXIT_FAILURE, "strdup");
>   if (authority_find(conf, s) == NULL) {
>   yyerror("use: unknown authority");
> + free(s);
>   YYERROR;
>   }
>   domain->auth = s;
>   }
> 

-- 
I'm not entirely sure you are real.

Re: uninitialised variable crashes in bgpd config parser

[Claudio Jeker]

OK claudio@

On Thu, Oct 19, 2017 at 04:34:35PM +1100, Jonathan Gray wrote:
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
> retrieving revision 1.315
> diff -u -p -r1.315 parse.y
> --- parse.y   21 Aug 2017 14:41:22 -  1.315
> +++ parse.y   19 Oct 2017 05:32:03 -
> @@ -3181,7 +3181,7 @@ parseextcommunity(struct filter_extcommu
>   switch (type) {
>   case -1:
>   if ((p = strchr(s, ':')) == NULL) {
> - yyerror("Bad ext-community %s is %s", s, errstr);
> + yyerror("Bad ext-community %s", s);
>   return (-1);
>   }
>   *p++ = '\0';
> @@ -3239,7 +3239,7 @@ parseextcommunity(struct filter_extcommu
>   else if (strcmp(s, "not-found") == 0)
>   c->data.ext_opaq = EXT_COMMUNITY_OVS_NOTFOUND;
>   else {
> - yyerror("Bad ext-community %s is %s", s, errstr);
> + yyerror("Bad ext-community %s", s);
>   return (-1);
>   }
>   break;
> 

-- 
:wq Claudio