Re: src/usr.sbin/slowcgi: possible bug

[Florian Obser]

On Mon, Jan 02, 2017 at 04:29:21PM +0330, temp+...@frad.ir wrote:
> Hi tech@,
> 
> I recently checked the slowcgi(8) and found that it might have an issue
> when buf_pos is at the end of buffer and buf_len is zero.
> 
> Am I right?

we can simplify this even more. There is no need to remember the
buffer position outside of this function. It will be 0 on every call,
either because we made progress in parsing data and then copied the
rest to the beginning of the buffer or we did not make progress at
all, and then we need to start parsing from the beginning again.

OK?

diff --git slowcgi.c slowcgi.c
index dec4df8d1a1..83d12d99160 100644
--- slowcgi.c
+++ slowcgi.c
@@ -118,7 +118,6 @@ struct request {
struct eventtmo;
int fd;
uint8_t buf[FCGI_RECORD_SIZE];
-   size_t  buf_pos;
size_t  buf_len;
struct fcgi_response_head   response_head;
struct fcgi_stdin_head  stdin_head;
@@ -495,7 +494,6 @@ slowcgi_accept(int fd, short events, void *arg)
return;
}
c->fd = s;
-   c->buf_pos = 0;
c->buf_len = 0;
c->request_started = 0;
c->stdin_fd_closed = c->stdout_fd_closed = c->stderr_fd_closed = 0;
@@ -632,12 +630,12 @@ slowcgi_request(int fd, short events, void *arg)
 {
struct request  *c;
ssize_t  n;
-   size_t   parsed;
+   size_t   parsed, pos = 0;
 
c = arg;
 
-   n = read(fd, c->buf + c->buf_pos + c->buf_len,
-   FCGI_RECORD_SIZE - c->buf_pos-c->buf_len);
+   n = read(fd, c->buf + c->buf_len,
+   FCGI_RECORD_SIZE - c->buf_len);
 
switch (n) {
case -1:
@@ -666,16 +664,15 @@ slowcgi_request(int fd, short events, void *arg)
 * at that point, which is what happens here.
 */
do {
-   parsed = parse_record(c->buf + c->buf_pos, c->buf_len, c);
-   c->buf_pos += parsed;
+   parsed = parse_record(c->buf + pos, c->buf_len, c);
+   pos += parsed;
c->buf_len -= parsed;
} while (parsed > 0 && c->buf_len > 0);
 
/* Make space for further reads */
-   if (c->buf_len > 0) {
-   bcopy(c->buf + c->buf_pos, c->buf, c->buf_len);
-   c->buf_pos = 0;
-   }
+   if (c->buf_len > 0 && pos > 0)
+   bcopy(c->buf + pos, c->buf, c->buf_len);
+
return;
 fail:
cleanup_request(c);

Re: traceroute(8): make progress on timeout

[Todd C. Miller]

On Fri, 13 Jan 2017 16:07:31 +, Florian Obser wrote:

> traceroute(8) never sees a timeout when poll(2) returns when it receives
> a packet not intended for us. E.g. a ping(8) is running in parallel.
> In this case we need to account for the time we already waited.

Looks good but please make waittime private to main() now that it
is not used in worker.c.

 - todd

Re: Move auth_approval in su.c before fork is lost due to pledge?

[Todd C. Miller]

One change with this diff is that the approval script will run as
the invoking user, not the target user.  I'm not sure that really
makes a difference though.

 - todd

traceroute(8): make progress on timeout

[Florian Obser]

traceroute(8) never sees a timeout when poll(2) returns when it receives
a packet not intended for us. E.g. a ping(8) is running in parallel.
In this case we need to account for the time we already waited.

Pointed out by Gabriel Nieto on bugs@, thanks!

OK?

diff --git traceroute.c traceroute.c
index 1444de11f9e..7c6f69c21c5 100644
--- traceroute.c
+++ traceroute.c
@@ -297,6 +297,7 @@ u_char  proto = IPPROTO_UDP;
 
 intverbose;
 intwaittime = 5;   /* time to wait for response (in seconds) */
+intcurwaittime;/* time left to wait for response */
 intnflag;  /* print addresses numerically */
 intdump;
 intAflag;  /* lookup ASN */
@@ -838,13 +839,20 @@ main(int argc, char *argv[])
 
gettime();
send_probe(++seq, ttl, incflag, to);
+   curwaittime = waittime * 1000;
while ((cc = wait_for_reply(rcvsock, ))) {
gettime();
i = packet_ok(to->sa_family, , cc, seq,
incflag);
-   /* Skip short packet */
-   if (i == 0)
+   /* Skip wrong packet */
+   if (i == 0) {
+   curwaittime = waittime * 1000 -
+   ((t2.tv_sec - t1.tv_sec) * 1000 +
+   (t2.tv_usec - t1.tv_usec) / 1000);
+   if (curwaittime < 0)
+   curwaittime = 0;
continue;
+   }
if (to->sa_family == AF_INET) {
ip = (struct ip *)packet;
if (from4.sin_addr.s_addr != lastaddr) {
diff --git traceroute.h traceroute.h
index 8b9e435c119..419168c2b4e 100644
--- traceroute.h
+++ traceroute.h
@@ -137,7 +137,7 @@ extern u_charproto;
 #define ICMP_CODE 0;
 
 extern int verbose;
-extern int waittime;   /* time to wait for response (in seconds) */
+extern int curwaittime;/* time left to wait for response */
 extern int nflag;  /* print addresses numerically */
 extern int dump;
 extern int Aflag;  /* lookup ASN */
diff --git worker.c worker.c
index d27e82abd52..7d0ad0fe496 100644
--- worker.c
+++ worker.c
@@ -227,7 +227,7 @@ wait_for_reply(int sock, struct msghdr *mhdr)
pfd[0].events = POLLIN;
pfd[0].revents = 0;
 
-   if (poll(pfd, 1, waittime * 1000) > 0)
+   if (poll(pfd, 1, curwaittime) > 0)
cc = recvmsg(rcvsock, mhdr, 0);
 
return (cc);

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

Re: Move auth_approval in su.c before fork is lost due to pledge?

[Todd C. Miller]

On 07 Jan 2017 21:14:17 -0700, "Andy Bradford" wrote:

> As  it   turns  out,  it  is   because  I  have  an   approve  entry  in
> /etc/login.conf  and this  requires  the ability  to  fork the  approval
> program. When su tries to run approve  it fails and I find the following
> in dmesg:
> 
> su(77960): syscall 2 "proc"
> 
> ktrace also  shows that pledge shut  it down. So is  the following patch
> correct? I  don't see any downsides,  but perhaps there reasons  for why
> auth_approval happens last?

This looks fine to me.  Since the approval script can do just about
anything it doesn't make sense to try to pledge it.

 - todd

Re: find -delete

[Dmitrij D. Czarkoff]

"Ted Unangst"  wrote:

> This option is not posix (not like that's stopped find accumulating a
> dozen extensions), but it is in gnu and freebsd (for 20 years). it's
> also somewhat popular among sysadmins and blogs, etc. and perhaps most
> importantly, it nicely solves one of the more troublesome caveats of
> find (which the man page actually covers twice because it's so common
> and easy to screw up). So I think it makes a good addition.

I find this option highly distasteful and largerly useless.  You
generally want to use find -exec when you know what you are doing, so
choosing between ; and + is not really an issue unless you are doing it
wrong.  (Of course it might be an issue if you don't use find very
often, but then -delete is a huge hazard because of its nuances.)

I am OK with adding it for compatibility reasons, but I oppose adding
examples of its usage to manual page.

Re: pfkey vs splsoftnet()

[Hrvoje Popovski]

On 12.1.2017. 18:27, Hrvoje Popovski wrote:
> On 12.1.2017. 16:20, Martin Pieuchot wrote:
>> On 10/01/17(Tue) 10:37, Martin Pieuchot wrote:
>>> In pfkey_sendup() we call sorwakeup() which asserts for NET_LOCK(), so
>>> we are already at IPL_SOFTNET.
>>>
>>> pfkeyv2_send() is called via pfkey_output() which is also called with
>>> the NET_LOCK() held.
>>>
>>> Finally replace a comment above pfkeyv2_ipo_walk() by the corresponding
>>> assert.
>> Anybody tested or reviewed this diff?
>>
> 
> 
> I applied this diff in production box with
> 
> carp
> pf
> pfsync
> isakmpd -K4
> sasyncd
> dhcpd
> dhcpd sync
> tcpdump -lnqttti pflog0

and pflow ipfix

and still solid as rock :)

Re: ksh(1): preserve xtrace option

[Otto Moerbeek]

On Fri, Jan 13, 2017 at 09:51:51AM +0100, Otto Moerbeek wrote:

> On Fri, Jan 13, 2017 at 09:46:54AM +0100, Anton Lindqvist wrote:
> 
> > Consider the following script which calculates the sum of the first N
> > integers recursively:
> > 
> > $ cat >sum.sh < > sum() {
> >   [ $1 -eq 0 ] && echo $2 || sum $(($1 - 1)) $(($2 + $1))
> > }
> > 
> > sum 5
> > !
> > 
> > Executing the script with the x option gives the following output:
> > 
> > $ sh -x sum.sh
> > + sum 5
> > 15
> > 
> > I would expect the recursive calls to be traced, similar to how GNU
> > bash/sh behaves. With the patch below applied the output is as expected:
> > 
> > $ sh -x sum.sh
> > + sum 5
> > + [ 5 -eq 0 ]
> > + sum 4 5
> > + [ 4 -eq 0 ]
> > + sum 3 9
> > + [ 3 -eq 0 ]
> > + sum 2 12
> > + [ 2 -eq 0 ]
> > + sum 1 14
> > + [ 1 -eq 0 ]
> > + sum 0 15
> > + [ 0 -eq 0 ]
> > + echo 15
> > 15
> > 
> > The patch make sure to assigns the TRACE flag to every user-defined
> > function if the x option is present. The piece of code that led me to
> > this:
> > 
> > $ sed -n 606,607p /usr/src/bin/ksh/exec.c
> > old_xflag = Flag(FXTRACE);
> > Flag(FXTRACE) = tp->flag & TRACE ? true : false;
> 
> Hmmm,
> 
> afaik -x has always been local to a function in ksh-like shells.
> To enable tracing within function call set -x in the fucntion te be traced.

or set the trace attribute for a specific function:

typeset -ft sum

> 
>   -Otto
> 
> > 
> > Index: exec.c
> > ===
> > RCS file: /cvs/src/bin/ksh/exec.c,v
> > retrieving revision 1.68
> > diff -u -p -r1.68 exec.c
> > --- exec.c  11 Dec 2016 17:49:19 -  1.68
> > +++ exec.c  12 Jan 2017 20:10:48 -
> > @@ -796,6 +796,9 @@ define(const char *name, struct op *t)
> > if (t->u.ksh_func)
> > tp->flag |= FKSH;
> >  
> > +   if (Flag(FXTRACE))
> > +   tp->flag |= TRACE;
> > +
> > return 0;
> >  }

Re: ksh(1): preserve xtrace option

[Otto Moerbeek]

On Fri, Jan 13, 2017 at 09:46:54AM +0100, Anton Lindqvist wrote:

> Consider the following script which calculates the sum of the first N
> integers recursively:
> 
> $ cat >sum.sh < sum() {
>   [ $1 -eq 0 ] && echo $2 || sum $(($1 - 1)) $(($2 + $1))
> }
> 
> sum 5
> !
> 
> Executing the script with the x option gives the following output:
> 
> $ sh -x sum.sh
> + sum 5
> 15
> 
> I would expect the recursive calls to be traced, similar to how GNU
> bash/sh behaves. With the patch below applied the output is as expected:
> 
> $ sh -x sum.sh
> + sum 5
> + [ 5 -eq 0 ]
> + sum 4 5
> + [ 4 -eq 0 ]
> + sum 3 9
> + [ 3 -eq 0 ]
> + sum 2 12
> + [ 2 -eq 0 ]
> + sum 1 14
> + [ 1 -eq 0 ]
> + sum 0 15
> + [ 0 -eq 0 ]
> + echo 15
> 15
> 
> The patch make sure to assigns the TRACE flag to every user-defined
> function if the x option is present. The piece of code that led me to
> this:
> 
> $ sed -n 606,607p /usr/src/bin/ksh/exec.c
> old_xflag = Flag(FXTRACE);
> Flag(FXTRACE) = tp->flag & TRACE ? true : false;

Hmmm,

afaik -x has always been local to a function in ksh-like shells.
To enable tracing within function call set -x in the fucntion te be traced.

-Otto

> 
> Index: exec.c
> ===
> RCS file: /cvs/src/bin/ksh/exec.c,v
> retrieving revision 1.68
> diff -u -p -r1.68 exec.c
> --- exec.c11 Dec 2016 17:49:19 -  1.68
> +++ exec.c12 Jan 2017 20:10:48 -
> @@ -796,6 +796,9 @@ define(const char *name, struct op *t)
>   if (t->u.ksh_func)
>   tp->flag |= FKSH;
>  
> + if (Flag(FXTRACE))
> + tp->flag |= TRACE;
> +
>   return 0;
>  }

ksh(1): preserve xtrace option

[Anton Lindqvist]

Consider the following script which calculates the sum of the first N
integers recursively:

$ cat >sum.sh u.ksh_func)
tp->flag |= FKSH;
 
+   if (Flag(FXTRACE))
+   tp->flag |= TRACE;
+
return 0;
 }