Re: Minor improvement to socket(2) man page

2016-12-21 Thread Michael McConville
Philip Guenther wrote:
> On Wed, Sep 21, 2016 at 6:23 PM, Michael McConville <mm...@mykolab.com> wrote:
> > The current version is somewhat awkward and forgets to mention that
> > errno is set. I adapted the paragraph found in most other system call
> > man pages.
> 
> There are six syscalls that return an fd on success...and their
> manpages all have totally different wording for the return value.
> Whee.
> 
> accept
>  The call returns -1 on error.  If it succeeds, it returns a non-negative
>  integer that is a descriptor for the accepted socket.
> 
> fhopen
>  Upon successful completion, fhopen() returns the file descriptor for the
>  opened file, while fhstat() and fhstatfs() return 0.  Otherwise, -1 is
>  returned and errno is set to indicate the error.
> 
> kqueue
>  kqueue() creates a new kernel event queue and returns a file descriptor.
>  If there was an error creating the kernel event queue, a value of -1 is
>  returned and errno set.
> 
> open
>  If successful, open() returns a non-negative integer, termed a file
>  descriptor.  Otherwise, a value of -1 is returned and errno is set to
>  indicate the error.
> 
> openat: same manpage as open but totally unmentioned in the RETURN
> VALUES section
> 
> socket
>  A -1 is returned if an error occurs, otherwise the return value is a
>  descriptor referencing the socket.
> 
>   Some consistency that doesn't sacrifice clarity would be nice...

Before I do the busy-work: do the man page gurus have a preferred
phrasing? I prefer fhopen(2)'s (listed above), which seems to be the
most common.



Minor improvement to socket(2) man page

2016-09-21 Thread Michael McConville
The current version is somewhat awkward and forgets to mention that
errno is set. I adapted the paragraph found in most other system call
man pages.

Thanks for your time,
Mike


Index: lib/libc/sys/socket.2
===
RCS file: /cvs/src/lib/libc/sys/socket.2,v
retrieving revision 1.41
diff -u -p -u -r1.41 socket.2
--- lib/libc/sys/socket.2   19 Mar 2016 22:10:49 -  1.41
+++ lib/libc/sys/socket.2   22 Sep 2016 01:20:30 -
@@ -219,8 +219,11 @@ and
 .Xr getsockopt 2
 are used to set and get options, respectively.
 .Sh RETURN VALUES
-A \-1 is returned if an error occurs, otherwise the return
-value is a descriptor referencing the socket.
+Upon successful completion, a descriptor referencing the socket is
+returned. Otherwise, the value \-1 is returned and the global
+variable
+.Va errno
+is set to indicate the error.
 .Sh ERRORS
 The
 .Fn socket



PF error message tweak

2016-06-02 Thread Michael McConville
The below error message sequence isn't as clear as it could be:

> mike:/home/mike$ doas pfctl -f /etc/pf. 
> pfctl: /etc/pf.: No such file or directory
> pfctl: cannot open the main config file!: No such file or directory
> pfctl: Syntax error in config file: pf rules not loaded

There are possible improvements other than the one below, but because
it's an ergonomic issue I figured I'd point it out and leave it to the
PF people.

Thanks,
Mike


Index: pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.334
diff -u -p -u -r1.334 pfctl.c
--- pfctl.c 14 Jan 2016 12:05:51 -  1.334
+++ pfctl.c 2 Jun 2016 13:17:32 -
@@ -1507,7 +1507,7 @@ pfctl_rules(int dev, char *filename, int
 
if (parse_config(filename, ) < 0) {
if ((opts & PF_OPT_NOACTION) == 0)
-   ERRX("Syntax error in config file: "
+   ERRX("Error parsing config file: "
"pf rules not loaded");
else
goto _error;



Re: malloc: add full delayed chunk double-free detection

2016-05-10 Thread Michael McConville
Daniel Micay wrote:
> This uses a hash table to maintain a set of delayed allocations,
> allowing full and efficient double-free detection. The current code
> can only catch it when the two pointers being swapped are equal, so
> double-frees that could be caught are missed. A naive loop over every
> delayed chunk would work fine with the current 16 slot array, but that
> would make scaling up the number of delayed allocations much more
> costly.
> 
> I'm using this with 2 other changes making the size configurable and
> adding a FIFO ring buffer in front of the randomized array of the same
> size, to get a minimum guaranteed delay more than the current 1 free
> cycle. The randomization still works fine and it ends up providing a
> nice balance. It's essentially equivalent to the quarantine feature in
> Valgrind/ASan, but suitable for production and with weaker
> use-after-free detection. It mixes well with the earlier change to
> detect write-after-free via junk validation here.

Seems valuable. Note, though, that you're developing on top of the
multi-pool malloc patches:

https://marc.info/?t=14587167622=1=2

http://www.drijf.net/openbsd/malloc/

So there are probably conflicts.

Thanks,
Mike


> diff --git a/stdlib/malloc.c b/stdlib/malloc.c
> index bc328d2..9e8bd16 100644
> --- a/stdlib/malloc.c
> +++ b/stdlib/malloc.c
> @@ -118,6 +118,7 @@ struct dir_info {
>   struct region_info free_regions[MALLOC_MAXCACHE];
>   /* delayed free chunk slots */
>   void *delayed_chunks[MALLOC_DELAYED_CHUNK_MASK + 1];
> + void *delayed_chunks_set[(MALLOC_DELAYED_CHUNK_MASK + 1) * 2];
>   size_t rbytesused;  /* random bytes used */
>   char *func; /* current function */
>   u_char rbytes[32];  /* random bytes */
> @@ -249,6 +250,22 @@ hash(void *p)
>   return sum;
>  }
>  
> +static inline size_t
> +hash_chunk(void *p)
> +{
> + size_t sum;
> + uintptr_t u;
> +
> + u = (uintptr_t)p >> MALLOC_MINSHIFT;
> + sum = u;
> + sum = (sum << 7) - sum + (u >> 16);
> +#ifdef __LP64__
> + sum = (sum << 7) - sum + (u >> 32);
> + sum = (sum << 7) - sum + (u >> 48);
> +#endif
> + return sum;
> +}
> +
>  static void
>  wrterror(struct dir_info *d, char *msg, void *p)
>  {
> @@ -864,6 +881,58 @@ delete(struct dir_info *d, struct region_info *ri)
>   }
>  }
>  
> +void delayed_chunks_insert(struct dir_info *d, void *p) {
> + size_t index;
> + size_t mask = sizeof(d->delayed_chunks_set) / sizeof(void *) - 1;
> + void *q;
> +
> + index = hash_chunk(p) & mask;
> + q = d->delayed_chunks_set[index];
> + while (q != NULL) {
> + if (p == q) {
> + wrterror(d, "double free", p);
> + return;
> + }
> + index = (index - 1) & mask;
> + q = d->delayed_chunks_set[index];
> + }
> + d->delayed_chunks_set[index] = p;
> +}
> +
> +void delayed_chunks_delete(struct dir_info *d, void *p) {
> + size_t mask = sizeof(d->delayed_chunks_set) / sizeof(void *) - 1;
> + size_t i, j, r;
> + void *q;
> +
> + i = hash_chunk(p) & mask;
> + q = d->delayed_chunks_set[i];
> + while (q != p) {
> + if (q == NULL) {
> + wrterror(d, "pointer missing from address tracking 
> table", p);
> + return;
> + }
> + i = (i - 1) & mask;
> + q = d->delayed_chunks_set[i];
> + }
> +
> + for (;;) {
> + d->delayed_chunks_set[i] = NULL;
> + j = i;
> + for (;;) {
> + i = (i - 1) & mask;
> + if (d->delayed_chunks_set[i] == NULL)
> + return;
> + r = hash_chunk(d->delayed_chunks_set[i]) & mask;
> + if ((i <= r && r < j) || (r < j && j < i) ||
> + (j < i && i <= r))
> + continue;
> + d->delayed_chunks_set[j] = d->delayed_chunks_set[i];
> + break;
> + }
> + }
> +}
> +
> +
>  /*
>   * Allocate a page of chunks
>   */
> @@ -1315,13 +1384,21 @@ ofree(struct dir_info *pool, void *p)
>   if (!mopts.malloc_freenow) {
>   if (find_chunknum(pool, r, p) == -1)
>   return;
> +
> + if (p == NULL)
> + return;
> +
> + delayed_chunks_insert(pool, p);
> +
>   i = getrbyte(pool) & MALLOC_DELAYED_CHUNK_MASK;
>   tmp = p;
>   p = pool->delayed_chunks[i];
> - if (tmp == p) {
> - wrterror(pool, "double free", p);
> +
> + if (p == NULL)
>   return;
> - }
> +
> + 

Re: midiplay: Fix out-of-bounds memory access

2016-05-01 Thread Michael McConville
Vadim Zhukov wrote:
> 2016-04-30 7:38 GMT+03:00 Jonathan Gray :
> > On Wed, Apr 27, 2016 at 07:49:50PM -0700, Geoff Hill wrote:
> >> Fix possible reads past the end of the buffer.
> >>
> >> Found by random fuzz testing (zzuf). Without the fix the fuzzer crashes
> >> in several seconds; with the patch, the fuzzer runs clean for hours.
> >
> > Any reason to not replace the somewhat arbitary earlier test
> > for this?
> >
> > Index: midiplay.c
> > ===
> > RCS file: /cvs/src/usr.bin/midiplay/midiplay.c,v
> > retrieving revision 1.17
> > diff -u -p -U9 -r1.17 midiplay.c
> > --- midiplay.c  8 Feb 2015 23:40:34 -   1.17
> > +++ midiplay.c  30 Apr 2016 04:35:31 -
> > @@ -306,19 +306,19 @@ playdata(u_char *buf, u_int tot, char *n
> > tracks = calloc(ntrks, sizeof(struct track));
> > if (tracks == NULL)
> > err(1, "malloc() tracks failed");
> > for (t = 0; t < ntrks; ) {
> > if (p >= end - MARK_LEN - SIZE_LEN) {
> > warnx("Cannot find track %d", t);
> > goto ret;
> > }
> > len = GET32(p + MARK_LEN);
> > -   if (len > 100) { /* a safe guard */
> > +   if (p + MARK_LEN + SIZE_LEN + len > end) {
> 
> It's better to avoid "+" in checks to avoid overflow, no?

Yeah, I'm pretty sure the above overflow check is undefined.



Re: bufcache KNF

2016-04-11 Thread Michael McConville
Ted Unangst wrote:
> Martin Pieuchot wrote:
> > ok?
> > 
> > -int chillbufs(struct
> > -bufcache *cache, struct bufqueue *queue, int64_t *queuepages);
> > +int chillbufs(struct bufcache *, struct bufqueue *, int64_t *);
> 
> fwiw i like names in prototypes, so i know what's going on. i know
> style says that, but i think the advice is obsolete.

The compiler doesn't check that the argument names in the prototype
match those in the definition. The below program compiles without
warning.


int f(int a);

int
f(int b)
{
return b+3;
}

int
main()
{
return f(2);
}



Re: Pledge error handling

2016-04-03 Thread Michael McConville
H├ęctor Luis Gimbatti wrote:
> I noticed there are (at least 2) diferent ways to handle a pledge
> error (eg: /usr/src/usr.bin/):
> 
> If (pledge(args, NULL) == -1)
> . err(1, "pledge"); (wc; w; ..)
> . perror("pledge"); exit(EXIT_CODE); (vi; openssl; ...)
> 
> I am not familiar with the case of use of each function but perror +
> exit isnt the same as err ? Can we use just one function (either error
> or perror) to handle pledge
> 
> I've also seen the use of fatal(...) I guess this might be another
> mechanism which is useful.

Some of these are meant to conform to the style conventions of codebases
that are otherwise only imported (not developed by OpenBSD). For cases
like Open^WLibreSSL and Libre^WOpenSSH, though, patches are welcome.
There was a thread about this a few months ago. IIRC, doug@ and djm@
pointed out that all such codebases' compat files define common err.h
functions, and that those functions are nicer looking.



Re: spamd - DNS whitelist

2016-04-03 Thread Michael McConville
Bob Beck wrote:
> No.  DNS based whitelisting does not belong in there. because it is
> slow and DOS'able
> 
> spamd is designed to be high speed low drag. If you want to do a DNS
> based whitelist, write a little co-thing that spits one into a file or
> into your nospamd table that then spamd *does not even see*.
> 
> In short *spamd* is the wrong place to do this.  put your dns based
> whitelist in a table periodically

This sounds like a potentially problematic approach. There are now spam
networks that circumvent DNS blacklists, even if the SMTP server queries
for each domain it receives. The best I can tell, they do this by
burning through domains on cheap TLDs like .xyz. Locally caching DNS
blacklist responses seems like it could magnify this problem
substantially.



Re: Possible kernel function with stack frame > 2048 bytes

2016-03-27 Thread Michael McConville
Michael McConville wrote:
> Clang 3.7 gives this warning when building the kernel:
> 
> > ../../../../dev/audio.c:1852:1: warning: stack frame size of 2504 bytes in 
> > function 'wskbd_initvol' [-Wframe-larger-than=]
> > wskbd_initvol(struct audio_softc *sc, struct wskbd_vol *vol, char *cn, char 
> > *dn)
> > ^
> 
> -Wframe-larger-than was backported to base GCC, so maybe there's a bug
> that caused it to erroneously overlook this one. It's also possible that
> Clang just compiles this function differently, but >450 bytes seems like
> a really big difference.
> 
> Maybe it isn't worth fixing, but I thought it was worth pointing out. It
> seems like it may have been introduced by the simplifications last June:
> 
> https://marc.info/?t=14340075202=1=2

I broke out objdump (should have done that initially) and, if I'm
reading the asm right, the stack frame is only 1,688 bytes with base
GCC. Maybe there's more aggressive stack space reuse.



Possible kernel function with stack frame > 2048 bytes

2016-03-27 Thread Michael McConville
Clang 3.7 gives this warning when building the kernel:

> ../../../../dev/audio.c:1852:1: warning: stack frame size of 2504 bytes in 
> function 'wskbd_initvol' [-Wframe-larger-than=]
> wskbd_initvol(struct audio_softc *sc, struct wskbd_vol *vol, char *cn, char 
> *dn)
> ^

-Wframe-larger-than was backported to base GCC, so maybe there's a bug
that caused it to erroneously overlook this one. It's also possible that
Clang just compiles this function differently, but >450 bytes seems like
a really big difference.

Maybe it isn't worth fixing, but I thought it was worth pointing out. It
seems like it may have been introduced by the simplifications last June:

https://marc.info/?t=14340075202=1=2



[mm...@mykolab.com: Merge memleak fix from BoringSSL]

2016-03-26 Thread Michael McConville
Does this look good?


- Forwarded message from Michael McConville <mm...@mykolab.com> -

Date: Sun, 20 Mar 2016 23:15:48 -0400
From: Michael McConville <mm...@mykolab.com>
To: libre...@openbsd.org
Subject: Merge memleak fix from BoringSSL

Looks like it applies to us:

https://boringssl.googlesource.com/boringssl/+/6b6e0b20893e2be0e68af605a60ffa2cbb0ffa64

Anecdotally, I need to check whether sk_X509_NAME_pop_free() are also
NULL-safe for us, or if BoringSSL made that change. One way or the
other, they removed their NULL checks. It's a little hard to confidently
discern because they're at least triple-nested macros, but I'll have the
time to spelunk eventually.


Index: src/ssl/s3_clnt.c
===
RCS file: /cvs/src/lib/libssl/src/ssl/s3_clnt.c,v
retrieving revision 1.137
diff -u -p -r1.137 s3_clnt.c
--- src/ssl/s3_clnt.c   11 Mar 2016 07:08:45 -  1.137
+++ src/ssl/s3_clnt.c   21 Mar 2016 03:08:40 -
@@ -1641,6 +1641,7 @@ ssl3_get_certificate_request(SSL *s)
ERR_R_MALLOC_FAILURE);
goto err;
}
+   xn = NULL;  /* avoid free in err block */
}
 
/* we should setup a certificate to return */
@@ -1658,6 +1659,7 @@ truncated:
SSL_R_BAD_PACKET_LENGTH);
}
 err:
+   X509_NAME_free(xn);
if (ca_sk != NULL)
sk_X509_NAME_pop_free(ca_sk, X509_NAME_free);
return (ret);


- End forwarded message -



Re: Fix overflow check in sys/kern/kern_time.c

2016-03-23 Thread Michael McConville
Mark Kettenis wrote:
> > I'm not sure whether avoiding incrementing here is an ideal move, but
> > this diff definitely works toward a local optimum. Namely, that error
> > check is technically meaningless because signed overflow is undefined.
> 
> Within the kernel, signed overflow actually is well-defined.  We don't
> run on hypethetical one's complement machines (or machines that use
> some other weird scheme to encode signed integers) and we pass flags
> to the compiler that make it do the right thing.
> 
> Yes, the fact that the C standard doesn't want to make assumptions
> about the encoding of signed inttegers is the real reason why the
> standard leaves integer overflow undefined.  Unfortunately that has
> lead the language lwayer types in charge of compilers to implement
> misguided optimizations that break stuff.

I agree that it's unfortunate that signed overflow is undefined. I think
ignoring that fact is a long-term problem, though. Presumably, we're
eventually going to switch compilers, and that could silently break
things. In particular, the flags used for saner signed overflow behavior
are often poorly maintained or broken. For example, -ftrapv is
completely broken (no effect) in both the base and port GCC. If we
decide to depend on signed overflow, we restrict ourselves to compilers
implementing our non-standard dialect of C, and we rely on these
somewhat uncommonly used flags.

> > ok? Or would people prefer a solution that's robust to changing
> > *curpps's type?
> 
> I you absolutely want to "fix" this issue, I'd prefer that you just
> fixed the check and left the comment in place.  Then at least the code
> continues to document the corner case.  So simply:
> 
> > -   if (*curpps + 1 > *curpps)
> > +   if (*curpps != INT_MAX)
> 
> Of course that breaks if you start out with a negative number...

Can you explain? I don't understand.

> So I think I'd prefer if you simply left this alone.



Fix overflow check in sys/kern/kern_time.c

2016-03-23 Thread Michael McConville
I'm not sure whether avoiding incrementing here is an ideal move, but
this diff definitely works toward a local optimum. Namely, that error
check is technically meaningless because signed overflow is undefined.

ok? Or would people prefer a solution that's robust to changing
*curpps's type?


Index: sys/kern/kern_time.c
===
RCS file: /cvs/src/sys/kern/kern_time.c,v
retrieving revision 1.96
diff -u -p -r1.96 kern_time.c
--- sys/kern/kern_time.c5 Dec 2015 10:11:53 -   1.96
+++ sys/kern/kern_time.c10 Feb 2016 05:25:35 -
@@ -765,20 +765,8 @@ ppsratecheck(struct timeval *lasttime, i
else
rv = 0;
 
-#if 1 /*DIAGNOSTIC?*/
-   /* be careful about wrap-around */
-   if (*curpps + 1 > *curpps)
-   *curpps = *curpps + 1;
-#else
-   /*
-* assume that there's not too many calls to this function.
-* not sure if the assumption holds, as it depends on *caller's*
-* behavior, not the behavior of this function.
-* IMHO it is wrong to make assumption on the caller's behavior,
-* so the above #if is #if 1, not #ifdef DIAGNOSTIC.
-*/
-   *curpps = *curpps + 1;
-#endif
+   if (*curpps != INT_MAX)
+   (*curpps)++;
 
return (rv);
 }



Re: Scheduler hack for multi-threaded processes

2016-03-22 Thread Michael McConville
Douglas Ray wrote:
> On 21/03/16 11:29 AM, Mark Kettenis wrote:
> >>From: Amit Kulkarni 
> >>Date: Sun, 20 Mar 2016 17:57:49 -0500
> ...
> >>+1. Previously, when I did a cvs update with original scheduler code, doing
> >>the ports update the machine always froze solid while doing cvs update,
> >>taking 3 minutes to recover. This time with Martin's patch, the freezing
> >>period seems to have decreased quite a bit, although the freeze still
> >>happens. Stefan's amap diff and Bob's VFS/UVM diff also seems to have a
> >>made a difference.
> >>
> >>Pentium G2020 2.9GHz dual core Ivy bridge 22nm... 8 GB RAM
> >>
> >>IMHO, this patch should go in!
> >
> >No.  It's a hack. It points out aproblem that should be investigated
> >deeper.
> >
> 
> If it gives a significant performance improvement but is too distant
> from a real solution, maybe it could be worth distributing in
> package(s) form.
> 
> The team then has the option to actively promote it, or not; and
> the package could be updated as new refinements are found.

It's kernel code, you can't package it.



Re: kern_sched.c: unused functions

2016-03-19 Thread Michael McConville
Michal Mazurek wrote:
> On 17:19:39,  2.03.16, Martin Natano wrote:
> > On Wed, Mar 02, 2016 at 05:07:21PM +0100, Michal Mazurek wrote:
> > > kern_sched.c:
> > > - remove unused functions
> > > - mark static functions as static
> > 
> > Functions shouldn't be static in the kernel. "In userland,
> > functions local to one source module should be declared ???static???.  This
> > should not be done in kernel land since it makes it impossible to use the
> > kernel debugger." -- style(8)
> 
> Oh, right.
> 
> Remove cpuset_clear(), cpuset_add_all() and cpuset_union(), and move
> some declarations from proc.h to kern_sched.c.

ok mmcc@, pending an ok from someone who works with this code.

> Index: sys/kern/kern_sched.c
> ===
> RCS file: /cvs/src/sys/kern/kern_sched.c,v
> retrieving revision 1.41
> diff -u -p -r1.41 kern_sched.c
> --- sys/kern/kern_sched.c 23 Dec 2015 14:51:17 -  1.41
> +++ sys/kern/kern_sched.c 2 Mar 2016 16:37:18 -
> @@ -28,11 +28,18 @@
>  
>  #include 
>  
> -void sched_kthreads_create(void *);
> -
> -int sched_proc_to_cpu_cost(struct cpu_info *ci, struct proc *p);
> +void  sched_kthreads_create(void *);
> +int   sched_proc_to_cpu_cost(struct cpu_info *ci, struct proc *p);
>  struct proc *sched_steal_proc(struct cpu_info *);
>  
> +void  cpuset_complement(struct cpuset *, struct cpuset *, struct cpuset *);
> +void  cpuset_copy(struct cpuset *, struct cpuset *);
> +struct cpu_info *cpuset_first(struct cpuset *);
> +void  cpuset_del(struct cpuset *, struct cpu_info *);
> +void  cpuset_init_cpu(struct cpu_info *);
> +void  cpuset_intersection(struct cpuset *t, struct cpuset *,
> + struct cpuset *);
> +
>  /*
>   * To help choosing which cpu should run which process we keep track
>   * of cpus which are currently idle and which cpus have processes
> @@ -717,12 +724,6 @@ cpuset_init_cpu(struct cpu_info *ci)
>  }
>  
>  void
> -cpuset_clear(struct cpuset *cs)
> -{
> - memset(cs, 0, sizeof(*cs));
> -}
> -
> -void
>  cpuset_add(struct cpuset *cs, struct cpu_info *ci)
>  {
>   unsigned int num = CPU_INFO_UNIT(ci);
> @@ -744,12 +745,6 @@ cpuset_isset(struct cpuset *cs, struct c
>  }
>  
>  void
> -cpuset_add_all(struct cpuset *cs)
> -{
> - cpuset_copy(cs, _all);
> -}
> -
> -void
>  cpuset_copy(struct cpuset *to, struct cpuset *from)
>  {
>   memcpy(to, from, sizeof(*to));
> @@ -765,15 +760,6 @@ cpuset_first(struct cpuset *cs)
>   return (cpuset_infos[i * 32 + ffs(cs->cs_set[i]) - 1]);
>  
>   return (NULL);
> -}
> -
> -void
> -cpuset_union(struct cpuset *to, struct cpuset *a, struct cpuset *b)
> -{
> - int i;
> -
> - for (i = 0; i < CPUSET_ASIZE(ncpus); i++)
> - to->cs_set[i] = a->cs_set[i] | b->cs_set[i];
>  }
>  
>  void
> Index: sys/sys/proc.h
> ===
> RCS file: /cvs/src/sys/sys/proc.h,v
> retrieving revision 1.213
> diff -u -p -r1.213 proc.h
> --- sys/sys/proc.h6 Dec 2015 17:50:21 -   1.213
> +++ sys/sys/proc.h2 Mar 2016 15:56:33 -
> @@ -205,9 +205,9 @@ struct process {
>  
>   struct uprof {  /* profile arguments */
>   caddr_t pr_base;/* buffer base */
> - size_t  pr_size;/* buffer size */
> + size_t  pr_size;/* buffer size */
>   u_long  pr_off; /* pc offset */
> - u_int   pr_scale;   /* pc scaling */
> + u_int   pr_scale;   /* pc scaling */
>   } ps_prof;
>  
>   u_short ps_acflag;  /* Accounting flags. */
> @@ -215,8 +215,8 @@ struct process {
>   uint64_t ps_pledge;
>   struct whitepaths *ps_pledgepaths;
>  
> - int64_t ps_kbind_cookie;
> - u_long  ps_kbind_addr;
> + int64_t ps_kbind_cookie;
> + u_long  ps_kbind_addr;
>  
>  /* End area that is copied on creation. */
>  #define ps_endcopy   ps_refcnt
> @@ -255,7 +255,7 @@ struct process {
>  #define  PS_EMBRYO   0x0002  /* New process, not yet fledged 
> */
>  #define  PS_ZOMBIE   0x0004  /* Dead and ready to be waited 
> for */
>  #define  PS_NOBROADCASTKILL 0x0008   /* Process excluded from kill 
> -1. */
> -#define PS_PLEDGE0x0010  /* Has called pledge(2) */
> +#define  PS_PLEDGE   0x0010  /* Has called pledge(2) */
>  
>  #define  PS_BITS \
>  ("\20" "\01CONTROLT" "\02EXEC" "\03INEXEC" "\04EXITING" "\05SUGID" \
> @@ -380,12 +380,12 @@ struct proc {
>  #define  P_WEXIT 0x2000  /* Working on exiting. */
>  #define  P_OWEUPC0x8000  /* Owe proc an addupc() at next 
> ast. */
>  #define  P_SUSPSINGLE0x0008  /* Need to stop for single 
> threading. */
> -#define P_SYSTRACE   0x0040  /* Process system call tracing active*/
> -#define P_CONTINUED  0x0080  /* Proc has continued from a stopped 
> 

Re: New scheduler for OpenBSD

2016-03-19 Thread Michael McConville
Bob Beck wrote:
> this is cool .. but
> 
> I would be interested in someone comparing server workloads, as
> opposed to interactive GUI response, using this.
> 
> I wouldn't be surprised that inspiriation from BFS would produce
> better interactive response, my bigger concern would be does this
> adversely impact how we deal with non-interactive workloads.

Those interested might find the benchmarks/siege port useful.



Re: stty header cleanup

2016-03-19 Thread Michael McConville
One correction:

Edgar Pettijohn wrote:
> Index: cchar.c
> ===
> RCS file: /cvs/src/bin/stty/cchar.c,v
> retrieving revision 1.11
> diff -u -p -u -r1.11 cchar.c
> --- cchar.c27 Oct 2009 23:59:22 -1.11
> +++ cchar.c16 Mar 2016 23:53:58 -
> @@ -30,6 +30,7 @@
>   * SUCH DAMAGE.
>   */
> 
> +#include 
>  #include 
> 
>  #include 

deraadt pointed out that sys/types.h has to come first here.

> Index: key.c
> ===
> RCS file: /cvs/src/bin/stty/key.c,v
> retrieving revision 1.16
> diff -u -p -u -r1.16 key.c
> --- key.c20 Nov 2015 15:57:39 -1.16
> +++ key.c16 Mar 2016 23:55:51 -
> @@ -30,14 +30,16 @@
>   * SUCH DAMAGE.
>   */
> 
> +#include 
>  #include 

And here.

> Index: stty.c
> ===
> RCS file: /cvs/src/bin/stty/stty.c,v
> retrieving revision 1.18
> diff -u -p -u -r1.18 stty.c
> --- stty.c20 Nov 2015 15:58:28 -1.18
> +++ stty.c16 Mar 2016 23:58:14 -
> @@ -30,16 +30,18 @@
>   * SUCH DAMAGE.
>   */
> 
> +#include 
>  #include 

And here.



Re: stty header cleanup

2016-03-19 Thread Michael McConville
ok mmcc@

Edgar Pettijohn wrote:
> Index: cchar.c
> ===
> RCS file: /cvs/src/bin/stty/cchar.c,v
> retrieving revision 1.11
> diff -u -p -u -r1.11 cchar.c
> --- cchar.c27 Oct 2009 23:59:22 -1.11
> +++ cchar.c16 Mar 2016 23:53:58 -
> @@ -30,6 +30,7 @@
>   * SUCH DAMAGE.
>   */
> 
> +#include 
>  #include 
> 
>  #include 
> @@ -37,6 +38,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
> 
>  #include "stty.h"
>  #include "extern.h"
> Index: gfmt.c
> ===
> RCS file: /cvs/src/bin/stty/gfmt.c,v
> retrieving revision 1.8
> diff -u -p -u -r1.8 gfmt.c
> --- gfmt.c28 Oct 2009 07:12:59 -1.8
> +++ gfmt.c16 Mar 2016 23:54:29 -
> @@ -35,6 +35,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
> 
>  #include "stty.h"
>  #include "extern.h"
> Index: key.c
> ===
> RCS file: /cvs/src/bin/stty/key.c,v
> retrieving revision 1.16
> diff -u -p -u -r1.16 key.c
> --- key.c20 Nov 2015 15:57:39 -1.16
> +++ key.c16 Mar 2016 23:55:51 -
> @@ -30,14 +30,16 @@
>   * SUCH DAMAGE.
>   */
> 
> +#include 
>  #include 
> 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
> +#include 
>  #include 
> +#include 
> 
>  #include "stty.h"
>  #include "extern.h"
> Index: modes.c
> ===
> RCS file: /cvs/src/bin/stty/modes.c,v
> retrieving revision 1.10
> diff -u -p -u -r1.10 modes.c
> --- modes.c27 Oct 2009 23:59:22 -1.10
> +++ modes.c16 Mar 2016 23:56:40 -
> @@ -31,8 +31,11 @@
>   */
> 
>  #include 
> +
>  #include 
>  #include 
> +#include 
> +
>  #include "stty.h"
>  #include "extern.h"
> 
> Index: print.c
> ===
> RCS file: /cvs/src/bin/stty/print.c,v
> retrieving revision 1.13
> diff -u -p -u -r1.13 print.c
> --- print.c27 Oct 2009 23:59:22 -1.13
> +++ print.c16 Mar 2016 23:57:24 -
> @@ -35,6 +35,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
> 
>  #include "stty.h"
>  #include "extern.h"
> Index: stty.c
> ===
> RCS file: /cvs/src/bin/stty/stty.c,v
> retrieving revision 1.18
> diff -u -p -u -r1.18 stty.c
> --- stty.c20 Nov 2015 15:58:28 -1.18
> +++ stty.c16 Mar 2016 23:58:14 -
> @@ -30,16 +30,18 @@
>   * SUCH DAMAGE.
>   */
> 
> +#include 
>  #include 
> 
>  #include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
> -#include 
>  #include 
> +#include 
>  #include 
> 
>  #include "stty.h"
> Index: stty.h
> ===
> RCS file: /cvs/src/bin/stty/stty.h,v
> retrieving revision 1.4
> diff -u -p -u -r1.4 stty.h
> --- stty.h2 Jun 2003 23:32:09 -1.4
> +++ stty.h16 Mar 2016 23:52:41 -
> @@ -32,9 +32,6 @@
>   *@(#)stty.h8.1 (Berkeley) 5/31/93
>   */
> 
> -#include 
> -#include 
> -
>  struct info {
>  int fd;/* file descriptor */
>  int ldisc;/* line discipline */
> 



Re: ed header cleanup

2016-03-19 Thread Michael McConville
ok mmcc@

Edgar Pettijohn wrote:
> Index: buf.c
> ===
> RCS file: /cvs/src/bin/ed/buf.c,v
> retrieving revision 1.22
> diff -u -p -u -r1.22 buf.c
> --- buf.c9 Oct 2015 19:47:02 -1.22
> +++ buf.c16 Mar 2016 23:23:53 -
> @@ -30,6 +30,15 @@
>   */
> 
>  #include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> 
>  #include "ed.h"
> 
> Index: ed.h
> ===
> RCS file: /cvs/src/bin/ed/ed.h,v
> retrieving revision 1.21
> diff -u -p -u -r1.21 ed.h
> --- ed.h9 Oct 2015 21:24:05 -1.21
> +++ ed.h16 Mar 2016 23:11:41 -
> @@ -30,16 +30,6 @@
>   *@(#)ed.h,v 1.5 1994/02/01 00:34:39 alm Exp
>   */
> 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -
>  #define ERR(-2)
>  #define EMOD(-3)
>  #define FATAL(-4)
> Index: glbl.c
> ===
> RCS file: /cvs/src/bin/ed/glbl.c,v
> retrieving revision 1.17
> diff -u -p -u -r1.17 glbl.c
> --- glbl.c9 Oct 2015 20:27:28 -1.17
> +++ glbl.c16 Mar 2016 23:26:10 -
> @@ -32,6 +32,12 @@
>  #include 
>  #include 
> 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
>  #include "ed.h"
> 
>  static int set_active_node(line_t *);
> Index: io.c
> ===
> RCS file: /cvs/src/bin/ed/io.c,v
> retrieving revision 1.18
> diff -u -p -u -r1.18 io.c
> --- io.c9 Oct 2015 20:27:28 -1.18
> +++ io.c16 Mar 2016 23:28:09 -
> @@ -28,6 +28,12 @@
>   * SUCH DAMAGE.
>   */
> 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
>  #include "ed.h"
> 
>  static int read_stream(FILE *, int);
> Index: main.c
> ===
> RCS file: /cvs/src/bin/ed/main.c,v
> retrieving revision 1.56
> diff -u -p -u -r1.56 main.c
> --- main.c20 Nov 2015 08:53:28 -1.56
> +++ main.c16 Mar 2016 23:31:16 -
> @@ -44,11 +44,19 @@
>  #include 
>  #include 
>  #include 
> +
>  #include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
>  #include 
> +#include 
> +#include 
> +#include 
> +#include 
>  #include 
> -#include 
> -#include 
> 
>  #include "ed.h"
> 
> Index: re.c
> ===
> RCS file: /cvs/src/bin/ed/re.c,v
> retrieving revision 1.16
> diff -u -p -u -r1.16 re.c
> --- re.c9 Oct 2015 21:24:05 -1.16
> +++ re.c16 Mar 2016 23:34:46 -
> @@ -29,6 +29,12 @@
>   * SUCH DAMAGE.
>   */
> 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
>  #include "ed.h"
> 
>  static char *extract_pattern(int);
> Index: sub.c
> ===
> RCS file: /cvs/src/bin/ed/sub.c,v
> retrieving revision 1.14
> diff -u -p -u -r1.14 sub.c
> --- sub.c9 Oct 2015 20:27:28 -1.14
> +++ sub.c16 Mar 2016 23:36:29 -
> @@ -29,6 +29,13 @@
>   * SUCH DAMAGE.
>   */
> 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
>  #include "ed.h"
> 
>  static char *extract_subst_template(void);
> Index: undo.c
> ===
> RCS file: /cvs/src/bin/ed/undo.c,v
> retrieving revision 1.13
> diff -u -p -u -r1.13 undo.c
> --- undo.c9 Oct 2015 19:47:02 -1.13
> +++ undo.c16 Mar 2016 23:37:46 -
> @@ -28,8 +28,12 @@
>   * SUCH DAMAGE.
>   */
> 
> -#include "ed.h"
> +#include 
> +#include 
> +#include 
> +#include 
> 
> +#include "ed.h"
> 
>  #define USIZE 100/* undo stack size */
>  static undo_t *ustack = NULL;/* undo stack */
> 



Re: New scheduler for OpenBSD

2016-03-19 Thread Michael McConville
Edd Barrett wrote:
> On Sat, Mar 19, 2016 at 09:06:26AM +0100, Alexandre Ratchov wrote:
> > The browsers problems seem caused by the way pthreads behave;
> > browsers appear to spin.  With the proposed scheduler they spin
> > less.  But the real question is why they spin at all?
> 
> Inspired by this comment, I set out to see if I could find *where* the
> browsers are spinning. "Just build the browsers with profiling
> instrumentation and look where the time goes when playing a youtube
> video" I thought. "Easy" I thought.
> 
> Well no. We build our chrome and firefox with clang, which doesn't
> support gprof style profiling.
> 
> Clang does have this -fprofile-instr-generate flag, which throws out
> some profiling data at runtime, but it appears it is designed to be
> used by the compiler itself as compile-time optimisation hints[1].
> It's not even clear if there is any timing data in there.
> 
> There's another clang profiling mode which depends upon Linux perf,
> which is obviously not an option for us.
> 
> Nevertheless, I decided to try it on the off-chance that clang's
> profiling data could be useful (and I'm totally accepting that, if it
> is, I will probably have to write a python script to make sense of the
> output). Sadly I stumbled at the first hurdle:

I've also been meaning to try something like this:

http://poormansprofiler.org/

Seems applicable here.



Guard a macro arg in less(1)

2016-03-15 Thread Michael McConville
ok?


Index: ch.c
===
RCS file: /cvs/src/usr.bin/less/ch.c,v
retrieving revision 1.16
diff -u -p -r1.16 ch.c
--- ch.c27 Dec 2015 17:51:19 -  1.16
+++ ch.c15 Mar 2016 17:18:56 -
@@ -103,8 +103,8 @@ struct filestate {
  * Macros to manipulate the list of buffers in thisfile->hashtbl[n].
  */
 #defineFOR_BUFS_IN_CHAIN(h, bn) \
-   for (bn = thisfile->hashtbl[h].hnext;  \
-   bn != END_OF_HCHAIN(h);  bn = bn->hnext)
+   for ((bn) = thisfile->hashtbl[h].hnext;  \
+   (bn) != END_OF_HCHAIN(h);  (bn) = (bn)->hnext)
 
 #defineBUF_HASH_RM(bn) \
(bn)->hnext->hprev = (bn)->hprev; \



Re: spamd - blacklists

2016-03-15 Thread Michael McConville
Stuart Henderson wrote:
> On 2016/03/15 12:55, Craig Skinner wrote:
> > There are a few more paid rsync lists here:
> > http://en.wikipedia.org/wiki/Comparison_of_DNS_blacklists
> 
> Ah that is a useful page. Maybe we could list it, e.g.
> 
> Index: spamd.conf
> ===
> RCS file: /cvs/src/etc/mail/spamd.conf,v
> retrieving revision 1.5
> diff -u -p -r1.5 spamd.conf
> --- spamd.conf14 Mar 2016 21:36:52 -  1.5
> +++ spamd.conf15 Mar 2016 13:27:04 -
> @@ -13,8 +13,10 @@
>  # Lists specified with the :white: capability apply to the previous
>  # list with a :black: capability.
>  #
> -# As of November 2004, a place to search for blacklists is
> -# http://spamlinks.net/filter-bl.htm
> +# As of March 2016, a place to search for blacklists is
> +# http://en.wikipedia.org/wiki/Comparison_of_DNS_blacklists
> +# - most of these are DNS-based only and cannot be used with spamd(8),
> +# but some of the lists also provide access to text files via rsync.
>  
>  all:\
>   :uatraps:nixspam:

ok mmcc@

> > Generally, everything has changed from file feeds to DNS.
> 
> Yep, because for the more actively maintained ones 1) new entries show
> up more quickly than any sane rsync interval, this is quite important
> for good blocking these days 2) DNS is less resource intensive and more
> easily distributed than rsync, and 3) importantly for the rbl providers,
> it gives additional input to them about new mail sources (if an rbl
> suddenly starts seeing queries from all over the world for a previously
> unseen address, it's probably worth investigation - I am sure this is
> why some of the commercial antispam operators provide free DNS-based
> lookups for smaller orgs).
> 
> A more flexible approach would be to skip the PF table integration
> completely and do DNS lookups in spamd (or, uh, relayd, or something
> new) and based on that it could choose whether to tarpit, greylist or
> transparent-forward the connection to the real mail server. This
> would also give a way to use dnswl.org's whitelist to avoid greylisting
> for those hosts where it just doesn't work well (gmail, office365 etc).

Interesting, I didn't even know that rsync blacklists existed. That was
the cause for confusion about Spamhaus's price earlier.

Would it make sense to enable a blacklist or two by default in spamd?
They seem to be an effectively necessary part of a sane mail server
configuration these days.



Re: Optimizing chmod(1)

2016-03-14 Thread Michael McConville
Adam Thompson wrote:
> 
> On 16-03-13 11:11 PM, Michael McConville wrote:
> >It seems that chown(1) will write to a file even if it already has the
> >desired ownership. The below patch causes it to skip the write when
> >there would be no change. The best I could tell, the fts_read(3) and
> >fchownat(3) logic agree on whether to follow symlinks in all cases, so
> >there's no need to execute a useless write when certain options are
> >specified.
> >
> >I get a speedup of 5-10% on SSDs, and probably more on a slow storage
> >HD. This is when all the files are already owned by the specified user.
> >I think this is a common case, as chown is often used to "comb out"
> >offending files, ensure that a server can access everything in /var/www,
> >etc.
> >
> >The APIs involved are not simple and there's potential for subtle
> >breakage, so this only an initial patch. I'm interested to hear what
> >more experienced people think.
> >
> >If it's worthwhile, a similar approach can probably be applied to
> >chmod(1) et al. as well.
> 
> If this becomes the default behaviour, please allow a way to revert to the
> previous behaviour, as this change would break several real systems I'm
> aware of, including one I currently manage.

As I feared, and as Todd pointed out, this would break too much stuff to
be worth the modest performance improvement. It won't be committed.



Re: spamd - blacklists

2016-03-14 Thread Michael McConville
Craig Skinner wrote:
> Hi Hans,
> 
> On 2016-03-14 Mon 11:49 AM |, hans wrote:
> > On Mar 13 18:56:00, mm...@mykolab.com wrote:
> > > hans wrote:
> > > > The link to "the place to search for blacklists" is dead.
> > > 
> > > Might be better to replace it than to remove it.
> > 
> > Sure. Any suggestions?
> > 
> 
> Some DNSRBLs are available as files or rsync feeds.
> 
> It takes a bit of digging about, so start with effective ones:
> 
> http://www.intra2net.com/en/support/antispam/
> http://www.spamcannibal.org/dnsbl_compare.shtml
> http://en.wikipedia.org/wiki/Comparison_of_DNS_blacklists
> http://multirbl.valli.org/list/
> 
> Offenders can be checked in all at once on:
> http://multirbl.valli.org/dnsbl-lookup/

I think Spamhaus is the canonical one these days.



Optimizing chmod(1)

2016-03-13 Thread Michael McConville
It seems that chown(1) will write to a file even if it already has the
desired ownership. The below patch causes it to skip the write when
there would be no change. The best I could tell, the fts_read(3) and
fchownat(3) logic agree on whether to follow symlinks in all cases, so
there's no need to execute a useless write when certain options are
specified.

I get a speedup of 5-10% on SSDs, and probably more on a slow storage
HD. This is when all the files are already owned by the specified user.
I think this is a common case, as chown is often used to "comb out"
offending files, ensure that a server can access everything in /var/www,
etc.

The APIs involved are not simple and there's potential for subtle
breakage, so this only an initial patch. I'm interested to hear what
more experienced people think.

If it's worthwhile, a similar approach can probably be applied to
chmod(1) et al. as well.

Thanks for your time,
Michael


Index: chmod.c
===
RCS file: /cvs/src/bin/chmod/chmod.c,v
retrieving revision 1.39
diff -u -p -r1.39 chmod.c
--- chmod.c 31 Dec 2015 23:38:16 -  1.39
+++ chmod.c 14 Mar 2016 04:01:39 -
@@ -63,8 +63,8 @@ main(int argc, char *argv[])
int oct;
mode_t omode;
int Hflag, Lflag, Rflag, ch, fflag, fts_options, hflag, rval, atflags;
-   uid_t uid;
-   gid_t gid;
+   uid_t uid, orig_uid;
+   gid_t gid, orig_gid;
u_int32_t fclear, fset;
char *ep, *mode, *cp, *flags;
 
@@ -213,7 +213,12 @@ done:
 
if ((ftsp = fts_open(++argv, fts_options, 0)) == NULL)
err(1, NULL);
+
for (rval = 0; (p = fts_read(ftsp)) != NULL;) {
+
+   orig_uid = p->fts_statp->st_uid;
+   orig_gid = p->fts_statp->st_gid;
+
switch (p->fts_info) {
case FTS_D:
if (!Rflag)
@@ -265,6 +270,16 @@ done:
|| fflag)
continue;
} else if (!ischflags) {
+   /*
+* Don't modify the file if it already has the
+* specified ownership. fts_read(3) and
+* fchownat(3) both handle the symlink logic, so
+* we know we're deciding based on the right
+* file.
+*/
+   if ((gid == -1 || gid == orig_gid) &&
+   (uid == -1 || uid == orig_uid))
+   continue;
if (!fchownat(AT_FDCWD, p->fts_accpath, uid, gid,
atflags) || fflag)
continue;



Re: spamd - blacklists

2016-03-13 Thread Michael McConville
hans wrote:
> The link to "the place to search for blacklists" is dead.

Might be better to replace it than to remove it.

> Index: spamd.conf
> ===
> RCS file: /cvs/src/etc/mail/spamd.conf,v
> retrieving revision 1.4
> diff -u -p -r1.4 spamd.conf
> --- spamd.conf14 May 2012 16:58:46 -  1.4
> +++ spamd.conf13 Mar 2016 14:15:54 -
> @@ -12,9 +12,6 @@
>  # "all" must be here, and defines the order in which lists are applied.
>  # Lists specified with the :white: capability apply to the previous
>  # list with a :black: capability.
> -#
> -# As of November 2004, a place to search for blacklists is
> -# http://spamlinks.net/filter-bl.htm
>  
>  all:\
>   :uatraps:nixspam:
> 



Remove lint(1) reference from gcc-local(1)

2016-03-12 Thread Michael McConville
As jmc@ asked when I shared this with him, is there a remaining use of
our custom gcc -CC flag now that lint(1) is gone? It seems like it could
be useful for general macro debugging, but I'm not sure. Even if not,
removing it might be more effort than it's worth.

I'm interested to hear what people think about that. Either way, ok for
removing the lint(1) reference?


Index: share/man/man1/gcc-local.1
===
RCS file: /cvs/src/share/man/man1/gcc-local.1,v
retrieving revision 1.47
diff -u -p -r1.47 gcc-local.1
--- share/man/man1/gcc-local.1  23 Dec 2015 08:42:42 -  1.47
+++ share/man/man1/gcc-local.1  12 Mar 2016 22:54:50 -
@@ -237,8 +237,6 @@ recognizes the preprocessor flag
 that lets comments in macros pass through to the output (except in
 .Fl traditional
 mode).
-This is used to allow annotations in macros for
-lint.
 .It
 The warning option
 .Fl Wsystem-headers ,



Re: remove 'returns no value' from man pages

2016-03-12 Thread Michael McConville
Marc Espie wrote:
> On Fri, Mar 11, 2016 at 05:18:52PM -0800, Michael McConville wrote:
> > This is specified only irregularly, and people who don't know what a
> > void return type means are beyond help anyway.
> > 
> > This also adds a sentence specifying that X509_free(3) is NULL-safe, now
> > that we've removed all NULL-checks for it. I should sweep through and
> > add the sentence the rest of the NULL-safe LibreSSL *_free() functions
> > soon.
> > 
> > ok?
> 
> I would keep the one about rewind, maybe make it even stronger.
> 
> Using rewind is icky.   It deliberately hides an error message.

Sounds good - I'll leave that one and remove the rest. Let me know if
you have a proposed wording for the additional rewind(3) warning.



remove 'returns no value' from man pages

2016-03-11 Thread Michael McConville
This is specified only irregularly, and people who don't know what a
void return type means are beyond help anyway.

This also adds a sentence specifying that X509_free(3) is NULL-safe, now
that we've removed all NULL-checks for it. I should sweep through and
add the sentence the rest of the NULL-safe LibreSSL *_free() functions
soon.

ok?


Index: lib/libc/stdio/fseek.3
===
RCS file: /cvs/src/lib/libc/stdio/fseek.3,v
retrieving revision 1.16
diff -u -p -r1.16 fseek.3
--- lib/libc/stdio/fseek.3  17 Jul 2013 05:42:11 -  1.16
+++ lib/libc/stdio/fseek.3  12 Mar 2016 01:05:07 -
@@ -139,9 +139,6 @@ On some systems an
 object may be a complex object
 and these routines may be the only way to portably reposition a text stream.
 .Sh RETURN VALUES
-The
-.Fn rewind
-function returns no value.
 Upon successful completion,
 .Fn fgetpos ,
 .Fn fseek ,
Index: lib/libc/stdlib/hcreate.3
===
RCS file: /cvs/src/lib/libc/stdlib/hcreate.3,v
retrieving revision 1.6
diff -u -p -r1.6 hcreate.3
--- lib/libc/stdlib/hcreate.3   28 Jul 2010 09:00:20 -  1.6
+++ lib/libc/stdlib/hcreate.3   12 Mar 2016 01:05:07 -
@@ -159,10 +159,6 @@ Otherwise, a value of 0 is returned and
 .Va errno
 is set to indicate the error.
 .Pp
-The
-.Fn hdestroy
-functions
-returns no value.
 .Pp
 If successful, the
 .Fn hsearch
Index: lib/libc/stdlib/malloc.3
===
RCS file: /cvs/src/lib/libc/stdlib/malloc.3,v
retrieving revision 1.93
diff -u -p -r1.93 malloc.3
--- lib/libc/stdlib/malloc.35 Feb 2016 15:09:09 -   1.93
+++ lib/libc/stdlib/malloc.312 Mar 2016 01:05:07 -
@@ -174,9 +174,6 @@ and set
 to
 .Er ENOMEM .
 .Pp
-The
-.Fn free
-function returns no value.
 .Sh IDIOMS
 Consider
 .Fn calloc
Index: lib/libc/stdlib/qsort.3
===
RCS file: /cvs/src/lib/libc/stdlib/qsort.3,v
retrieving revision 1.18
diff -u -p -r1.18 qsort.3
--- lib/libc/stdlib/qsort.3 29 Jan 2015 01:46:31 -  1.18
+++ lib/libc/stdlib/qsort.3 12 Mar 2016 01:05:07 -
@@ -143,9 +143,6 @@ which is faster than
 .Fn heapsort .
 Memory availability and pre-existing order in the data can make this untrue.
 .Sh RETURN VALUES
-The
-.Fn qsort
-function returns no value.
 .Pp
 .Rv -std heapsort mergesort
 .Sh ERRORS
Index: lib/libc/stdlib/tsearch.3
===
RCS file: /cvs/src/lib/libc/stdlib/tsearch.3,v
retrieving revision 1.19
diff -u -p -r1.19 tsearch.3
--- lib/libc/stdlib/tsearch.3   5 Jun 2013 03:39:23 -   1.19
+++ lib/libc/stdlib/tsearch.3   12 Mar 2016 01:05:07 -
@@ -112,9 +112,6 @@ is
 .Dv NULL
 or the datum cannot be found.
 .Pp
-The
-.Fn twalk
-function returns no value.
 .Sh SEE ALSO
 .Xr bsearch 3 ,
 .Xr lsearch 3
Index: lib/libcrypto/man/ASN1_OBJECT_new.3
===
RCS file: /cvs/src/lib/libcrypto/man/ASN1_OBJECT_new.3,v
retrieving revision 1.2
diff -u -p -r1.2 ASN1_OBJECT_new.3
--- lib/libcrypto/man/ASN1_OBJECT_new.3 9 Sep 2015 21:58:20 -   1.2
+++ lib/libcrypto/man/ASN1_OBJECT_new.3 12 Mar 2016 01:05:07 -
@@ -47,9 +47,6 @@ returns
 and sets an error code that can be obtained by
 .Xr ERR_get_error 3 .
 Otherwise it returns a pointer to the newly allocated structure.
-.Pp
-.Fn ASN1_OBJECT_free
-returns no value.
 .Sh SEE ALSO
 .Xr d2i_ASN1_OBJECT 3 ,
 .Xr ERR_get_error 3 ,
Index: lib/libcrypto/man/BUF_MEM_new.3
===
RCS file: /cvs/src/lib/libcrypto/man/BUF_MEM_new.3,v
retrieving revision 1.2
diff -u -p -r1.2 BUF_MEM_new.3
--- lib/libcrypto/man/BUF_MEM_new.3 22 Sep 2015 08:08:07 -  1.2
+++ lib/libcrypto/man/BUF_MEM_new.3 12 Mar 2016 01:05:07 -
@@ -89,9 +89,6 @@ returns the buffer or
 .Dv NULL
 on error.
 .Pp
-.Fn BUF_MEM_free
-returns no value.
-.Pp
 .Fn BUF_MEM_grow
 returns zero on error or the new size (i.e.
 .Fa len Ns ).
Index: lib/libssl/src/doc/crypto/DH_new.pod
===
RCS file: /cvs/src/lib/libssl/src/doc/crypto/DH_new.pod,v
retrieving revision 1.3
diff -u -p -r1.3 DH_new.pod
--- lib/libssl/src/doc/crypto/DH_new.pod4 May 2014 22:26:33 -   
1.3
+++ lib/libssl/src/doc/crypto/DH_new.pod12 Mar 2016 01:05:07 -
@@ -25,8 +25,6 @@ If the allocation fails, DH_new() return
 can be obtained by L. Otherwise it returns a
 pointer to the newly allocated structure.
 
-DH_free() returns no value.
-
 =head1 SEE ALSO
 
 L, L,
Index: lib/libssl/src/doc/crypto/DH_set_method.pod
===
RCS file: 

Re: scheduler: abstract away spc_whichqs

2016-03-11 Thread Michael McConville
Michal Mazurek wrote:
> spc_whichqs is an implementation specific variable of the bsd scheduler.
> Abstract it away, easing potential future rewrite.
> 
> This gets rid of curcpu_is_idle() that's used only once, and replaces it
> with a more general cpu_is_idle(curcpu()).
> 
> As far as I can tell no binary change.

I don't usually like helper macros like that, but those conditions are
sufficiently verbose and unreadable that I think it's warranted.

ok mmcc@, pending an ok from someone who works with this code.

> Index: sys/arch/amd64/amd64/cpu.c
> ===
> RCS file: /cvs/src/sys/arch/amd64/amd64/cpu.c,v
> retrieving revision 1.95
> diff -u -p -r1.95 cpu.c
> --- sys/arch/amd64/amd64/cpu.c3 Feb 2016 03:25:07 -   1.95
> +++ sys/arch/amd64/amd64/cpu.c11 Mar 2016 15:19:41 -
> @@ -253,7 +253,7 @@ cpu_idle_mwait_cycle(void)
>   panic("idle with interrupts blocked!");
>  
>   /* something already queued? */
> - if (ci->ci_schedstate.spc_whichqs != 0)
> + if (!cpu_is_idle(ci))
>   return;
>  
>   /*
> @@ -267,7 +267,7 @@ cpu_idle_mwait_cycle(void)
>* the check in sched_idle() and here.
>*/
>   atomic_setbits_int(>ci_mwait, MWAIT_IDLING | MWAIT_ONLY);
> - if (ci->ci_schedstate.spc_whichqs == 0) {
> + if (cpu_is_idle(ci)) {
>   monitor(>ci_mwait, 0, 0);
>   if ((ci->ci_mwait & MWAIT_IDLING) == MWAIT_IDLING)
>   mwait(0, 0);
> Index: sys/arch/i386/i386/cpu.c
> ===
> RCS file: /cvs/src/sys/arch/i386/i386/cpu.c,v
> retrieving revision 1.72
> diff -u -p -r1.72 cpu.c
> --- sys/arch/i386/i386/cpu.c  7 Mar 2016 05:32:46 -   1.72
> +++ sys/arch/i386/i386/cpu.c  11 Mar 2016 15:19:41 -
> @@ -755,7 +755,7 @@ cpu_idle_mwait_cycle(void)
>   panic("idle with interrupts blocked!");
>  
>   /* something already queued? */
> - if (ci->ci_schedstate.spc_whichqs != 0)
> + if (!cpu_is_idle(ci))
>   return;
>  
>   /*
> @@ -769,7 +769,7 @@ cpu_idle_mwait_cycle(void)
>* the check in sched_idle() and here.
>*/
>   atomic_setbits_int(>ci_mwait, MWAIT_IDLING | MWAIT_ONLY);
> - if (ci->ci_schedstate.spc_whichqs == 0) {
> + if (cpu_is_idle(ci)) {
>   monitor(>ci_mwait, 0, 0);
>   if ((ci->ci_mwait & MWAIT_IDLING) == MWAIT_IDLING)
>   mwait(0, 0);
> Index: sys/dev/acpi/acpicpu.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpicpu.c,v
> retrieving revision 1.72
> diff -u -p -r1.72 acpicpu.c
> --- sys/dev/acpi/acpicpu.c29 Dec 2015 04:46:28 -  1.72
> +++ sys/dev/acpi/acpicpu.c11 Mar 2016 15:19:42 -
> @@ -1188,7 +1188,7 @@ acpicpu_idle(void)
>  #endif
>  
>   /* something already queued? */
> - if (ci->ci_schedstate.spc_whichqs != 0)
> + if (!cpu_is_idle(ci))
>   return;
>  
>   /*
> @@ -1204,7 +1204,7 @@ acpicpu_idle(void)
>   hints = (unsigned)best->address;
>   microuptime();
>   atomic_setbits_int(>ci_mwait, MWAIT_IDLING);
> - if (ci->ci_schedstate.spc_whichqs == 0) {
> + if (cpu_is_idle(ci)) {
>   /* intel errata AAI65: cflush before monitor */
>   if (ci->ci_cflushsz != 0) {
>   membar_sync();
> Index: sys/kern/kern_sched.c
> ===
> RCS file: /cvs/src/sys/kern/kern_sched.c,v
> retrieving revision 1.41
> diff -u -p -r1.41 kern_sched.c
> --- sys/kern/kern_sched.c 23 Dec 2015 14:51:17 -  1.41
> +++ sys/kern/kern_sched.c 11 Mar 2016 15:19:42 -
> @@ -148,7 +148,7 @@ sched_idle(void *v)
>   KASSERT(curproc == spc->spc_idleproc);
>  
>   while (1) {
> - while (!curcpu_is_idle()) {
> + while (!cpu_is_idle(curcpu())) {
>   struct proc *dead;
>  
>   SCHED_LOCK(s);
> Index: sys/sys/sched.h
> ===
> RCS file: /cvs/src/sys/sys/sched.h,v
> retrieving revision 1.40
> diff -u -p -r1.40 sched.h
> --- sys/sys/sched.h   9 Mar 2016 13:38:50 -   1.40
> +++ sys/sys/sched.h   11 Mar 2016 15:19:42 -
> @@ -163,7 +163,7 @@ void sched_start_secondary_cpus(void);
>  void sched_stop_secondary_cpus(void);
>  #endif
>  
> -#define curcpu_is_idle() (curcpu()->ci_schedstate.spc_whichqs == 0)
> +#define cpu_is_idle(ci)  ((ci)->ci_schedstate.spc_whichqs == 0)
>  
>  void sched_init_runqueues(void);
>  void setrunqueue(struct proc *);
> 
> -- 
> Michal Mazurek
> 



Re: malloc: 1st small step in long way to multiple pools

2016-03-10 Thread Michael McConville
Otto Moerbeek wrote:
> a future goal for malloc is to use multiple pools in threaded
> environments, to reduce lock contention. 
> 
> This is a small first step towards that goal: move two globals to the
> pool-specific struct dir_info. Currently there's only a single pool,
> but that will change one day.
> 
> Lightly tested by myself on amd64, you can help by reviewing and
> testing this.

Thanks for this - I've been meaning to study the pool logic more.

One minor thing I noticed:

> Index: malloc.c
> ===
> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.182
> diff -u -p -r1.182 malloc.c
> --- malloc.c  25 Feb 2016 00:38:51 -  1.182
> +++ malloc.c  9 Mar 2016 08:31:52 -
> @@ -93,13 +93,13 @@
>  #define MQUERY(a, sz)mquery((a), (size_t)(sz), PROT_READ | 
> PROT_WRITE, \
>  MAP_ANON | MAP_PRIVATE | MAP_FIXED, -1, (off_t)0)
>  
> -#define _MALLOC_LEAVE() if (__isthreaded) do { \
> - malloc_active--; \
> +#define _MALLOC_LEAVE(d) if (__isthreaded) do { \
> + (d)->active--; \
>   _MALLOC_UNLOCK(); \
>  } while (0)
> -#define _MALLOC_ENTER() if (__isthreaded) do { \
> +#define _MALLOC_ENTER(d) if (__isthreaded) do { \
>   _MALLOC_LOCK(); \
> - malloc_active++; \
> + (d)->active++; \
>  } while (0)

Are you sure these are guarded properly? I think the do-while block
should wrap the entire thing, including the if condition. Otherwise,
else conditions can still associate with the if.



Probable logic error in st(4)

2016-03-10 Thread Michael McConville
sys/scsi/st.c:1024 uses:

> cmd->len != 0

as a condition. However, cmd is of type struct scsi_rw_tape, and its len
field is of type u_int8_t len[3]. Therefore, the condition is always
true because len is treated as a pointer.

Clang warns about this.

Thanks for your time,
Mike



Re: head(1) -c

2016-03-09 Thread Michael McConville
Jeremie Courreges-Anglas wrote:
> >> @@ -66,13 +66,18 @@ main(int argc, char *argv[])
> >>argv++;
> >>}
> >>  
> >> -  while ((ch = getopt(argc, argv, "n:")) != -1) {
> >> +  while ((ch = getopt(argc, argv, "c:n:")) != -1) {
> >>switch (ch) {
> >> +  case 'c':
> >> +  dobytes = 1;
> >> +  p = optarg;
> >> +  break;
> >>case 'n':
> >> +  dobytes = 0;
> >
> > It's already initialized to 0, which is necessary for the head -count
> > style syntax.
> 
> I followed the behavior of GNU head here: the last specified option
> wins.  IIRC FreeBSD head(1) makes -c and -n incompatible.

I feel like erroring on multiple options would be an improvement. I
guess I can propose that as a separate diff when you're done.



Re: head(1) -c

2016-03-09 Thread Michael McConville
Jeremie Courreges-Anglas wrote:
> Today someone bumped into a port that contained head -c calls.  While
> upstream could be prodded to care a bit more about portability, support
> for head -c is widespread (GNU coreutils, busybox, FreeBSD, NetBSD) so
> I don't see any value in being different.  Plus, tail(1) already has
> support for -c.
> 
> Comments/ok?

Makes sense and works for me. I'll leave a few comments inline. Also:

> PS: the next diff will remove documentation for the obsolete "-count"
> syntax.

In what sense is it obsolete? I've always used this and I think it's
pretty standard these days. Now that I look, it seems that GNU head(1)
has it but FreeBSD's and NetBSD's don't. I'd be sad to see it go.

> Index: head.c
> ===
> RCS file: /cvs/src/usr.bin/head/head.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 head.c
> --- head.c9 Oct 2015 01:37:07 -   1.20
> +++ head.c9 Mar 2016 20:02:08 -
> @@ -51,9 +51,9 @@ main(int argc, char *argv[])
>   FILE*fp;
>   longcnt;
>   int ch, firsttime;
> - longlinecnt = 10;
> + longcount = 10;

It seems like it'd be better to make this a long long for the sake of
robustness. It's conceivable that someone could try to truncate a file
to a few gigabytes.

>   char*p = NULL;
> - int status = 0;
> + int dobytes = 0, status = 0;

I'd definitely prefer stdbool here, but that's contentious.

>  
>   if (pledge("stdio rpath", NULL) == -1)
>   err(1, "pledge");
> @@ -66,13 +66,18 @@ main(int argc, char *argv[])
>   argv++;
>   }
>  
> - while ((ch = getopt(argc, argv, "n:")) != -1) {
> + while ((ch = getopt(argc, argv, "c:n:")) != -1) {
>   switch (ch) {
> + case 'c':
> + dobytes = 1;
> + p = optarg;
> + break;
>   case 'n':
> + dobytes = 0;

It's already initialized to 0, which is necessary for the head -count
style syntax.

>   p = optarg;
>   break;
>   default:
> - usage();
> + usage();

Good catch.

>   }
>   }
>   argc -= optind, argv += optind;
> @@ -80,9 +85,10 @@ main(int argc, char *argv[])
>   if (p) {
>   const char *errstr;
>  
> - linecnt = strtonum(p, 1, LONG_MAX, );
> + count = strtonum(p, 1, LONG_MAX, );

If using long long, this would have to change too.

>   if (errstr)
> - errx(1, "line count %s: %s", errstr, p);
> + errx(1, "%s count %s: %s", dobytes ? "bytes" : "lines",
> + errstr, p);
>   }
>  
>   for (firsttime = 1; ; firsttime = 0) {
> @@ -105,10 +111,16 @@ main(int argc, char *argv[])
>   }
>   ++argv;
>   }
> - for (cnt = linecnt; cnt && !feof(fp); --cnt)
> - while ((ch = getc(fp)) != EOF)
> - if (putchar(ch) == '\n')
> - break;
> + if (dobytes) {
> + for (cnt = count; cnt && !feof(fp); --cnt)
> + if ((ch = getc(fp)) != EOF)
> + putchar(ch);
> + } else {
> + for (cnt = count; cnt && !feof(fp); --cnt)
> + while ((ch = getc(fp)) != EOF)
> + if (putchar(ch) == '\n')
> + break;
> + }

Total nitpick, but I think switching the for loop condition to "cnt > 0
&& !feof(fp)" would be nice.

>   fclose(fp);
>   }
>   /*NOTREACHED*/

Might as well delete /*NOTREACHED*/



Re: Xorg stipple

2016-03-09 Thread Michael McConville
Theo de Raadt wrote:
> > > Is anyone seriously finding video/Xorg bugs through the default X
> > > stipple pattern anymore?  Xorg changed the default to draw a black
> > > background a while ago (with stipple enabled using the -retro flag),
> > > but we have this local change that reverted it while adding a silly
> > > -retard flag in order to show the black background.
> > > 
> > > I think we can finally stop partying like it's 1989 (vax is dead,
> > > after all) and have X show a solid black background by default.
> > 
> > The reason for this is to be able distinguish between:
> > 
> >   "I get a black screen because X is not working"
> > 
> > and
> > 
> >   "I get a black screen because my desktop environment is not working"
> 
> And as we all can observe, this problem has become more severe over
> the last decade.

Can anyone think of a good option that isn't black but is less hideous
than stipple? Even a dark navy or something like that.



Simplify tcpdump

2016-03-09 Thread Michael McConville
I think we can assume that people have abs(3) these days...

This macro only had one usage, which is visible in the diff. It doesn't
look like replacing it should cause any functional changes to the
arithmetic.

ok?


Index: print-icmp6.c
===
RCS file: /cvs/src/usr.sbin/tcpdump/print-icmp6.c,v
retrieving revision 1.17
diff -u -p -r1.17 print-icmp6.c
--- print-icmp6.c   16 Nov 2015 00:16:39 -  1.17
+++ print-icmp6.c   9 Mar 2016 08:05:24 -
@@ -43,6 +43,7 @@
 #include 
 
 #include 
+#include 
 #include 
 
 #include 
@@ -435,9 +436,6 @@ icmp6_print(const u_char *bp, u_int leng
mode = FQDN;
}
}
-#ifndef abs
-#define abs(a) ((0 < (a)) ? (a) : -(a))
-#endif
if (mode == UNKNOWN && 2 < abs(buf[12] - (ep - buf - 13)))
mode = WRU;
if (mode == UNKNOWN)



Re: NULL checks of static arrays

2016-03-08 Thread Michael McConville
Martin Pieuchot wrote:
> On 06/03/16(Sun) 19:20, Michael McConville wrote:
> > We check static arrays against NULL pretty often in the kernel. I
> > suspect most of these are due to recent kernel API changes. Should they
> > be removed, or do people want to keep them around in case the APIs
> > change again? Clang 3.7 warns about them by default, so they're easy to
> > find.
> 
> I think you should post diffs so we can decide on a case-by-case basis.
> 
> But if they are leftovers from cleanups, then we should clean them.

Here are a few examples I have sitting in my tree:


Index: sys/dev/ic/ti.c
===
RCS file: /cvs/src/sys/dev/ic/ti.c,v
retrieving revision 1.22
diff -u -p -r1.22 ti.c
--- sys/dev/ic/ti.c 25 Nov 2015 03:09:58 -  1.22
+++ sys/dev/ic/ti.c 8 Mar 2016 22:02:50 -
@@ -484,9 +484,6 @@ ti_handle_events(struct ti_softc *sc)
struct ti_event_desc*e;
struct ifnet*ifp = >arpcom.ac_if;
 
-   if (sc->ti_rdata->ti_event_ring == NULL)
-   return;
-
while (sc->ti_ev_saved_considx != sc->ti_ev_prodidx.ti_idx) {
e = >ti_rdata->ti_event_ring[sc->ti_ev_saved_considx];
switch (TI_EVENT_EVENT(e)) {
@@ -846,9 +843,6 @@ ti_free_tx_ring(struct ti_softc *sc)
 {
int i;
struct ti_txmap_entry *entry;
-
-   if (sc->ti_rdata->ti_tx_ring == NULL)
-   return;
 
for (i = 0; i < TI_TX_RING_CNT; i++) {
if (sc->ti_cdata.ti_tx_chain[i] != NULL) {
Index: sys/dev/usb/uoakrh.c
===
RCS file: /cvs/src/sys/dev/usb/uoakrh.c,v
retrieving revision 1.13
diff -u -p -r1.13 uoakrh.c
--- sys/dev/usb/uoakrh.c9 Jan 2016 04:14:42 -   1.13
+++ sys/dev/usb/uoakrh.c8 Mar 2016 22:02:50 -
@@ -215,11 +215,6 @@ uoakrh_detach(struct device *self, int f
wakeup(>sc_sensortask);
sensordev_deinstall(>sc_sensordev);
 
-   if (>sc_sensordev != NULL) {
-   sensor_detach(>sc_sensordev, >sc_sensor.temp);
-   sensor_detach(>sc_sensordev, >sc_sensor.humi);
-   }
-
if (sc->sc_sensortask != NULL)
sensor_task_unregister(sc->sc_sensortask);
 



Re: df(1): replace __progname with getprogname()

2016-03-06 Thread Michael McConville
ok mmcc@

Michal Mazurek wrote:
> Index: bin/df/df.c
> ===
> RCS file: /cvs/src/bin/df/df.c,v
> retrieving revision 1.55
> diff -u -p -r1.55 df.c
> --- bin/df/df.c   8 Feb 2016 16:23:54 -   1.55
> +++ bin/df/df.c   1 Mar 2016 11:36:54 -
> @@ -47,8 +47,6 @@
>  #include 
>  #include 
>  
> -extern   char *__progname;
> -
>  char *getmntpt(char *);
>  int   selected(const char *);
>  void  maketypelist(char *);
> @@ -461,6 +459,6 @@ usage(void)
>  {
>   (void)fprintf(stderr,
>   "usage: %s [-hiklnP] [-t type] [[file | file_system] ...]\n",
> - __progname);
> + getprogname());
>   exit(1);
>  }
> 
> -- 
> Michal Mazurek
> 



NULL checks of static arrays

2016-03-06 Thread Michael McConville
We check static arrays against NULL pretty often in the kernel. I
suspect most of these are due to recent kernel API changes. Should they
be removed, or do people want to keep them around in case the APIs
change again? Clang 3.7 warns about them by default, so they're easy to
find.



Re: [patch] telnet: stored value never read

2016-03-06 Thread Michael McConville
frit...@alokat.org wrote:
> the value stored in "old" is never read.

It's best to leave these unless there's a good reason not to. It
probably means that the programmer wanted to maintain the validity of
the variable in case we decide to use it later in the function.

> Index: sys_bsd.c
> ===
> RCS file: /cvs/src/usr.bin/telnet/sys_bsd.c,v
> retrieving revision 1.31
> diff -u -r1.31 sys_bsd.c
> --- sys_bsd.c 29 Nov 2015 14:18:40 -  1.31
> +++ sys_bsd.c 6 Mar 2016 20:36:11 -
> @@ -272,7 +272,6 @@
>   } while (old < 0 || old > 1);
>  }
>  
> -old = prevmode;
>  prevmode = f&~MODE_FORCE;
>  tmp_tc = new_tc;
>  
> 



Re: /usr/games/adventure include clean up

2016-03-06 Thread Michael McConville
There has to be a blank line after the sys includes.

Edgar Pettijohn wrote:
> --- io.c.origSun Mar  6 12:43:12 2016
> +++ io.cSun Mar  6 12:43:36 2016
> @@ -37,7 +37,9 @@
> 
>  /*Re-coding of advent in C: file i/o and user i/o*/
> 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> --- subr.c.origSun Mar  6 12:39:53 2016
> +++ subr.cSun Mar  6 12:42:36 2016
> @@ -37,8 +37,11 @@
> 
>  /*Re-coding of advent in C: subroutines from main*/
> 
> +#include 
> +#include 
>  #include 
>  #include 
> +#include 
>  #include "hdr.h"
>  #include "extern.h"
> 
> --- wizard.c.origSun Mar  6 12:40:09 2016
> +++ wizard.cSun Mar  6 12:40:59 2016
> @@ -37,11 +37,12 @@
> 
>  /*Re-coding of advent in C: privileged operations*/
> 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> -
>  #include "extern.h"
>  #include "hdr.h"
> 
> 
> 
> On 03/06/16 12:35, Michael McConville wrote:
> >The sys includes should be first and separate. See style(9).
> >
> >Edgar Pettijohn wrote:
> >>--- crc.c.origSun Mar  6 10:31:34 2016
> >>+++ crc.cSun Mar  6 10:31:55 2016
> >>@@ -33,6 +33,7 @@
> >>   * SUCH DAMAGE.
> >>   */
> >>
> >>+#include 
> >>  #include "extern.h"
> >>
> >>  unsigned long crctab[] = {
> >>--- done.c.origSun Mar  6 10:40:59 2016
> >>+++ done.cSun Mar  6 10:42:25 2016
> >>@@ -37,8 +37,10 @@
> >>
> >>  /*Re-coding of advent in C: termination routines*/
> >>
> >>+#include 
> >>  #include 
> >>  #include 
> >>+#include 
> >>  #include "hdr.h"
> >>  #include "extern.h"
> >>
> >>--- extern.h.origSun Mar  6 10:24:01 2016
> >>+++ extern.hSun Mar  6 10:24:10 2016
> >>@@ -30,9 +30,6 @@
> >>   * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >>   */
> >>
> >>-#include 
> >>-#include 
> >>-
> >>  /* crc.c */
> >>  void crc_start(void);
> >>  unsigned long crc(const char *, int);
> >>--- hdr.h.origSun Mar  6 10:40:26 2016
> >>+++ hdr.hSun Mar  6 10:40:43 2016
> >>@@ -52,9 +52,6 @@
> >>
> >>  /* hdr.h: included by c advent files */
> >>
> >>-#include 
> >>-#include 
> >>-
> >>  int datfd;/* message file descriptor*/
> >>  volatile sig_atomic_t delhit;
> >>  int yea;
> >>--- io.c.origSun Mar  6 10:47:06 2016
> >>+++ io.cSun Mar  6 10:47:42 2016
> >>@@ -38,9 +38,11 @@
> >>  /*Re-coding of advent in C: file i/o and user i/o*/
> >>
> >>  #include 
> >>+#include 
> >>  #include 
> >>  #include 
> >>  #include 
> >>+#include 
> >>  #include "hdr.h"
> >>  #include "extern.h"
> >>
> >>--- main.c.origSun Mar  6 10:25:22 2016
> >>+++ main.cSun Mar  6 10:25:38 2016
> >>@@ -41,6 +41,7 @@
> >>  #include 
> >>  #include 
> >>  #include 
> >>+#include 
> >>  #include 
> >>  #include "hdr.h"
> >>  #include "extern.h"
> >>--- save.c.origSun Mar  6 10:42:59 2016
> >>+++ save.cSun Mar  6 10:43:46 2016
> >>@@ -35,6 +35,7 @@
> >>   * SUCH DAMAGE.
> >>   */
> >>
> >>+#include 
> >>  #include 
> >>  #include 
> >>  #include "hdr.h"
> >>--- subr.c.origSun Mar  6 10:26:03 2016
> >>+++ subr.cSun Mar  6 10:45:14 2016
> >>@@ -37,8 +37,11 @@
> >>
> >>  /*Re-coding of advent in C: subroutines from main*/
> >>
> >>+#include 
> >>  #include 
> >>  #include 
> >>+#include 
> >>+#include 
> >>  #include "hdr.h"
> >>  #include "extern.h"
> >>
> >>--- vocab.c.origSun Mar  6 10:26:40 2016
> >>+++ vocab.cSun Mar  6 10:46:00 2016
> >>@@ -38,8 +38,11 @@
> >>  /*Re-coding of advent in C: data structure routines*/
> >>
> >>  #include 
> >>+#include 
> >>  #include 
> >>  #include 
> >>+#include 
> >>+#include 
> >>  #include "hdr.h"
> >>  #include "extern.h"
> >>
> >>--- wizard.c.origSun Mar  6 10:46:17 2016
> >>+++ wizard.cSun Mar  6 10:46:55 2016
> >>@@ -38,9 +38,11 @@
> >>  /*Re-coding of advent in C: privileged operations*/
> >>
> >>  #include 
> >>+#include 
> >>  #include 
> >>  #include 
> >>  #include 
> >>+#include 
> >>  #include "extern.h"
> >>  #include "hdr.h"
> >>
> 



Re: /usr/games/adventure include clean up

2016-03-06 Thread Michael McConville
The sys includes should be first and separate. See style(9).

Edgar Pettijohn wrote:
> --- crc.c.origSun Mar  6 10:31:34 2016
> +++ crc.cSun Mar  6 10:31:55 2016
> @@ -33,6 +33,7 @@
>   * SUCH DAMAGE.
>   */
> 
> +#include 
>  #include "extern.h"
> 
>  unsigned long crctab[] = {
> --- done.c.origSun Mar  6 10:40:59 2016
> +++ done.cSun Mar  6 10:42:25 2016
> @@ -37,8 +37,10 @@
> 
>  /*Re-coding of advent in C: termination routines*/
> 
> +#include 
>  #include 
>  #include 
> +#include 
>  #include "hdr.h"
>  #include "extern.h"
> 
> --- extern.h.origSun Mar  6 10:24:01 2016
> +++ extern.hSun Mar  6 10:24:10 2016
> @@ -30,9 +30,6 @@
>   * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
> 
> -#include 
> -#include 
> -
>  /* crc.c */
>  void crc_start(void);
>  unsigned long crc(const char *, int);
> --- hdr.h.origSun Mar  6 10:40:26 2016
> +++ hdr.hSun Mar  6 10:40:43 2016
> @@ -52,9 +52,6 @@
> 
>  /* hdr.h: included by c advent files */
> 
> -#include 
> -#include 
> -
>  int datfd;/* message file descriptor*/
>  volatile sig_atomic_t delhit;
>  int yea;
> --- io.c.origSun Mar  6 10:47:06 2016
> +++ io.cSun Mar  6 10:47:42 2016
> @@ -38,9 +38,11 @@
>  /*Re-coding of advent in C: file i/o and user i/o*/
> 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "hdr.h"
>  #include "extern.h"
> 
> --- main.c.origSun Mar  6 10:25:22 2016
> +++ main.cSun Mar  6 10:25:38 2016
> @@ -41,6 +41,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include "hdr.h"
>  #include "extern.h"
> --- save.c.origSun Mar  6 10:42:59 2016
> +++ save.cSun Mar  6 10:43:46 2016
> @@ -35,6 +35,7 @@
>   * SUCH DAMAGE.
>   */
> 
> +#include 
>  #include 
>  #include 
>  #include "hdr.h"
> --- subr.c.origSun Mar  6 10:26:03 2016
> +++ subr.cSun Mar  6 10:45:14 2016
> @@ -37,8 +37,11 @@
> 
>  /*Re-coding of advent in C: subroutines from main*/
> 
> +#include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include "hdr.h"
>  #include "extern.h"
> 
> --- vocab.c.origSun Mar  6 10:26:40 2016
> +++ vocab.cSun Mar  6 10:46:00 2016
> @@ -38,8 +38,11 @@
>  /*Re-coding of advent in C: data structure routines*/
> 
>  #include 
> +#include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include "hdr.h"
>  #include "extern.h"
> 
> --- wizard.c.origSun Mar  6 10:46:17 2016
> +++ wizard.cSun Mar  6 10:46:55 2016
> @@ -38,9 +38,11 @@
>  /*Re-coding of advent in C: privileged operations*/
> 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "extern.h"
>  #include "hdr.h"
> 



Re: cpu.h: parens in macros

2016-03-01 Thread Michael McConville
Michal Mazurek wrote:
> I noticed these macros, I think they should have more parenthesis. I
> don't know how to test this though.

tentative ok mmcc@

> Index: sys/arch/alpha/include/cpu.h
> ===
> RCS file: /cvs/src/sys/arch/alpha/include/cpu.h,v
> retrieving revision 1.55
> diff -u -p -r1.55 cpu.h
> --- sys/arch/alpha/include/cpu.h  2 Jul 2015 01:33:59 -   1.55
> +++ sys/arch/alpha/include/cpu.h  1 Mar 2016 18:57:50 -
> @@ -320,7 +320,7 @@ do {  
> \
>  #define signotify(p) aston(p)
>  #endif
>  
> -#define  aston(p)(p)->p_md.md_astpending = 1
> +#define  aston(p)((p)->p_md.md_astpending = 1)
>  #endif /* _KERNEL */
>  
>  /*
> Index: sys/arch/mips64/include/cpu.h
> ===
> RCS file: /cvs/src/sys/arch/mips64/include/cpu.h,v
> retrieving revision 1.108
> diff -u -p -r1.108 cpu.h
> --- sys/arch/mips64/include/cpu.h 5 Jan 2016 05:27:54 -   1.108
> +++ sys/arch/mips64/include/cpu.h 1 Mar 2016 18:57:50 -
> @@ -318,12 +318,12 @@ voidcp0_calibrate(struct cpu_info *);
>   * process as soon as possible.
>   */
>  #ifdef MULTIPROCESSOR
> -#define  signotify(p)(aston(p), cpu_unidle(p->p_cpu))
> +#define  signotify(p)(aston(p), cpu_unidle((p)->p_cpu))
>  #else
>  #define  signotify(p)aston(p)
>  #endif
>  
> -#define  aston(p)p->p_md.md_astpending = 1
> +#define  aston(p)((p)->p_md.md_astpending = 1)
>  
>  #ifdef CPU_R8000
>  #define  mips_sync() __asm__ volatile ("lw $0, 0(%0)" :: \
> 
> -- 
> Michal Mazurek
> 



Re: df(1): remove unneeded includes

2016-03-01 Thread Michael McConville
Theo Buehler wrote:
> On Tue, Mar 01, 2016 at 12:37:17PM +0100, Theo Buehler wrote:
> > Wasn't this the part of your previous header cleanup that mmcc
> > decided not to commit for some reason? 
> 
> if mmcc no longer objects, this once more ok tb@

Right, I misunderstood which file this was removing unistd.h and
stdlib.h from. I'll commit.



tmpfs improvements

2016-02-27 Thread Michael McConville
 * EINVAL is returned in a case where there is insufficient memory.
   ENOMEM makes more sense. This one is ok natano@.

 * There wasn't yet a list of possible errors for tmpfs in mount(2).
   That said, I question the value of maintaining these lists. It's
   almost impossible for them to be comprehensive - for example, some
   *_mount() functions return the error value of library functions like
   copyin(9) and copyout(9) if they fail. Errors like ENOMEM are
   probably sufficiently self-explanatory to be excluded. These lists
   likely make sense for explaining anomalous errors, though.

   This one was was suggested by but not yet reviewed by natano@.

 * The KASSERT() on sys/tmpfs/tmpfs_vfsops.c:160 checks root against
   NULL after calling tmpfs_alloc_node(). However, tmpfs_alloc_node()
   only sets root on success, and in that case the value will always be
   non-null. We should therefore initialize root to NULL. We should
   maybe consider just returning tmpfs_alloc_node()'s error code instead
   of KASSERTing.

 * Replace a few bzeros with memset. Also ok natano@.


Index: lib/libc/sys/mount.2
===
RCS file: /cvs/src/lib/libc/sys/mount.2,v
retrieving revision 1.45
diff -u -p -r1.45 mount.2
--- lib/libc/sys/mount.223 Nov 2015 10:01:45 -  1.45
+++ lib/libc/sys/mount.227 Feb 2016 22:21:51 -
@@ -384,6 +384,18 @@ An I/O error occurred while writing cach
 .Fa dir
 points outside the process's allocated address space.
 .El
+.Pp
+The following errors can occur for a
+.Em tmpfs
+filesystem mount:
+.Bl -tag -width [ENOMEM]
+.It Bq Er ENOMEM
+There is not enough memory available to allocate the mount structures.
+.It Bq Er ENOTSUPP
+The
+.Dv MNT_UPDATE
+flag, which is not yet supported, was set.
+.El
 .Sh SEE ALSO
 .Xr statfs 2 ,
 .Xr mount 8 ,
Index: sys/tmpfs/tmpfs_vfsops.c
===
RCS file: /cvs/src/sys/tmpfs/tmpfs_vfsops.c,v
retrieving revision 1.8
diff -u -p -r1.8 tmpfs_vfsops.c
--- sys/tmpfs/tmpfs_vfsops.c13 Jan 2016 13:01:40 -  1.8
+++ sys/tmpfs/tmpfs_vfsops.c27 Feb 2016 22:21:55 -
@@ -87,7 +87,7 @@ tmpfs_mount(struct mount *mp, const char
 {
struct tmpfs_args args;
tmpfs_mount_t *tmp;
-   tmpfs_node_t *root;
+   tmpfs_node_t *root = NULL;
uint64_t memlimit;
uint64_t nodes;
int error;
@@ -120,7 +120,7 @@ tmpfs_mount(struct mount *mp, const char
 
/* Prohibit mounts if there is not enough memory. */
if (tmpfs_mem_info(1) < TMPFS_PAGES_RESERVED)
-   return EINVAL;
+   return ENOMEM;
 
error = copyin(data, , sizeof(struct tmpfs_args));
if (error)
@@ -180,9 +180,12 @@ tmpfs_mount(struct mount *mp, const char
 
mp->mnt_stat.mount_info.tmpfs_args = args;
 
-   bzero(>mnt_stat.f_mntonname, sizeof(mp->mnt_stat.f_mntonname));
-   bzero(>mnt_stat.f_mntfromname, sizeof(mp->mnt_stat.f_mntfromname));
-   bzero(>mnt_stat.f_mntfromspec, sizeof(mp->mnt_stat.f_mntfromspec));
+   memset(>mnt_stat.f_mntonname, 0,
+   sizeof(mp->mnt_stat.f_mntonname));
+   memset(>mnt_stat.f_mntfromname, 0,
+   sizeof(mp->mnt_stat.f_mntfromname));
+   memset(>mnt_stat.f_mntfromspec, 0,
+   sizeof(mp->mnt_stat.f_mntfromspec));
 
strlcpy(mp->mnt_stat.f_mntonname, path,
sizeof(mp->mnt_stat.f_mntonname) - 1);



Re: Correct error number in crypto(9)

2016-02-27 Thread Michael McConville
Mike Belopuhov wrote:
> On 27 February 2016 at 08:21, Michael McConville <mm...@mykolab.com> wrote:
> > Michael McConville wrote:
> >> Michael McConville wrote:
> >> > Does this make sense?
> >>
> >> I just realized that the allocation failure checks earlier in the
> >> function return ENOBUFS. This probably makes more sense for the sake of
> >> consistency.
> >
> > The best I can tell, the only use of this function is in
> > sys/crypto/crypto.c:157. It's accessed through a pointer stored in a
> > struct by crypto_register(). That usage doesn't seem to be affected
> > by the below change, considering that the outcome would be no
> > different than that of the other ENOBUFS failures above it.
> >
> 
> So why change it to ENOMEM then?  Nothing there returns it.
> I think this is just needless churn.

I was proposing to change it from EINVAL to ENOBUFS.

I don't think it's churn. The current returned error code seems
objectively wrong to me, and all other allocation failures in this
function return ENOBUFS. deraadt supported it but asked me to ensure
that it wouldn't affect downstream error handling.



Re: Correct error number in crypto(9)

2016-02-26 Thread Michael McConville
Michael McConville wrote:
> Michael McConville wrote:
> > Does this make sense?
> 
> I just realized that the allocation failure checks earlier in the
> function return ENOBUFS. This probably makes more sense for the sake of
> consistency.

The best I can tell, the only use of this function is in
sys/crypto/crypto.c:157. It's accessed through a pointer stored in a
struct by crypto_register(). That usage doesn't seem to be affected by
the below change, considering that the outcome would be no different
than that of the other ENOBUFS failures above it.

> > Index: sys/crypto/cryptosoft.c
> > ===
> > RCS file: /cvs/src/sys/crypto/cryptosoft.c,v
> > retrieving revision 1.80
> > diff -u -p -r1.80 cryptosoft.c
> > --- sys/crypto/cryptosoft.c 10 Dec 2015 21:00:51 -  1.80
> > +++ sys/crypto/cryptosoft.c 26 Feb 2016 17:21:00 -
> > @@ -826,7 +826,7 @@ swcr_newsession(u_int32_t *sid, struct c
> > M_CRYPTO_DATA, M_NOWAIT | M_ZERO);
> > if ((*swd)->sw_kschedule == NULL) {
> > swcr_freesession(i);
> > -   return EINVAL;
> > +   return ENOMEM;
> > }
> > }
> > if (txf->setkey((*swd)->sw_kschedule, cri->cri_key,
> > 
> 



Re: Correct error number in crypto(9)

2016-02-26 Thread Michael McConville
Michael McConville wrote:
> Does this make sense?

I just realized that the allocation failure checks earlier in the
function return ENOBUFS. This probably makes more sense for the sake of
consistency.

> Index: sys/crypto/cryptosoft.c
> ===
> RCS file: /cvs/src/sys/crypto/cryptosoft.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 cryptosoft.c
> --- sys/crypto/cryptosoft.c   10 Dec 2015 21:00:51 -  1.80
> +++ sys/crypto/cryptosoft.c   26 Feb 2016 17:21:00 -
> @@ -826,7 +826,7 @@ swcr_newsession(u_int32_t *sid, struct c
>   M_CRYPTO_DATA, M_NOWAIT | M_ZERO);
>   if ((*swd)->sw_kschedule == NULL) {
>   swcr_freesession(i);
> - return EINVAL;
> + return ENOMEM;
>   }
>   }
>   if (txf->setkey((*swd)->sw_kschedule, cri->cri_key,
> 



Correct error number in crypto(9)

2016-02-26 Thread Michael McConville
Does this make sense?


Index: sys/crypto/cryptosoft.c
===
RCS file: /cvs/src/sys/crypto/cryptosoft.c,v
retrieving revision 1.80
diff -u -p -r1.80 cryptosoft.c
--- sys/crypto/cryptosoft.c 10 Dec 2015 21:00:51 -  1.80
+++ sys/crypto/cryptosoft.c 26 Feb 2016 17:21:00 -
@@ -826,7 +826,7 @@ swcr_newsession(u_int32_t *sid, struct c
M_CRYPTO_DATA, M_NOWAIT | M_ZERO);
if ((*swd)->sw_kschedule == NULL) {
swcr_freesession(i);
-   return EINVAL;
+   return ENOMEM;
}
}
if (txf->setkey((*swd)->sw_kschedule, cri->cri_key,



Re: Make alpha 2038-safe

2016-02-17 Thread Michael McConville
Michael McConville wrote:
> Now that we have 64-bit time. Otherwise, all the people running Alpha in
> 2038 will yell at us.

And sh:


Index: arch/sh/sh/clock.c
===
RCS file: /cvs/src/sys/arch/sh/sh/clock.c,v
retrieving revision 1.7
diff -u -p -r1.7 clock.c
--- arch/sh/sh/clock.c  8 Sep 2012 22:01:25 -   1.7
+++ arch/sh/sh/clock.c  17 Feb 2016 18:32:44 -
@@ -309,7 +309,7 @@ inittodr(time_t base)
 
if (!(sh_clock.flags & SH_CLOCK_NOINITTODR) &&
(rtc < base ||
-   dt.dt_year < MINYEAR || dt.dt_year > 2037 ||
+   dt.dt_year < MINYEAR ||
dt.dt_mon < 1 || dt.dt_mon > 12 ||
dt.dt_wday > 6 ||
dt.dt_day < 1 || dt.dt_day > 31 ||



Make alpha 2038-safe

2016-02-17 Thread Michael McConville
Now that we have 64-bit time. Otherwise, all the people running Alpha in
2038 will yell at us.


Index: sys/arch/alpha/alpha/clock.c
===
RCS file: /cvs/src/sys/arch/alpha/alpha/clock.c,v
retrieving revision 1.21
diff -u -p -r1.21 clock.c
--- sys/arch/alpha/alpha/clock.c23 Mar 2012 15:51:25 -  1.21
+++ sys/arch/alpha/alpha/clock.c17 Feb 2016 16:59:22 -
@@ -212,9 +212,8 @@ inittodr(time_t base)
year = 1900 + UNIX_YEAR_OFFSET + ct.year;
if (year < 1970)
year += 100;
-   /* simple sanity checks (2037 = time_t overflow) */
-   if (year < MINYEAR || year > 2037 ||
-   ct.mon < 1 || ct.mon > 12 || ct.day < 1 ||
+   /* simple sanity checks */
+   if (year < MINYEAR || ct.mon < 1 || ct.mon > 12 || ct.day < 1 ||
ct.day > 31 || ct.hour > 23 || ct.min > 59 || ct.sec > 59) {
/*
 * Believe the time in the file system for lack of



Fix overflow check in sys/netinet6/in6.c

2016-02-12 Thread Michael McConville
time_second is a time_t, which we define as int64_t. The other operands
used are of type uint32_t. Therefore, these checks get promoted to
int64_t and the overflow being tested is undefined because it uses
signed arithmetic.

I think that the below diff fixes the overflow check. However, I'm
pretty tired at the moment and this is hard to do right, so review is
appreciated.


Index: netinet6/in6.c
===
RCS file: /cvs/src/sys/netinet6/in6.c,v
retrieving revision 1.183
diff -u -p -r1.183 in6.c
--- netinet6/in6.c  21 Jan 2016 11:23:48 -  1.183
+++ netinet6/in6.c  12 Feb 2016 19:44:10 -
@@ -70,6 +70,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -348,11 +349,13 @@ in6_control(struct socket *so, u_long cm
/* sanity for overflow - beware unsigned */
lt = >ifr_ifru.ifru_lifetime;
if (lt->ia6t_vltime != ND6_INFINITE_LIFETIME
-&& lt->ia6t_vltime + time_second < time_second) {
+   && (time_second > 0
+   && time_second > INT64_MAX - lt->ia6t_vltime)) {
return EINVAL;
}
if (lt->ia6t_pltime != ND6_INFINITE_LIFETIME
-&& lt->ia6t_pltime + time_second < time_second) {
+   && (time_second > 0
+   && time_second > INT64_MAX - lt->ia6t_pltime)) {
return EINVAL;
}
break;



Re: Integer overflow in syslogd

2016-02-12 Thread Michael McConville
Michael Savage wrote:
> Here's a patch with less fragile parsing code.

Comments inline.

> Index: syslogd.c
> ===
> RCS file: /cvs/src/usr.sbin/syslogd/syslogd.c,v
> retrieving revision 1.177
> diff -u -p -r1.177 syslogd.c
> --- syslogd.c 20 Jul 2015 19:49:33 -  1.177
> +++ syslogd.c 12 Feb 2016 20:51:41 -
> @@ -1324,6 +1324,30 @@ usage(void)
>   exit(1);
>  }

A one- or two-sentence comment describing the function may be nice, as
it isn't immediately obvious what it does. Not entirely necessary
though.

> +size_t
> +parsepriority(const char *msg, int *pri)

Nitpick, but I'd probably slightly prefer parse_priority.

> +{
> + size_t nlen;
> + char buf[11];
> + const char *errstr;
> + int maybepri;
> +
> + if (*msg++ == '<') {
> + nlen = strspn(msg, "1234567890");
> + if (nlen > 0 && nlen < sizeof(buf) && msg[nlen] == '>') {
> + memcpy(buf, msg, nlen);
> + buf[nlen] = '\0';

The above two lines should probably be strlcpy(buf, msg, nlen+1).

> + maybepri = strtonum(buf, 0, INT_MAX, );
> + if (errstr == NULL) {
> + *pri = maybepri;
> + return nlen + 2;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
>  /*
>   * Take a raw input line, decode the message, and print the message
>   * on the appropriate log files.
> @@ -1337,13 +1361,7 @@ printline(char *hname, char *msg)
>   /* test for special codes */
>   pri = DEFUPRI;
>   p = msg;
> - if (*p == '<') {
> - pri = 0;
> - while (isdigit((unsigned char)*++p))
> - pri = 10 * pri + (*p - '0');
> - if (*p == '>')
> - ++p;
> - }
> + p += parsepriority(p, );
>   if (pri &~ (LOG_FACMASK|LOG_PRIMASK))
>   pri = DEFUPRI;
>  
> @@ -1374,19 +1392,16 @@ printsys(char *msg)
>  {
>   int c, pri, flags;
>   char *lp, *p, *q, line[MAXLINE + 1];
> + size_t prilen;
>  
>   (void)snprintf(line, sizeof line, "%s: ", _PATH_UNIX);
>   lp = line + strlen(line);
>   for (p = msg; *p != '\0'; ) {
>   flags = SYNC_FILE | ADDDATE;/* fsync file after write */
>   pri = DEFSPRI;
> - if (*p == '<') {
> - pri = 0;
> - while (isdigit((unsigned char)*++p))
> - pri = 10 * pri + (*p - '0');
> - if (*p == '>')
> - ++p;
> - } else {
> + prilen = parsepriority(p, );
> + p += prilen;
> + if (prilen == 0) {
>   /* kernel printf's come out on console */
>   flags |= IGN_CONS;
>   }
> 

Looking at the old code again, it seems like the '>' was effectively
optional, and a stray '<' could zero pri. Was this a bug or a feature?

Otherwise, looks good to me. Thanks again!



Re: Integer overflow in syslogd

2016-02-11 Thread Michael McConville
Michael Savage wrote:
> I found an integer overflow in syslogd which can be triggered by
> compiling and running:
> 
> #include 
> #include 
> #include 
> 
> int main( int argc, char ** argv ) {
>   const char * msg = "<> hello";
> return sendsyslog( msg, strlen( msg ) );
> }
> 
> 
> The problematic code is a hand-rolled int parser in printline/printsys:
> 
> pri = 0;
> while (isdigit((unsigned char)*++p))
>   pri = 10 * pri + (*p - '0');
> if (*p == '>')
>   ++p;
> 
> 
> I've attached a patch which converts the loop to strtonum, but doesn't
> exactly mirror the behaviour of the old code in funny cases, like
> sendsyslog("<1no closing angle bracket").

Thanks for this.

A few comments, some trivial:

> Index: syslogd.c
> ===
> RCS file: /cvs/src/usr.sbin/syslogd/syslogd.c,v
> retrieving revision 1.177
> diff -u -p -r1.177 syslogd.c
> --- syslogd.c 20 Jul 2015 19:49:33 -  1.177
> +++ syslogd.c 11 Feb 2016 20:31:55 -
> @@ -1331,18 +1331,23 @@ usage(void)
>  void
>  printline(char *hname, char *msg)
>  {
> - int pri;
> - char *p, *q, line[MAXLINE + 4 + 1];  /* message, encoding, NUL */
> + int pri, possiblepri;

A shorter name would be nice. pos_pri, or something like that.

> + char *p, *q, line[MAXLINE + 4 + 1], *gt;  /* message, encoding, NUL */
> + const char *errstr;
>  
>   /* test for special codes */
>   pri = DEFUPRI;
>   p = msg;
>   if (*p == '<') {
> - pri = 0;
> - while (isdigit((unsigned char)*++p))
> - pri = 10 * pri + (*p - '0');
> - if (*p == '>')
> - ++p;
> + gt = strchr(p, '>');
> + if (gt) {

if (gt != NULL) {

> + *gt = '\0';
> + possiblepri = strtonum(p + 1, 0, INT_MAX, );
> + if (!errstr)

if (errstr == NULL)

> + pri = possiblepri;
> + *gt = '>';
> + p = gt + 1;
> + }

This block modifies msg. It seems that the existing code doesn't. Are
you sure that won't have consequences elsewhere?

>   }
>   if (pri &~ (LOG_FACMASK|LOG_PRIMASK))
>   pri = DEFUPRI;
> @@ -1372,8 +1377,9 @@ printline(char *hname, char *msg)
>  void
>  printsys(char *msg)
>  {
> - int c, pri, flags;
> - char *lp, *p, *q, line[MAXLINE + 1];
> + int c, pri, flags, possiblepri;
> + char *lp, *p, *q, line[MAXLINE + 1], *gt;
> + const char *errstr;
>  
>   (void)snprintf(line, sizeof line, "%s: ", _PATH_UNIX);
>   lp = line + strlen(line);
> @@ -1381,11 +1387,15 @@ printsys(char *msg)
>   flags = SYNC_FILE | ADDDATE;/* fsync file after write */
>   pri = DEFSPRI;
>   if (*p == '<') {
> - pri = 0;
> - while (isdigit((unsigned char)*++p))
> - pri = 10 * pri + (*p - '0');
> - if (*p == '>')
> - ++p;
> + gt = strchr(p, '>');
> + if (gt) {
> + *gt = '\0';
> + possiblepri = strtonum(p + 1, 0, INT_MAX, 
> );
> + if (!errstr)
> + pri = possiblepri;
> + *gt = '>';
> + p = gt + 1;
> + }

Same as above. Also, maybe this should be broken out into a helper
function?

Maybe more robust logic would look something like this:

size_t nlen;
char nstr[11];

if (*p++ == '<') {
nlen = strspn(p, "0123456789")
if (nlen > 0 && nlen < 11 && p[nlen] == '>') {
strlcpy(nstr, p, sizeof(nstr));
[strtonum logic here]
}
}



Re: df(1) - unneeded headers

2016-02-08 Thread Michael McConville
Michal Mazurek wrote:
> This was ok tb@ but never commited.
> 
> Remove uneeded includes, and sort those that can be sorted (some can't):

Committed (mostly). Thanks!

I left stdlib.h and unistd.h because I think that C99 requires that
those be included when using libc, although failing to do so is
non-fatal. You'll see warnings for this if you use a newer compiler and
build with -Wall.

> Index: df.c
> ===
> RCS file: /cvs/src/bin/df/df.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 df.c
> --- df.c  9 Oct 2015 01:37:06 -   1.54
> +++ df.c  1 Feb 2016 09:43:38 -
> @@ -41,7 +41,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> Index: ext2fs_df.c
> ===
> RCS file: /cvs/src/bin/df/ext2fs_df.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 ext2fs_df.c
> --- ext2fs_df.c   27 Nov 2015 13:49:41 -  1.14
> +++ ext2fs_df.c   1 Feb 2016 09:43:38 -
> @@ -42,11 +42,8 @@
>  #include 
>  #include 
>  #include 
> -#include 
> -#include 
> +
>  #include 
> -#include 
> -#include 
>  
>  int  e2fs_df(int, char *, struct statfs *);
>  
> Index: ffs_df.c
> ===
> RCS file: /cvs/src/bin/df/ffs_df.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 ffs_df.c
> --- ffs_df.c  27 Nov 2015 13:49:41 -  1.17
> +++ ffs_df.c  1 Feb 2016 09:43:38 -
> @@ -36,13 +36,10 @@
>  
>  #include 
>  #include 
> -#include 
>  #include 
> +#include 
>  
> -#include 
> -#include 
>  #include 
> -#include 
>  
>  int  ffs_df(int, char *, struct statfs *);
> 
> -- 
> Michal Mazurek
> 



Re: [patch] vi enable -pedantic

2016-02-08 Thread Michael McConville
Jonathan Gray wrote:
> I don't think we should enable -pedantic anywhere in the tree.
> Different versions of gcc are going to have different ideas of what
> pedantic is.

This was my reaction too. I like the approach of keeping few to no
warnings in default builds. It's easy to "env CFLAGS='-Wextra -pedantic'
make" when you're auditing.

> I'm not sold on the value of all these patches to nvi when it is
> possibly hindering people who may be looking to add utf-8 support (via
> nvi2 or otherwise).

I think that vi is enough of a mess to warrant this. Most of these
patches are getting accepted upstream, too. I'm no longer actively
upstreaming, but I think Martijn and Michael Reed are.



Re: iwn firmware error

2016-02-07 Thread Michael McConville
Stefan Sperling wrote:
> On Fri, Feb 05, 2016 at 01:23:18AM -0500, Michael McConville wrote:
> > When forcing my laptop to swap, I almost immediately got the following
> > firmware error. I'm not sure whether this is expected. Restarting the
> > interface brought things back to normal.
> > 
> > dmesg follows.
> 
> Looks like the firmware crashed because an interrupt could not be
> serviced by the host on time, perhaps?

I suspect this was the case. My machine was almost completely
non-responsive.

> > Feb  5 01:19:04 thinkpad /bsd: iwn0: fatal firmware error
> > Feb  5 01:19:04 thinkpad /bsd: firmware error log:
> > Feb  5 01:19:04 thinkpad /bsd:   error type  = "NMI_INTERRUPT_WDG" 
> > (0x0004)
> > Feb  5 01:19:04 thinkpad /bsd:   program counter = 0x06B4
> > Feb  5 01:19:04 thinkpad /bsd:   source line = 0xD3EA
> > Feb  5 01:19:04 thinkpad /bsd:   error data  = 0x00020263
> > Feb  5 01:19:04 thinkpad /bsd:   branch link = 0x067A071A
> > Feb  5 01:19:04 thinkpad /bsd:   interrupt link  = 0x1532E762
> > Feb  5 01:19:04 thinkpad /bsd:   time= 1346051188
> > Feb  5 01:19:04 thinkpad /bsd: driver status:
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring  0: qid=0  cur=188 queued=135
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring  1: qid=1  cur=0   queued=0  
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring  2: qid=2  cur=0   queued=0  
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring  3: qid=3  cur=0   queued=0  
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring  4: qid=4  cur=135 queued=0  
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring  5: qid=5  cur=0   queued=0  
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring  6: qid=6  cur=0   queued=0  
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring  7: qid=7  cur=0   queued=0  
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring  8: qid=8  cur=0   queued=0  
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring  9: qid=9  cur=0   queued=0  
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring 10: qid=10 cur=0   queued=0  
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring 11: qid=11 cur=0   queued=0  
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring 12: qid=12 cur=0   queued=0  
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring 13: qid=13 cur=0   queued=0  
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring 14: qid=14 cur=0   queued=0  
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring 15: qid=15 cur=0   queued=0  
> > Feb  5 01:19:04 thinkpad /bsd:   tx ring 16: qid=16 cur=0   queued=0  
> > Feb  5 01:19:05 thinkpad /bsd:   tx ring 17: qid=17 cur=0   queued=0  
> > Feb  5 01:19:06 thinkpad /bsd:   tx ring 18: qid=18 cur=0   queued=0  
> > Feb  5 01:19:06 thinkpad /bsd:   tx ring 19: qid=19 cur=0   queued=0  
> > Feb  5 01:19:06 thinkpad /bsd:   rx ring: cur=47
> > Feb  5 01:19:06 thinkpad /bsd:   802.11 state 4



Re: pkg_add.1

2016-02-07 Thread Michael McConville
joshua stein wrote:
> We don't recommend FTP mirrors anymore, installing a package via a
> pipe doesn't seem to work anymore, and packages have to be signed to
> be installed so the advice about miscreants is not very relevant.

Good catch with the FTP link.

I think it's still worth mentioning that you put trust in the packages
you install. Although the package tarballs themselves are now signed (by
default), the porter or software author could still try to slip
something in.

> Index: pkg_add.1
> ===
> RCS file: /var/cvsync/src/usr.sbin/pkg_add/pkg_add.1,v
> retrieving revision 1.134
> diff -u -p -u -p -r1.134 pkg_add.1
> --- pkg_add.1 4 Nov 2015 16:59:58 -   1.134
> +++ pkg_add.1 20 Jan 2016 21:06:53 -
> @@ -198,41 +198,6 @@ dependencies with the list of packages l
>  user's opinion in interactive mode,
>  then install default packages that satisfy the dependencies.
>  .Pp
> -Alternatively, it is possible to add packages interactively from within the
> -.Xr ftp 1
> -client,
> -in which case setting
> -.Ev PKG_PATH
> -correctly will be necessary for any dependency to be found out and retrieved
> -the same way.
> -For example, the following works:
> -.Bd -literal -offset indent
> -$ ftp ftp://ftp.openbsd.org/pub/OpenBSD/2.7/packages/i386/
> -250 CWD command successful
> -ftp> ls m*
> -227 Entering Passive Mode (129,128,5,191,164,73)
> -150 Opening ASCII mode data connection for m*.
> -m4-1.4.tgz
> -metamail-2.7.tgz
> -mh-6.8.4.tgz
> -mm-1.0.12.tgz
> -mpeg_lib-1.2.1.tgz
> -mpeg_play-2.4.tgz
> -mpg123-0.59q.tgz
> -mutt-0.95.7i.tgz
> -226 Transfer complete.
> -ftp> get m4-1.4.tgz "|pkg_add -v -"
> -.Ed
> -.Pp
> -.Sy Warning:
> -Since the
> -.Nm
> -command may execute scripts or programs contained within a package file,
> -your system may be susceptible to
> -.Dq trojan horses
> -or other subtle attacks from miscreants who create dangerous packages.
> -Be sure the specified package(s) are from trusted sources.
> -.Pp
>  The options are as follows:
>  .Bl -tag -width keyword
>  .It Fl A Ar arch
> 



tcpdump CDP crashes

2016-02-06 Thread Michael McConville
Can E. Acar wrote:
> The crashes you posted to icb recently
> (https://www.sccs.swarthmore.edu/users/16/mmcconv1/dump/tcpdump-crashes/)
> are related to print-cdp.c where the bounds checks are missing/broken.
> 
> The attached diff seems to fix the issues.

canacar@ asked me to forward this to the list. It makes sense to me, but
it needs review by someone with more knowledge of tcpdump's woeful ways
of buffer handling.


Index: print-cdp.c
===
RCS file: /cvs/src/usr.sbin/tcpdump/print-cdp.c,v
retrieving revision 1.5
diff -u -p -u -p -r1.5 print-cdp.c
--- print-cdp.c 16 Jan 2015 06:40:21 -  1.5
+++ print-cdp.c 28 Jan 2016 05:22:51 -
@@ -65,7 +65,7 @@ cdp_print(const u_char *p, u_int length,
 
while (i < length) {
if (i + 4 > caplen) {
-   printf("[!cdp]");
+   printf("[|cdp]");
return;
}
 
@@ -75,8 +75,11 @@ cdp_print(const u_char *p, u_int length,
if (vflag)
printf(" %02x/%02x", type, len);
 
+   if (len < 4)
+   goto error;
+
if (i+len > caplen) {
-   printf("[!cdp]");
+   printf("[|cdp]");
return;
}
 
@@ -92,6 +95,8 @@ cdp_print(const u_char *p, u_int length,
printf(" PortID '%.*s'", len - 4, p + i + 4);
break;
case 0x04:
+   if (len < 7)
+   goto error;
printf(" CAP 0x%02x", (unsigned) p[i+7]);
break;
case 0x05:
@@ -110,6 +115,8 @@ cdp_print(const u_char *p, u_int length,
printf(" VTP-Management-Domain '%.*s'", len-4, p+i+4 );
break;
case 0x0a:  /* guess - not documented */
+   if (len < 5)
+   goto error;
printf(" Native-VLAN-ID %d", (p[i+4]<<8) + p[i+4+1] - 1 
);
break;
case 0x0b:  /* guess - not documented */
@@ -124,6 +131,9 @@ cdp_print(const u_char *p, u_int length,
break;
i += len;
}
+   return;
+error:
+   printf("[!cdp]");
 }
 
 void
@@ -137,13 +147,20 @@ cdp_print_addr(const u_char * p, int l)
 
printf(" (%d): ", num);
 
-   while(p < endp && num >= 0) {
+   while(num >= 0) {
+   if (p + 1 >= endp)
+   break;
pl=*(p+1);
p+=2;
 
+   if (p + l + 2 >= endp)
+   break;
+
/* special case: IPv4, protocol type=0xcc, addr. length=4 */
if (pl == 1 && *p == 0xcc && p[1] == 0 && p[2] == 4) {
p+=3;
+   if (p + 4 >= endp)
+   break;
 
printf("IPv4 %d.%d.%d.%d ", p[0], p[1], p[2], p[3]);
p+=4;
@@ -154,6 +171,8 @@ cdp_print_addr(const u_char * p, int l)
al=(*p << 8) + *(p+1);
printf(", al=%d, a=", al);
p+=2;
+   if (p + al >= endp)
+   break;
while(al-- > 0)
printf(" %02x", *p++);
}
@@ -169,7 +188,8 @@ cdp_print_prefixes(const u_char * p, int
printf(" IPv4 Prefixes (%d):", l/5);
 
while (l > 0) {
-   printf(" %d.%d.%d.%d/%d", p[0], p[1], p[2], p[3], p[4] );
+   if (l >= 5)
+   printf(" %d.%d.%d.%d/%d", p[0], p[1], p[2], p[3], p[4] 
);
l-=5; p+=5;
}
 }



Re: [patch] kern/exec_script: return error when the shell name is not specified

2016-02-06 Thread Michael McConville
Mark Kettenis wrote:
> > Date: Fri, 5 Feb 2016 13:06:56 -0500
> > From: Michael McConville <mm...@mykolab.com>
> > 
> > Michael McConville wrote:
> > > Michael McConville wrote:
> > > > Maxim Pugachev wrote:
> > > > > In a case when the shell name is not specified (i.e. just "#!" without
> > > > > a path), don't run the heavy logic that checks shell, simply return
> > > > > ENOENT.
> > > > 
> > > > I'm not sure whether this is a reasonable thing to do. Someone with more
> > > > kernel API experience will have to comment.
> > > > 
> > > > > Also, as a tiny improvement: avoid a loop when calculating shell's
> > > > > args length.
> > > > 
> > > > It seems like there's a simpler solution to this: strlen. Also, the
> > > > assignment that follows the for loop seems dead, as we know *cp will be
> > > > NUL.
> > > > 
> > > > The below diff makes that change, removes the useless if condition you
> > > > noticed, and bumps a few variable initializations into the declaration
> > > > block.
> > > 
> > > I just remembered that this was probably a perfomance concern as well.
> > > In that case, we could assign shellarg and shellarglen earlier in the
> > > function, right?
> > 
> > Is anyone willing to work with me on this? This cleanup should obviously
> > be done very carefully, but I think it's worth doing because this code
> > is... unfortunate. I'd be happy to submit and review in smaller chunks.
> 
> What bug is this fixing?  I'm really not interested in diffs that are
> basically just churn.

No bug. It just seems unfortunate to have code like this in sys/kern.
Dead ops, manually inlined strlen() calls, etc. Not a big deal, though.
I'll drop it.



Re: dhclient.c patch

2016-02-04 Thread Michael McConville
Edgar Pettijohn wrote:
> --- dhclient.c.origThu Feb  4 17:57:41 2016
> +++ dhclient.cThu Feb  4 17:57:55 2016
> @@ -57,7 +57,6 @@
>  #include "privsep.h"
> 
>  #include 
> -#include 
>  #include 
>  #include 
> 
>  is brought in through dhcpd.h

It'd probably be better to add  to all source files that
need it and remove it from dhcpd.h. We've been moving toward including
everything directly where it's needed. This makes builds faster and
makes files more portable. ksh(1) and less(1) have been partially
converted.



Re: Simplify tcpdump

2016-02-04 Thread Michael McConville
Michael McConville wrote:
> Stuart Henderson wrote:
> > On 2016/01/26 20:16, Michael McConville wrote:
> > > Stuart Henderson wrote:
> > > > On 2016/01/25 22:52, Michael McConville wrote:
> > > > > fddi_bitswap is only used once, and it just adds a layer of
> > > > > indirection to its preprocessor condition.
> > > > 
> > > > Oh yuk. This is bogus anyway, and there's no good way to handle it. We
> > > > dropped support for FDDI interfaces so it only affect decodes of pcap
> > > > files, and who knows where they were created?
> > > 
> > > I'm not familiar with FDDI and I'm new to tcpdump, so I can't offer much
> > > input. How much do you think can/should be removed?
> > 
> > FDDI is a dual token-ring network based on 100Mb fibre connections,
> > often over larger distances (campus/metro) than a typical lan. It's
> > obsolete, we removed support for the adapters, the only place this
> > code could possibly be used now is for parsing pcap files captured
> > on another system or from older OpenBSD (and there's a kitchen-sink
> > pcap decoder in ports these days..).
> > 
> > This bitswap thing is because some OS bitswap the network addresses
> > (in the driver or somewhere; they are swapped in pcap files) and some
> > don't.
> > 
> > I don't think that it's particularly useful for OpenBSD to support
> > decoding this any more. Maybe we should stop rearranging these
> > deckchairs and borrow tedu's axe instead.
> 
> Something like this? There's a lot to be removed from libpcap too.

Ping. Does anyone else think this is a good idea? Would anyone miss FDDI
support?


> Index: INSTALL
> ===
> RCS file: /cvs/src/usr.sbin/tcpdump/INSTALL,v
> retrieving revision 1.6
> diff -u -p -r1.6 INSTALL
> --- INSTALL   5 Dec 2015 21:43:51 -   1.6
> +++ INSTALL   27 Jan 2016 01:49:10 -
> @@ -15,7 +15,6 @@ bpf_dump.c  - bpf instruction pretty-prin
>  decnet.h - DECnet definitions
>  ethertype.h  - ethernet definitions
>  extract.h- alignment definitions
> -fddi.h   - Fiber Distributed Data Interface definitions
>  gmt2local.c  - time conversion routines
>  gmt2local.h  - time conversion prototypes
>  igrp.h   - Interior Gateway Routing Protocol definitions
> @@ -43,7 +42,6 @@ print-decnet.c  - DECnet printer routines
>  print-domain.c   - Domain Name System printer routines
>  print-enc.c  - Encapsulated printer routines
>  print-ether.c- ethernet printer routines
> -print-fddi.c - Fiber Distributed Data Interface printer routines
>  print-gre.c  - Generic Routing Encapsulation printer routines
>  print-icmp.c - Internet Control Message Protocol printer routines
>  print-igrp.c - Interior Gateway Routing Protocol printer routines
> Index: Makefile
> ===
> RCS file: /cvs/src/usr.sbin/tcpdump/Makefile,v
> retrieving revision 1.59
> diff -u -p -r1.59 Makefile
> --- Makefile  14 Oct 2015 04:55:17 -  1.59
> +++ Makefile  27 Jan 2016 01:49:10 -
> @@ -28,7 +28,7 @@ CFLAGS+=-Wall -I${.CURDIR}/../../sbin/pf
>  # for pcap-int.h
>  CFLAGS+=-I${.CURDIR}/../../lib/libpcap
>  
> -CFLAGS+=-DCSLIP -DPPP -DHAVE_FDDI -DETHER_SERVICE -DHAVE_NET_SLIP_H 
> -DHAVE_ETHER_NTOHOST -DINET6
> +CFLAGS+=-DCSLIP -DPPP -DETHER_SERVICE -DHAVE_NET_SLIP_H -DHAVE_ETHER_NTOHOST 
> -DINET6
>  
>  LDADD+=  -lpcap -ll -lcrypto
>  DPADD+=  ${LIBL} ${LIBPCAP} ${LIBCRYPTO}
> @@ -38,7 +38,7 @@ SRCS=   tcpdump.c addrtoname.c privsep.c p
>   print-atalk.c print-domain.c print-tftp.c print-bootp.c print-nfs.c \
>   print-icmp.c print-sl.c print-ppp.c print-rip.c print-timed.c \
>   print-snmp.c print-ntp.c print-null.c print-ospf.c print-gtp.c \
> - print-fddi.c print-llc.c print-sunrpc.c print-hsrp.c print-vqp.c \
> + print-llc.c print-sunrpc.c print-hsrp.c print-vqp.c \
>   print-vrrp.c print-wb.c print-decnet.c print-isoclns.c print-ipx.c \
>   print-atm.c print-dvmrp.c print-krb.c print-pim.c print-netbios.c \
>   util.c bpf_dump.c parsenfsfh.c version.c print-igrp.c \
> Index: fddi.h
> ===
> RCS file: fddi.h
> diff -N fddi.h
> --- fddi.h7 Oct 2007 16:41:05 -   1.7
> +++ /dev/null 1 Jan 1970 00:00:00 -
> @@ -1,71 +0,0 @@
> -/*   $OpenBSD: fddi.h,v 1.7 2007/10/07 16:41:05 deraadt Exp $*/
> -
> -/*
> - * Copyright (c) 1992, 1993, 1994, 1995, 1996
> - *   The Regents of the University of California.  All rights reserved.
> - *
> -

Nop null checks in ti(4)

2016-02-04 Thread Michael McConville
Clang 3.7 warns about this. The rings are arrays, not pointers, so they
can't be NULL.

ok? Or should these checks be replaced rather than removed?


Index: dev/ic/ti.c
===
RCS file: /cvs/src/sys/dev/ic/ti.c,v
retrieving revision 1.22
diff -u -p -r1.22 ti.c
--- dev/ic/ti.c 25 Nov 2015 03:09:58 -  1.22
+++ dev/ic/ti.c 5 Feb 2016 05:34:14 -
@@ -484,9 +484,6 @@ ti_handle_events(struct ti_softc *sc)
struct ti_event_desc*e;
struct ifnet*ifp = >arpcom.ac_if;
 
-   if (sc->ti_rdata->ti_event_ring == NULL)
-   return;
-
while (sc->ti_ev_saved_considx != sc->ti_ev_prodidx.ti_idx) {
e = >ti_rdata->ti_event_ring[sc->ti_ev_saved_considx];
switch (TI_EVENT_EVENT(e)) {
@@ -846,9 +843,6 @@ ti_free_tx_ring(struct ti_softc *sc)
 {
int i;
struct ti_txmap_entry *entry;
-
-   if (sc->ti_rdata->ti_tx_ring == NULL)
-   return;
 
for (i = 0; i < TI_TX_RING_CNT; i++) {
if (sc->ti_cdata.ti_tx_chain[i] != NULL) {



iwn firmware error

2016-02-04 Thread Michael McConville
When forcing my laptop to swap, I almost immediately got the following
firmware error. I'm not sure whether this is expected. Restarting the
interface brought things back to normal.

dmesg follows.

Feb  5 01:19:04 thinkpad /bsd: iwn0: fatal firmware error
Feb  5 01:19:04 thinkpad /bsd: firmware error log:
Feb  5 01:19:04 thinkpad /bsd:   error type  = "NMI_INTERRUPT_WDG" 
(0x0004)
Feb  5 01:19:04 thinkpad /bsd:   program counter = 0x06B4
Feb  5 01:19:04 thinkpad /bsd:   source line = 0xD3EA
Feb  5 01:19:04 thinkpad /bsd:   error data  = 0x00020263
Feb  5 01:19:04 thinkpad /bsd:   branch link = 0x067A071A
Feb  5 01:19:04 thinkpad /bsd:   interrupt link  = 0x1532E762
Feb  5 01:19:04 thinkpad /bsd:   time= 1346051188
Feb  5 01:19:04 thinkpad /bsd: driver status:
Feb  5 01:19:04 thinkpad /bsd:   tx ring  0: qid=0  cur=188 queued=135
Feb  5 01:19:04 thinkpad /bsd:   tx ring  1: qid=1  cur=0   queued=0  
Feb  5 01:19:04 thinkpad /bsd:   tx ring  2: qid=2  cur=0   queued=0  
Feb  5 01:19:04 thinkpad /bsd:   tx ring  3: qid=3  cur=0   queued=0  
Feb  5 01:19:04 thinkpad /bsd:   tx ring  4: qid=4  cur=135 queued=0  
Feb  5 01:19:04 thinkpad /bsd:   tx ring  5: qid=5  cur=0   queued=0  
Feb  5 01:19:04 thinkpad /bsd:   tx ring  6: qid=6  cur=0   queued=0  
Feb  5 01:19:04 thinkpad /bsd:   tx ring  7: qid=7  cur=0   queued=0  
Feb  5 01:19:04 thinkpad /bsd:   tx ring  8: qid=8  cur=0   queued=0  
Feb  5 01:19:04 thinkpad /bsd:   tx ring  9: qid=9  cur=0   queued=0  
Feb  5 01:19:04 thinkpad /bsd:   tx ring 10: qid=10 cur=0   queued=0  
Feb  5 01:19:04 thinkpad /bsd:   tx ring 11: qid=11 cur=0   queued=0  
Feb  5 01:19:04 thinkpad /bsd:   tx ring 12: qid=12 cur=0   queued=0  
Feb  5 01:19:04 thinkpad /bsd:   tx ring 13: qid=13 cur=0   queued=0  
Feb  5 01:19:04 thinkpad /bsd:   tx ring 14: qid=14 cur=0   queued=0  
Feb  5 01:19:04 thinkpad /bsd:   tx ring 15: qid=15 cur=0   queued=0  
Feb  5 01:19:04 thinkpad /bsd:   tx ring 16: qid=16 cur=0   queued=0  
Feb  5 01:19:05 thinkpad /bsd:   tx ring 17: qid=17 cur=0   queued=0  
Feb  5 01:19:06 thinkpad /bsd:   tx ring 18: qid=18 cur=0   queued=0  
Feb  5 01:19:06 thinkpad /bsd:   tx ring 19: qid=19 cur=0   queued=0  
Feb  5 01:19:06 thinkpad /bsd:   rx ring: cur=47
Feb  5 01:19:06 thinkpad /bsd:   802.11 state 4



OpenBSD 5.9 (GENERIC.MP) #0: Fri Feb  5 00:38:43 EST 2016
mike@thinkpad:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 4062691328 (3874MB)
avail mem = 3933769728 (3751MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.6 @ 0xe0010 (78 entries)
bios0: vendor LENOVO version "6QET61WW (1.31 )" date 10/26/2010
bios0: LENOVO 3626FAU
acpi0 at bios0: rev 2
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP SSDT ECDT APIC MCFG HPET ASF! SLIC BOOT SSDT TCPA SSDT 
SSDT SSDT
acpi0: wakeup devices LID_(S3) SLPB(S3) IGBE(S4) EXP1(S4) EXP2(S4) EXP3(S4) 
EXP4(S4) EXP5(S4) EHC1(S3) EHC2(S3) HDEF(S4)
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpiec0 at acpi0
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Core(TM) i5 CPU M 540 @ 2.53GHz, 2793.51 MHz
cpu0: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,POPCNT,AES,NXE,LONG,LAHF,PERF,ITSC,SENSOR,ARAT
cpu0: 256KB 64b/line 8-way L2 cache
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges
cpu0: apic clock running at 132MHz
cpu0: mwait min=64, max=64, C-substates=0.2.1.1, IBE
cpu1 at mainbus0: apid 1 (application processor)
cpu1: Intel(R) Core(TM) i5 CPU M 540 @ 2.53GHz, 2793.00 MHz
cpu1: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,POPCNT,AES,NXE,LONG,LAHF,PERF,ITSC,SENSOR,ARAT
cpu1: 256KB 64b/line 8-way L2 cache
cpu1: smt 1, core 0, package 0
cpu2 at mainbus0: apid 4 (application processor)
cpu2: Intel(R) Core(TM) i5 CPU M 540 @ 2.53GHz, 2793.00 MHz
cpu2: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,POPCNT,AES,NXE,LONG,LAHF,PERF,ITSC,SENSOR,ARAT
cpu2: 256KB 64b/line 8-way L2 cache
cpu2: smt 0, core 2, package 0
cpu3 at mainbus0: apid 5 (application processor)
cpu3: Intel(R) Core(TM) i5 CPU M 540 @ 2.53GHz, 2793.00 MHz
cpu3: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,POPCNT,AES,NXE,LONG,LAHF,PERF,ITSC,SENSOR,ARAT
cpu3: 256KB 64b/line 8-way L2 cache
cpu3: smt 1, core 2, 

Re: [PATCH] uname, arch/machine -> %c, %a update in PKG_PATH

2016-02-03 Thread Michael McConville
Stuart Henderson wrote:
> > >>  Raf Czlonka wrote:
> > >> - ftp.openbsd.org is, AFAIC, overloaded
> 
> Whenever I've checked speeds from ftp.openbsd.org they have been
> fairly consistent, this isn't the usual expected behaviour of an
> overloaded machine. (not super fast, but they have been consistent).

At the very least, it seems like there are concerns about their
operating costs.



Re: [PATCH] uname, arch/machine -> %c, %a update in PKG_PATH

2016-02-02 Thread Michael McConville
Raf Czlonka wrote:
> - ftp.openbsd.org is, AFAIC, overloaded

I haven't been following this thread fully, but I agree that
ftp.openbsd.org shouldn't be used in examples. Many many people use the
default mirror whenever possible.



Re: Some df(1) cleanup

2016-02-01 Thread Michael McConville
Theo Buehler wrote:
> On Mon, Feb 01, 2016 at 05:17:03PM +0100, Michal Mazurek wrote:
> > Meaningful error messages:
> >
> > mntbuf = calloc(argc, sizeof(struct statfs));
> > if (mntbuf == NULL)
> > -   err(1, NULL);
> > +   err(1, "calloc");
> 
> I disagree with the changes in this patch.  If either malloc or calloc
> fails it will typically set errno to ENOMEM, so you'll
> 
> df: Cannot allocate memory
> 
> I don't think adding the information that it was malloc or calloc that
> failed is helpful at all.

Seconded.

IIUC, the only cause of an error other than ENOMEM is a bug in the
allocator. This will definitely or almost definitely happen in a part of
the allocator that is agnostic to the function called (malloc, calloc,
etc.). So printing the function name is just noise.



Re: less.h small cleanup

2016-01-29 Thread Michael McConville
Michael Reed wrote:
> - sorts includes + remove unneeded comment
> - less.h 1.24[1] removes the only use of CHAR_BIT, so remove it

Committed. Thanks!

> - remove SHELL_META_QUEST, doesn't seem to be used either

I'm going to leave this for now because I don't know what it is/was.



Replace less(1)'s stdbool clone with the real McCoy

2016-01-29 Thread Michael McConville
Does this make sense?

Note that screen_crashed can also be 2, so we leave it as an int.
Assigning it to TRUE seems to have been a mistake.

No binary change, surprisingly.


Index: ch.c
===
RCS file: /cvs/src/usr.bin/less/ch.c,v
retrieving revision 1.16
diff -u -p -r1.16 ch.c
--- ch.c27 Dec 2015 17:51:19 -  1.16
+++ ch.c29 Jan 2016 16:29:30 -
@@ -141,7 +141,7 @@ ch_get(void)
struct buf *bp;
struct bufnode *bn;
int n;
-   int slept;
+   bool slept;
int h;
off_t pos;
off_t len;
@@ -159,7 +159,7 @@ ch_get(void)
return (bp->data[ch_offset]);
}
 
-   slept = FALSE;
+   slept = false;
 
/*
 * Look for a buffer holding the desired block.
@@ -282,7 +282,7 @@ read_more:
ierror("%s", );
}
sleep(1);
-   slept = TRUE;
+   slept = true;
 
if (follow_mode == FOLLOW_NAME) {
/*
@@ -352,12 +352,12 @@ ch_ungetchar(int c)
 void
 end_logfile(void)
 {
-   static int tried = FALSE;
+   static bool tried = false;
 
if (logfile < 0)
return;
if (!tried && ch_fsize == -1) {
-   tried = TRUE;
+   tried = true;
ierror("Finishing logfile", NULL);
while (ch_forw_get() != EOI)
if (ABORT_SIGS())
@@ -378,25 +378,25 @@ sync_logfile(void)
 {
struct buf *bp;
struct bufnode *bn;
-   int warned = FALSE;
+   bool warned = false;
BLOCKNUM block;
BLOCKNUM nblocks;
 
nblocks = (ch_fpos + LBUFSIZE - 1) / LBUFSIZE;
for (block = 0;  block < nblocks;  block++) {
-   int wrote = FALSE;
+   bool wrote = false;
FOR_BUFS(bn) {
bp = bufnode_buf(bn);
if (bp->block == block) {
(void) write(logfile, (char *)bp->data,
bp->datasize);
-   wrote = TRUE;
+   wrote = true;
break;
}
}
if (!wrote && !warned) {
error("Warning: log file is incomplete", NULL);
-   warned = TRUE;
+   warned = true;
}
}
 }
@@ -404,7 +404,7 @@ sync_logfile(void)
 /*
  * Determine if a specific block is currently in one of the buffers.
  */
-static int
+static bool
 buffered(BLOCKNUM block)
 {
struct buf *bp;
@@ -415,9 +415,9 @@ buffered(BLOCKNUM block)
FOR_BUFS_IN_CHAIN(h, bn) {
bp = bufnode_buf(bn);
if (bp->block == block)
-   return (TRUE);
+   return true;
}
-   return (FALSE);
+   return false;
 }
 
 /*
@@ -785,7 +785,7 @@ ch_init(int f, int flags)
 void
 ch_close(void)
 {
-   int keepstate = FALSE;
+   bool keepstate = false;
 
if (thisfile == NULL)
return;
@@ -796,7 +796,7 @@ ch_close(void)
 */
ch_delbufs();
} else {
-   keepstate = TRUE;
+   keepstate = true;
}
if (!(ch_flags & CH_KEEPOPEN)) {
/*
@@ -809,7 +809,7 @@ ch_close(void)
close(ch_file);
ch_file = -1;
} else {
-   keepstate = TRUE;
+   keepstate = true;
}
if (!keepstate) {
/*
Index: command.c
===
RCS file: /cvs/src/usr.bin/less/command.c,v
retrieving revision 1.28
diff -u -p -r1.28 command.c
--- command.c   12 Jan 2016 17:48:04 -  1.28
+++ command.c   29 Jan 2016 16:29:30 -
@@ -20,7 +20,7 @@
 
 extern int erase_char, erase2_char, kill_char;
 extern volatile sig_atomic_t sigs;
-extern int quit_if_one_screen;
+extern bool quit_if_one_screen;
 extern int less_is_more;
 extern int squished;
 extern int sc_width;
@@ -270,7 +270,7 @@ mca_opt_first_char(int c)
switch (c) {
case '_':
/* "__" = long option name. */
-   optgetname = TRUE;
+   optgetname = true;
mca_opt_toggle();
return (MCA_MORE);
}
@@ -292,7 +292,7 @@ mca_opt_first_char(int c)
return (MCA_MORE);
case '-':
/* "--" = long option name. */
-   optgetname = TRUE;
+   optgetname = true;
mca_opt_toggle();
return (MCA_MORE);

Re: _PATH_SENDMAIL in lots of places outside of lpd stuff also

2016-01-29 Thread Michael McConville
Chris Bennett wrote:
> I found a subroutine in printjob.c called sendmail with uses
> _PATH_SENDMAIL.
> 
> I found it all over the place:

Are you implying that they should be replaced? IIUC, we create a
sendmail binary (or at least a link) even though we no longer
technically use sendmail. See usr.sbin/mailwrapper.

That said, _PATH_SENDMAIL could be deprecated for other reasons. I'm
just guessing at what you meant.

> blue src # ack _PATH_SENDMAIL  
> include/paths.h
> 63:#define  _PATH_SENDMAIL  "/usr/sbin/sendmail"
> 
> usr.bin/calendar/io.c
> 410:execl(_PATH_SENDMAIL, "sendmail", "-i", "-t", "-F",
> 412:warn(_PATH_SENDMAIL);
> 
> usr.bin/mail/send.c
> 422:cp = _PATH_SENDMAIL;
> 
> usr.bin/rdist/docmd.c
> 137:   _PATH_SENDMAIL);
> 140:error("notify: \"%s\" failed\n", _PATH_SENDMAIL);
> 
> usr.bin/sendbug/sendbug.c
> 339:execl(_PATH_SENDMAIL, "sendmail",
> 
> usr.bin/skeyaudit/skeyaudit.c
> 215:execl(_PATH_SENDMAIL, "sendmail", "-t", (char *)NULL);
> 216:warn("cannot run \"%s -t\"", _PATH_SENDMAIL);
> 
> usr.bin/vacation/vacation.c
> 475:execl(_PATH_SENDMAIL, "sendmail", "-f", myname, "--",
> 477:syslog(LOG_ERR, "can't exec %s: %m", _PATH_SENDMAIL);
> 
> usr.bin/vi/common/recover.c
> 826:if (_PATH_SENDMAIL[0] != '/' || stat(_PATH_SENDMAIL, ))
> 828:_PATH_SENDMAIL, "not sending email: %s");
> 838:"%s -t < %s", _PATH_SENDMAIL, fname);
> 
> usr.sbin/cron/config.h
> 40:#define MAILARG _PATH_SENDMAIL   /*-*/
> 
> usr.sbin/lpr/lpd/printjob.c
> 1079:   if ((cp = strrchr(_PATH_SENDMAIL, '/')) != NULL)
> 1082:   cp = _PATH_SENDMAIL;
> 1083:   execl(_PATH_SENDMAIL, cp, "-t", (char *)NULL);
> 
> Chris
> 



Re: _PATH_SENDMAIL in lots of places outside of lpd stuff also

2016-01-29 Thread Michael McConville
Chris Bennett wrote:
> On Fri, Jan 29, 2016 at 09:18:14PM -0500, Michael McConville wrote:
> > Chris Bennett wrote:
> > > I found a subroutine in printjob.c called sendmail with uses
> > > _PATH_SENDMAIL.
> > > 
> > > I found it all over the place:
> > 
> > Are you implying that they should be replaced? IIUC, we create a
> > sendmail binary (or at least a link) even though we no longer
> > technically use sendmail. See usr.sbin/mailwrapper.
> > 
> > That said, _PATH_SENDMAIL could be deprecated for other reasons. I'm
> > just guessing at what you meant.
> > 
> 
> Well, sendmail is no longer in base.
> But sendmail is installable from ports.

Right, but we emulate it by using the same binary name. I'm not sure
whether there's a suggested alternative. It seems if there were an easy
replacement, the existing _PATH_SENDMAIL uses would have been removed.



Re: `ifstated -n' more useful error message

2016-01-28 Thread Michael McConville
Michael Reed wrote:
> If fopen("/etc/ifstated.conf", ...) fails for whatever reason the
> error message isn't very helpful:
> 
>   $ ifstated -n
>   ifstated: /etc/ifstated.conf
> 
> With this patch:
> 
>   $ ./ifstated -n
>   ifstated: /etc/ifstated.conf: No such file or directory

Committed. Thanks!

I also converted ~13 uses of this general form:

> errx(1, "parse_config malloc");

To this:

> err(1, NULL);



Re: Simplify tcpdump

2016-01-26 Thread Michael McConville
Stuart Henderson wrote:
> On 2016/01/25 22:52, Michael McConville wrote:
> > fddi_bitswap is only used once, and it just adds a layer of
> > indirection to its preprocessor condition.
> 
> Oh yuk. This is bogus anyway, and there's no good way to handle it. We
> dropped support for FDDI interfaces so it only affect decodes of pcap
> files, and who knows where they were created?

I'm not familiar with FDDI and I'm new to tcpdump, so I can't offer much
input. How much do you think can/should be removed?



Re: Simplify tcpdump

2016-01-26 Thread Michael McConville
Stuart Henderson wrote:
> On 2016/01/26 20:16, Michael McConville wrote:
> > Stuart Henderson wrote:
> > > On 2016/01/25 22:52, Michael McConville wrote:
> > > > fddi_bitswap is only used once, and it just adds a layer of
> > > > indirection to its preprocessor condition.
> > > 
> > > Oh yuk. This is bogus anyway, and there's no good way to handle it. We
> > > dropped support for FDDI interfaces so it only affect decodes of pcap
> > > files, and who knows where they were created?
> > 
> > I'm not familiar with FDDI and I'm new to tcpdump, so I can't offer much
> > input. How much do you think can/should be removed?
> 
> FDDI is a dual token-ring network based on 100Mb fibre connections,
> often over larger distances (campus/metro) than a typical lan. It's
> obsolete, we removed support for the adapters, the only place this
> code could possibly be used now is for parsing pcap files captured
> on another system or from older OpenBSD (and there's a kitchen-sink
> pcap decoder in ports these days..).
> 
> This bitswap thing is because some OS bitswap the network addresses
> (in the driver or somewhere; they are swapped in pcap files) and some
> don't.
> 
> I don't think that it's particularly useful for OpenBSD to support
> decoding this any more. Maybe we should stop rearranging these
> deckchairs and borrow tedu's axe instead.

Something like this? There's a lot to be removed from libpcap too.


Index: INSTALL
===
RCS file: /cvs/src/usr.sbin/tcpdump/INSTALL,v
retrieving revision 1.6
diff -u -p -r1.6 INSTALL
--- INSTALL 5 Dec 2015 21:43:51 -   1.6
+++ INSTALL 27 Jan 2016 01:49:10 -
@@ -15,7 +15,6 @@ bpf_dump.c- bpf instruction pretty-prin
 decnet.h   - DECnet definitions
 ethertype.h- ethernet definitions
 extract.h  - alignment definitions
-fddi.h - Fiber Distributed Data Interface definitions
 gmt2local.c- time conversion routines
 gmt2local.h- time conversion prototypes
 igrp.h - Interior Gateway Routing Protocol definitions
@@ -43,7 +42,6 @@ print-decnet.c- DECnet printer routines
 print-domain.c - Domain Name System printer routines
 print-enc.c- Encapsulated printer routines
 print-ether.c  - ethernet printer routines
-print-fddi.c   - Fiber Distributed Data Interface printer routines
 print-gre.c- Generic Routing Encapsulation printer routines
 print-icmp.c   - Internet Control Message Protocol printer routines
 print-igrp.c   - Interior Gateway Routing Protocol printer routines
Index: Makefile
===
RCS file: /cvs/src/usr.sbin/tcpdump/Makefile,v
retrieving revision 1.59
diff -u -p -r1.59 Makefile
--- Makefile14 Oct 2015 04:55:17 -  1.59
+++ Makefile27 Jan 2016 01:49:10 -
@@ -28,7 +28,7 @@ CFLAGS+=-Wall -I${.CURDIR}/../../sbin/pf
 # for pcap-int.h
 CFLAGS+=-I${.CURDIR}/../../lib/libpcap
 
-CFLAGS+=-DCSLIP -DPPP -DHAVE_FDDI -DETHER_SERVICE -DHAVE_NET_SLIP_H 
-DHAVE_ETHER_NTOHOST -DINET6
+CFLAGS+=-DCSLIP -DPPP -DETHER_SERVICE -DHAVE_NET_SLIP_H -DHAVE_ETHER_NTOHOST 
-DINET6
 
 LDADD+=-lpcap -ll -lcrypto
 DPADD+=${LIBL} ${LIBPCAP} ${LIBCRYPTO}
@@ -38,7 +38,7 @@ SRCS= tcpdump.c addrtoname.c privsep.c p
print-atalk.c print-domain.c print-tftp.c print-bootp.c print-nfs.c \
print-icmp.c print-sl.c print-ppp.c print-rip.c print-timed.c \
print-snmp.c print-ntp.c print-null.c print-ospf.c print-gtp.c \
-   print-fddi.c print-llc.c print-sunrpc.c print-hsrp.c print-vqp.c \
+   print-llc.c print-sunrpc.c print-hsrp.c print-vqp.c \
print-vrrp.c print-wb.c print-decnet.c print-isoclns.c print-ipx.c \
print-atm.c print-dvmrp.c print-krb.c print-pim.c print-netbios.c \
util.c bpf_dump.c parsenfsfh.c version.c print-igrp.c \
Index: fddi.h
===
RCS file: fddi.h
diff -N fddi.h
--- fddi.h  7 Oct 2007 16:41:05 -   1.7
+++ /dev/null   1 Jan 1970 00:00:00 -
@@ -1,71 +0,0 @@
-/* $OpenBSD: fddi.h,v 1.7 2007/10/07 16:41:05 deraadt Exp $*/
-
-/*
- * Copyright (c) 1992, 1993, 1994, 1995, 1996
- * The Regents of the University of California.  All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that: (1) source code distributions
- * retain the above copyright notice and this paragraph in its entirety, (2)
- * distributions including binary code include the above copyright notice and
- * this paragraph in its entirety in the documentation or other materials
- * provided with the distribution, and (3) all advertising materials mentioning
- * features or use of this software display the following acknowledgement:
- * ``This p

Simplify tcpdump

2016-01-25 Thread Michael McConville
fddi_bitswap is only used once, and it just adds a layer of indirection
to its preprocessor condition.

This also removes macros for BSDi and Ultrix.

ok?


Index: print-fddi.c
===
RCS file: /cvs/src/usr.sbin/tcpdump/print-fddi.c,v
retrieving revision 1.17
diff -u -p -r1.17 print-fddi.c
--- print-fddi.c16 Nov 2015 00:16:39 -  1.17
+++ print-fddi.c26 Jan 2016 03:47:55 -
@@ -48,16 +48,6 @@ struct rtentry;
 #include "fddi.h"
 
 /*
- * Some FDDI interfaces use bit-swapped addresses.
- */
-#if defined(ultrix) || defined(__alpha) || defined(__bsdi) || \
-   defined(__NetBSD__) || defined(__OpenBSD__)
-intfddi_bitswap = 0;
-#else
-intfddi_bitswap = 1;
-#endif
-
-/*
  * FDDI support for tcpdump, by Jeffrey Mogul [DECWRL], June 1992
  *
  * Based in part on code by Van Jacobson, which bears this note:
@@ -200,20 +190,22 @@ extract_fddi_addrs(const struct fddi_hea
 {
int i;
 
-   if (fddi_bitswap) {
-   /*
-* bit-swap the fddi addresses (isn't the IEEE standards
-* process wonderful!) then convert them to names.
-*/
-   for (i = 0; i < 6; ++i)
-   fdst[i] = fddi_bit_swap[fddip->fddi_dhost[i]];
-   for (i = 0; i < 6; ++i)
-   fsrc[i] = fddi_bit_swap[fddip->fddi_shost[i]];
-   }
-   else {
-   memcpy(fdst, (char *)fddip->fddi_dhost, 6);
-   memcpy(fsrc, (char *)fddip->fddi_shost, 6);
-   }
+   /*
+* Some FDDI interfaces use bit-swapped addresses.
+*/
+#if defined(__alpha) || defined(__NetBSD__) || defined(__OpenBSD__)
+   memcpy(fdst, (char *)fddip->fddi_dhost, 6);
+   memcpy(fsrc, (char *)fddip->fddi_shost, 6);
+#else
+   /*
+* bit-swap the fddi addresses (isn't the IEEE standards
+* process wonderful!) then convert them to names.
+*/
+   for (i = 0; i < 6; ++i)
+   fdst[i] = fddi_bit_swap[fddip->fddi_dhost[i]];
+   for (i = 0; i < 6; ++i)
+   fsrc[i] = fddi_bit_swap[fddip->fddi_shost[i]];
+#endif
 }
 
 /*



Loops depending on undefined behavior

2016-01-25 Thread Michael McConville
I've discussed the below with otto@, who thought it was sound but wanted
confirmation before I commit.

While debugging rarpd(8) for unrelated reasons, I noticed a loop of the
following form:

> for (i = 1; i; i <<= 1)

where i is an int.

This relies on shifting into and then out of the sign bit, both of which
are undefined operations. The loop contains no breaks or returns, so
they are always executed.

It was copied around a few times, so there are a few nearly identical
instances included in the below diff.

These instances use i for:

 o boolean tests
 o comparison with small positive constants (RTA_*)
 o  with an int

The third case seems questionable. However, (IIUC) integer conversion
rules dictate that the int is implicitly cast to an unsigned int[1] and
that two's complement arithmetic is preserved in this cast.[2]

C integer type rules are such a joy to work with...

Am I interpreting this correctly?

[1] Usual Arithmetic Conversions #3 in:
https://www.securecoding.cert.org/confluence/x/QgE

[2] C99/C11 6.3.1.3
http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1570.pdf


Index: sbin/dhclient/dhclient.c
===
RCS file: /cvs/src/sbin/dhclient/dhclient.c,v
retrieving revision 1.370
diff -u -p -r1.370 dhclient.c
--- sbin/dhclient/dhclient.c12 Dec 2015 14:48:17 -  1.370
+++ sbin/dhclient/dhclient.c21 Jan 2016 02:16:17 -
@@ -160,7 +160,7 @@ int
 findproto(char *cp, int n)
 {
struct sockaddr *sa;
-   int i;
+   unsigned int i;
 
if (n == 0)
return -1;
Index: usr.sbin/arp/arp.c
===
RCS file: /cvs/src/usr.sbin/arp/arp.c,v
retrieving revision 1.70
diff -u -p -r1.70 arp.c
--- usr.sbin/arp/arp.c  8 Dec 2015 14:20:24 -   1.70
+++ usr.sbin/arp/arp.c  21 Jan 2016 02:16:17 -
@@ -695,7 +695,7 @@ rtget(struct sockaddr_inarp **sinp, stru
struct sockaddr_dl *sdl = NULL;
struct sockaddr *sa;
char *cp;
-   int i;
+   unsigned int i;
 
if (rtmsg(RTM_GET) < 0)
return (1);
Index: usr.sbin/ndp/ndp.c
===
RCS file: /cvs/src/usr.sbin/ndp/ndp.c,v
retrieving revision 1.68
diff -u -p -r1.68 ndp.c
--- usr.sbin/ndp/ndp.c  12 Jan 2016 09:47:13 -  1.68
+++ usr.sbin/ndp/ndp.c  21 Jan 2016 02:16:17 -
@@ -879,7 +879,7 @@ rtget(struct sockaddr_in6 **sinp, struct
struct sockaddr_dl *sdl = NULL;
struct sockaddr *sa;
char *cp;
-   int i;
+   unsigned int i;
 
if (rtmsg(RTM_GET) < 0)
return (1);
Index: usr.sbin/rarpd/arptab.c
===
RCS file: /cvs/src/usr.sbin/rarpd/arptab.c,v
retrieving revision 1.25
diff -u -p -r1.25 arptab.c
--- usr.sbin/rarpd/arptab.c 19 Nov 2015 19:31:20 -  1.25
+++ usr.sbin/rarpd/arptab.c 21 Jan 2016 02:16:17 -
@@ -240,7 +240,7 @@ rtget(struct sockaddr_inarp **sinp, stru
struct sockaddr_dl *sdl = NULL;
struct sockaddr *sa;
char *cp;
-   int i;
+   unsigned int i;
 
if (rtmsg(RTM_GET) < 0)
return (1);



Re: Writing Documentation

2016-01-23 Thread Michael McConville
Timo wrote:
> Hi who is responsible for writing documentation for OpenBSD? I'd like
> to get involved in writing documentation for OpenBSD as I really like
> OpenBSD and I feel technical writing is one of my strong points.

In case you (or other readers) need more of an introduction than other
people gave:

You'll need to learn how to use CVS. It's very simple:

http://www.openbsd.org/anoncvs.html

The website, including the FAQ, is in the www/ repo. The man pages are
combined with the source code in the src/ repo.

A typical simple CVS workflow looks something like this:

 o cd to the repo dir

 o cvs up -Pd(use cvs up -PdC if you want to undo any existing
  changes)

 o make your changes

 o cvs diff > /tmp/t.diff

 o append your diff to an email bound for tech@

The man pages are in the mdoc format - see the mdoc(7) man page. Man
pages have filenames like mdoc.7, where the trailing digit is the
section number. To view your local changes, you can use something like:

> mandoc path/to/mdoc.7 | less

Remember that while OpenBSD hosts and develops many of its own tools, it
also includes some third-party projects like GCC. We generally leave
third-party man pages alone unless there's a serious issue.

The mdoc format can be complicated , so make sure to read the man page
as much as you can manage.  :-)  In general, try to imitate the style of
the surrounding text.

Enjoy! And let me know if you have any remaining questions.

Michael



Re: [patch] cleanup vi

2016-01-22 Thread Michael McConville
Vadim Zhukov wrote:
> 2016-01-22 23:18 GMT+03:00 Martijn van Duren 
> :
> > 5) 5_vi_remove_v_strdup.diff: Use strdup instead of v_strdup.
> 
> Not okay: MALLOC macro prints information message to user, while
> patched version isn't. I think refactoring shouldn't have such visible
> impact on functionality, to make sure nothing breaks.

On the subject of the allocation macros:

 o maybe we should wrap them in do { ... } while (0) for safety's sake

 o I got the *_NOMSG removals upstreamed. Is the below diff ok? No
   binary change.


Index: cl/cl_main.c
===
RCS file: /cvs/src/usr.bin/vi/cl/cl_main.c,v
retrieving revision 1.28
diff -u -p -r1.28 cl_main.c
--- cl/cl_main.c28 Dec 2015 19:24:01 -  1.28
+++ cl/cl_main.c23 Jan 2016 06:40:49 -
@@ -151,7 +151,7 @@ gs_init(char *name)
name = p + 1;
 
/* Allocate the global structure. */
-   CALLOC_NOMSG(NULL, gp, 1, sizeof(GS));
+   gp = calloc(1, sizeof(GS));
if (gp == NULL)
err(1, NULL);
 
@@ -171,7 +171,7 @@ cl_init(GS *gp)
int fd;
 
/* Allocate the CL private structure. */
-   CALLOC_NOMSG(NULL, clp, 1, sizeof(CL_PRIVATE));
+   clp = calloc(1, sizeof(CL_PRIVATE));
if (clp == NULL)
err(1, NULL);
gp->cl_private = clp;
Index: common/main.c
===
RCS file: /cvs/src/usr.bin/vi/common/main.c,v
retrieving revision 1.33
diff -u -p -r1.33 main.c
--- common/main.c   9 Jan 2016 16:13:26 -   1.33
+++ common/main.c   23 Jan 2016 06:40:49 -
@@ -361,7 +361,7 @@ editor(GS *gp, int argc, char *argv[])
size_t l;
/* Cheat -- we know we have an extra argv slot. */
l = strlen(sp->frp->name) + 1;
-   MALLOC_NOMSG(sp, *--argv, l);
+   *--argv = malloc(l);
if (*argv == NULL) {
v_estr(gp->progname, errno, NULL);
goto err;
@@ -561,7 +561,7 @@ v_obsolete(char *name, char *argv[])
} else  {
p = argv[0];
len = strlen(argv[0]);
-   MALLOC_NOMSG(NULL, argv[0], len + 2);
+   argv[0] = malloc(len + 2);
if (argv[0] == NULL)
goto nomem;
argv[0][0] = '-';
Index: common/mem.h
===
RCS file: /cvs/src/usr.bin/vi/common/mem.h,v
retrieving revision 1.7
diff -u -p -r1.7 mem.h
--- common/mem.h7 Dec 2015 20:39:19 -   1.7
+++ common/mem.h23 Jan 2016 06:40:49 -
@@ -120,9 +120,6 @@
if (((p) = calloc((nmemb), (size))) == NULL)\
goto alloc_err; \
 }
-#defineCALLOC_NOMSG(sp, p, nmemb, size) {  
\
-   (p) = calloc((nmemb), (size));  \
-}
 #defineCALLOC_RET(sp, p, nmemb, size) {
\
if (((p) = calloc((nmemb), (size))) == NULL) {  \
msgq((sp), M_SYSERR, NULL); \
@@ -137,9 +134,6 @@
 #defineMALLOC_GOTO(sp, p, size) {  
\
if (((p) = malloc(size)) == NULL)   \
goto alloc_err; \
-}
-#defineMALLOC_NOMSG(sp, p, size) { 
\
-   (p) = malloc(size); \
 }
 #defineMALLOC_RET(sp, p, size) {   
\
if (((p) = malloc(size)) == NULL) { \



Fix nfs_nget() description

2016-01-19 Thread Michael McConville
Apparently the function was refactored and but the old description
remained. I came across this a few weeks ago. Does my revision look
good?


Index: nfs/nfs_node.c
===
RCS file: /cvs/src/sys/nfs/nfs_node.c,v
retrieving revision 1.62
diff -u -p -r1.62 nfs_node.c
--- nfs/nfs_node.c  14 Mar 2015 03:38:52 -  1.62
+++ nfs/nfs_node.c  20 Jan 2016 01:56:19 -
@@ -76,10 +76,9 @@ RB_PROTOTYPE(nfs_nodetree, nfsnode, n_en
 RB_GENERATE(nfs_nodetree, nfsnode, n_entry, nfsnode_cmp);
 
 /*
- * Look up a vnode/nfsnode by file handle.
+ * Look up a vnode/nfsnode by file handle and store the pointer in *npp.
  * Callers must check for mount points!!
- * In all cases, a pointer to a
- * nfsnode structure is returned.
+ * An error number is returned.
  */
 int
 nfs_nget(struct mount *mnt, nfsfh_t *fh, int fhsize, struct nfsnode **npp)



Proper printf attribute in less(1)

2016-01-17 Thread Michael McConville
Michael Reed and I noticed the straggling lintism PRINTFLIKE1 in
less(1). Should it be replaced with an attribute? If so, am I doing this
right?


Index: funcs.h
===
RCS file: /cvs/src/usr.bin/less/funcs.h,v
retrieving revision 1.17
diff -u -p -r1.17 funcs.h
--- funcs.h 15 Jan 2016 22:22:38 -  1.17
+++ funcs.h 18 Jan 2016 03:34:55 -
@@ -10,8 +10,8 @@ struct mlist;
 struct loption;
 
 void *ecalloc(int, unsigned int);
-/*PRINTFLIKE1*/
-char *easprintf(const char *, ...);
+char *easprintf(const char *, ...)
+__attribute__((__format__(printf, 1, 2)));
 char *estrdup(const char *);
 char *skipsp(char *);
 int sprefix(char *, char *, int);



Re: [patch] ls + utf-8 support

2016-01-17 Thread Michael McConville
Martin Natano wrote:
> I agree with Ingo, ls(1) shouldn't generate unsafe output. Regardless
> of whether the output is to a terminal or some other file.

While POSIX is not holy law, doing what you suggest would probably be a
violation. See the description of the -q option:

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/ls.html

That combined with Stuart's mentioned legitimate uses makes me think
that this could cause subtle and unexpected problems. I do see the
danger of allowing raw bytestring redirection, though.



Re: [PATCH] uname, arch/machine -> %c, %a update in PKG_PATH

2016-01-17 Thread Michael McConville
Raf Czlonka wrote:
> Hi all,
> 
> Given that PKG_PATH and pkg.conf(5)'s installpath, now supports %c, %a,
> etc. sequences, it might be worth advertising it a bit more by changing
> all relevant uname(1), arch(1)/machine(1) occurrences or (hard-coded
> release versions or hardware architectures for that matter) in the
> documentation.
> 
> While there, I have also taken the liberty of changing ftp.openbsd.org
> to your.local.mirror and ftp to http in packages(7) to keep it
> consistent with other examples.
> 
> Main benefits:
> - as the sequences themselves - not need to hard-code the values
> - no need to run uname, arch/machine is sub-shells any more
> - short and sweet

ok mmcc@

> Index: share/man/man7/packages.7
> ===
> RCS file: /cvs/src/share/man/man7/packages.7,v
> retrieving revision 1.40
> diff -u -p -r1.40 packages.7
> --- share/man/man7/packages.7 24 Oct 2015 08:44:49 -  1.40
> +++ share/man/man7/packages.7 11 Jan 2016 14:26:49 -
> @@ -240,7 +240,7 @@ are supported: pointing
>  .Ev PKG_PATH
>  to a distant package repository, e.g.,
>  .Bd -literal -offset 1n
> -# export PKG_PATH=ftp://ftp.openbsd.org/pub/OpenBSD/5.2/packages/i386/
> +# export PKG_PATH=http://your.local.mirror/pub/OpenBSD/%c/packages/%a/
>  .Ed
>  .Pp
>  will let
> Index: faq/faq15.html
> ===
> RCS file: /cvs/www/faq/faq15.html,v
> retrieving revision 1.116
> diff -u -p -r1.116 faq15.html
> --- faq/faq15.html23 Nov 2015 03:15:50 -  1.116
> +++ faq/faq15.html11 Jan 2016 14:29:33 -
> @@ -203,13 +203,13 @@ A list of possible locations to fetch pa
>  Example 1: fetching from your CD-ROM,
>  assuming you mounted it on /mnt/cdrom
>  
> -$ export PKG_PATH=/mnt/cdrom/$(uname -r)/packages/$(machine -a)/
> +$ export PKG_PATH=/mnt/cdrom/%c/packages/%a/
>  
>  
>  
>  Example 2: fetching from a nearby mirror
>  
> -$ export PKG_PATH=http://your.local.mirror/pub/OpenBSD/$(uname 
> -r)/packages/$(machine -a)/
> +$ export PKG_PATH=http://your.local.mirror/pub/OpenBSD/%c/packages/%a/
>  
>  
>  
> @@ -404,7 +404,7 @@ HTTP, or SCP locations.
>  Let's consider installation via HTTP in the next example:
>  
>  
> -# pkg_add http://your.local.mirror/pub/OpenBSD/$(uname 
> -r)/packages/$(machine -a)/screen-4.0.3p3.tgz
> +# pkg_add 
> http://your.local.mirror/pub/OpenBSD/%c/packages/%a/screen-4.0.3p3.tgz
>  screen-4.0.3p3: complete
>  
>  
> Index: faq/faq9.html
> ===
> RCS file: /cvs/www/faq/faq9.html,v
> retrieving revision 1.116
> diff -u -p -r1.116 faq9.html
> --- faq/faq9.html 23 Nov 2015 03:16:31 -  1.116
> +++ faq/faq9.html 11 Jan 2016 14:29:33 -
> @@ -362,7 +362,7 @@ To find out more about the packages and 
>  To install the above mentioned package you would issue
>  
>  
> -# export PKG_PATH=http://your.local.mirror/pub/OpenBSD/$(uname 
> -r)/packages/$(uname -m)/
> +# export PKG_PATH=http://your.local.mirror/pub/OpenBSD/%c/packages/%a/
>  # pkg_add fedora_base
>  
>  
> Index: faq/pf/example1.html
> ===
> RCS file: /cvs/www/faq/pf/example1.html,v
> retrieving revision 1.63
> diff -u -p -r1.63 example1.html
> --- faq/pf/example1.html  10 Jan 2016 01:28:23 -  1.63
> +++ faq/pf/example1.html  11 Jan 2016 14:29:33 -
> @@ -346,7 +346,7 @@ to use (8.8.8.8@53, for example
>  installing the tool and choosing a resolver.
>  
>  
> -# export PKG_PATH=http://ftp.openbsd.org/pub/OpenBSD/$(uname 
> -r)/packages/$(uname -m)
> +# export PKG_PATH=http://your.local.mirror/pub/OpenBSD/%c/packages/%a/
>  # pkg_add dnscrypt-proxy
>  # rcctl enable dnscrypt_proxy
>  
> Index: faq/ports/guide.html
> ===
> RCS file: /cvs/www/faq/ports/guide.html,v
> retrieving revision 1.46
> diff -u -p -r1.46 guide.html
> --- faq/ports/guide.html  21 Dec 2015 16:35:48 -  1.46
> +++ faq/ports/guide.html  11 Jan 2016 14:29:33 -
> @@ -563,7 +563,7 @@ When dealing with multi-packages, it may
>  >pkg_add(1) and
>   href="http://www.openbsd.org/cgi-bin/man.cgi?sektion=1query=pkg_delete;
>  >pkg_delete(1) directly,
> -setting PKG_PATH to /usr/ports/packages/`arch -s`/all/ in 
> the
> +setting PKG_PATH to /usr/ports/packages/%a/all/ in the
>  environment.
>  
>  
> 



[less] start simplifying message buffer logic

2016-01-13 Thread Michael McConville
message is a global static char[2048].

If I understand correctly (I've been playing with this for a while):

 * message is the user command result message displayed at the bottom
   (e.g. "Pattern not found  (press RETURN)")

 * message is initialized to all NUL bytes because it's static

 * the global static mp points to message's terminating NUL

I think we should try to drop mp. This would allow us to remove many of
the goofy homebrewed appending functions and call strlcat et al.
directly. They are currently just strlcat et al. clones that also update
mp.

Alternatives include:

 * storing message's length
 
 * just calling strlen when necessary

The strlen calls are a performance price, but message has a small max
size, is usually very small (almost all command messages are concise),
and is only updated on user interaction.

I thought I'd describe this to see if other people consider it an
improvement.

The below is a tiny first step, removing uses of mp to determine whether
message is empty.

Thoughts? ok?


Index: prompt.c
===
RCS file: /cvs/src/usr.bin/less/prompt.c,v
retrieving revision 1.20
diff -u -p -r1.20 prompt.c
--- prompt.c13 Jan 2016 22:47:45 -  1.20
+++ prompt.c13 Jan 2016 22:58:16 -
@@ -174,7 +174,7 @@ cond(char c, int where)
 
switch (c) {
case 'a':   /* Anything in the message yet? */
-   return (mp > message);
+   return (*message != '\0');
case 'b':   /* Current byte offset known? */
return (curr_byte(where) != -1);
case 'c':
@@ -478,7 +478,7 @@ pr_expand(const char *proto, int maxwidt
}
}
 
-   if (mp == message)
+   if (*message == '\0')
return ("");
if (maxwidth > 0 && mp >= message + maxwidth) {
/*



Re: Simplify less(1) off_t formatting

2016-01-13 Thread Michael McConville
Nicholas Marriott wrote:
> I like the idea, but I don't like calling them ap_off_t and offttoa,
> I'd just keep ap_pos and postoa and remove the linenum functions.

Does this look good?


Index: less.h
===
RCS file: /cvs/src/usr.bin/less/less.h,v
retrieving revision 1.21
diff -u -p -r1.21 less.h
--- less.h  12 Jan 2016 17:48:04 -  1.21
+++ less.h  13 Jan 2016 22:40:49 -
@@ -206,5 +206,4 @@ struct textlist {
 
 /* Functions not included in funcs.h */
 void postoa(off_t, char *, size_t);
-void linenumtoa(off_t, char *, size_t);
 void inttoa(int, char *, size_t);
Index: line.c
===
RCS file: /cvs/src/usr.bin/less/line.c,v
retrieving revision 1.16
diff -u -p -r1.16 line.c
--- line.c  12 Jan 2016 17:48:04 -  1.16
+++ line.c  13 Jan 2016 22:40:49 -
@@ -178,7 +178,7 @@ plinenum(off_t pos)
char buf[INT_STRLEN_BOUND(pos) + 2];
int n;
 
-   linenumtoa(linenum, buf, sizeof (buf));
+   postoa(linenum, buf, sizeof(buf));
n = strlen(buf);
if (n < MIN_LINENUM_WIDTH)
n = MIN_LINENUM_WIDTH;
Index: output.c
===
RCS file: /cvs/src/usr.bin/less/output.c,v
retrieving revision 1.14
diff -u -p -r1.14 output.c
--- output.c12 Jan 2016 17:48:04 -  1.14
+++ output.c13 Jan 2016 22:40:50 -
@@ -149,7 +149,6 @@ funcname(type num, char *buf, size_t len
 }
 
 TYPE_TO_A_FUNC(postoa, off_t)
-TYPE_TO_A_FUNC(linenumtoa, off_t)
 TYPE_TO_A_FUNC(inttoa, int)
 
 /*
@@ -173,7 +172,7 @@ iprint_linenum(off_t num)
 {
char buf[INT_STRLEN_BOUND(num)];
 
-   linenumtoa(num, buf, sizeof (buf));
+   postoa(num, buf, sizeof(buf));
putstr(buf);
return (strlen(buf));
 }
Index: prompt.c
===
RCS file: /cvs/src/usr.bin/less/prompt.c,v
retrieving revision 1.19
diff -u -p -r1.19 prompt.c
--- prompt.c12 Jan 2016 23:01:23 -  1.19
+++ prompt.c13 Jan 2016 22:40:50 -
@@ -120,19 +120,7 @@ ap_pos(off_t pos)
 {
char buf[INT_STRLEN_BOUND(pos) + 2];
 
-   postoa(pos, buf, sizeof buf);
-   ap_str(buf);
-}
-
-/*
- * Append a line number to the end of the message.
- */
-static void
-ap_linenum(off_t linenum)
-{
-   char buf[INT_STRLEN_BOUND(linenum) + 2];
-
-   linenumtoa(linenum, buf, sizeof buf);
+   postoa(pos, buf, sizeof(buf));
ap_str(buf);
 }
 
@@ -255,7 +243,7 @@ protochar(int c, int where)
case 'd':   /* Current page number */
linenum = currline(where);
if (linenum > 0 && sc_height > 1)
-   ap_linenum(PAGE_NUM(linenum));
+   ap_pos(PAGE_NUM(linenum));
else
ap_quest();
break;
@@ -266,13 +254,13 @@ protochar(int c, int where)
ap_quest();
} else if (len == 0) {
/* An empty file has no pages. */
-   ap_linenum(0);
+   ap_pos(0);
} else {
linenum = find_linenum(len - 1);
if (linenum <= 0)
ap_quest();
else
-   ap_linenum(PAGE_NUM(linenum));
+   ap_pos(PAGE_NUM(linenum));
}
break;
case 'E':   /* Editor name */
@@ -293,7 +281,7 @@ protochar(int c, int where)
case 'l':   /* Current line number */
linenum = currline(where);
if (linenum != 0)
-   ap_linenum(linenum);
+   ap_pos(linenum);
else
ap_quest();
break;
@@ -303,7 +291,7 @@ protochar(int c, int where)
(linenum = find_linenum(len)) <= 0)
ap_quest();
else
-   ap_linenum(linenum-1);
+   ap_pos(linenum-1);
break;
case 'm':   /* Number of files */
n = ntags();



Re: gcc typo

2016-01-13 Thread Michael McConville
Jan Schreiber wrote:
> this looks like a typo in gcc.

Committed. Thanks!

> Index: gnu/gcc/gcc/config/mt/mt.c
> ===
> RCS file: /cvs/src/gnu/gcc/gcc/config/mt/mt.c,v
> retrieving revision 1.1.1.1
> diff -u -p -r1.1.1.1 mt.c
> --- gnu/gcc/gcc/config/mt/mt.c15 Oct 2009 17:11:30 -  1.1.1.1
> +++ gnu/gcc/gcc/config/mt/mt.c12 Jan 2016 20:07:28 -
> @@ -475,7 +475,7 @@ mt_print_operand (FILE * file, rtx x, in
>break;
>
>  default:
> -  fprintf(file, "Uknown code: %d", GET_CODE (x));
> +  fprintf(file, "Unknown code: %d", GET_CODE (x));
>break;
>  }
>  
> 



Simplify less(1) off_t formatting

2016-01-12 Thread Michael McConville
I'm working on bigger simplifications for less's string formatting, but
this is a good start. We've removed the off_t aliases for linenums and
positions, so we now have two identical sets of off_t formatting and
appending functions. The below diff unifies them.

As you can see, there is more cleanup to be done.

Thoughts?


Index: less.h
===
RCS file: /cvs/src/usr.bin/less/less.h,v
retrieving revision 1.21
diff -u -p -r1.21 less.h
--- less.h  12 Jan 2016 17:48:04 -  1.21
+++ less.h  13 Jan 2016 04:43:15 -
@@ -205,6 +205,5 @@ struct textlist {
 #include "funcs.h"
 
 /* Functions not included in funcs.h */
-void postoa(off_t, char *, size_t);
-void linenumtoa(off_t, char *, size_t);
+void offttoa(off_t, char *, size_t);
 void inttoa(int, char *, size_t);
Index: line.c
===
RCS file: /cvs/src/usr.bin/less/line.c,v
retrieving revision 1.16
diff -u -p -r1.16 line.c
--- line.c  12 Jan 2016 17:48:04 -  1.16
+++ line.c  13 Jan 2016 04:43:15 -
@@ -178,7 +178,7 @@ plinenum(off_t pos)
char buf[INT_STRLEN_BOUND(pos) + 2];
int n;
 
-   linenumtoa(linenum, buf, sizeof (buf));
+   offttoa(linenum, buf, sizeof(buf));
n = strlen(buf);
if (n < MIN_LINENUM_WIDTH)
n = MIN_LINENUM_WIDTH;
Index: output.c
===
RCS file: /cvs/src/usr.bin/less/output.c,v
retrieving revision 1.14
diff -u -p -r1.14 output.c
--- output.c12 Jan 2016 17:48:04 -  1.14
+++ output.c13 Jan 2016 04:43:15 -
@@ -148,8 +148,7 @@ funcname(type num, char *buf, size_t len
(void) strlcpy(buf, s, len);\
 }
 
-TYPE_TO_A_FUNC(postoa, off_t)
-TYPE_TO_A_FUNC(linenumtoa, off_t)
+TYPE_TO_A_FUNC(offttoa, off_t)
 TYPE_TO_A_FUNC(inttoa, int)
 
 /*
@@ -173,7 +172,7 @@ iprint_linenum(off_t num)
 {
char buf[INT_STRLEN_BOUND(num)];
 
-   linenumtoa(num, buf, sizeof (buf));
+   offttoa(num, buf, sizeof(buf));
putstr(buf);
return (strlen(buf));
 }
Index: prompt.c
===
RCS file: /cvs/src/usr.bin/less/prompt.c,v
retrieving revision 1.19
diff -u -p -r1.19 prompt.c
--- prompt.c12 Jan 2016 23:01:23 -  1.19
+++ prompt.c13 Jan 2016 04:43:15 -
@@ -116,23 +116,11 @@ ap_char(char c)
  * Append a off_t (as a decimal integer) to the end of the message.
  */
 static void
-ap_pos(off_t pos)
+ap_off_t(off_t pos)
 {
char buf[INT_STRLEN_BOUND(pos) + 2];
 
-   postoa(pos, buf, sizeof buf);
-   ap_str(buf);
-}
-
-/*
- * Append a line number to the end of the message.
- */
-static void
-ap_linenum(off_t linenum)
-{
-   char buf[INT_STRLEN_BOUND(linenum) + 2];
-
-   linenumtoa(linenum, buf, sizeof buf);
+   offttoa(pos, buf, sizeof(buf));
ap_str(buf);
 }
 
@@ -245,7 +233,7 @@ protochar(int c, int where)
case 'b':   /* Current byte offset */
pos = curr_byte(where);
if (pos != -1)
-   ap_pos(pos);
+   ap_off_t(pos);
else
ap_quest();
break;
@@ -255,7 +243,7 @@ protochar(int c, int where)
case 'd':   /* Current page number */
linenum = currline(where);
if (linenum > 0 && sc_height > 1)
-   ap_linenum(PAGE_NUM(linenum));
+   ap_off_t(PAGE_NUM(linenum));
else
ap_quest();
break;
@@ -266,13 +254,13 @@ protochar(int c, int where)
ap_quest();
} else if (len == 0) {
/* An empty file has no pages. */
-   ap_linenum(0);
+   ap_off_t(0);
} else {
linenum = find_linenum(len - 1);
if (linenum <= 0)
ap_quest();
else
-   ap_linenum(PAGE_NUM(linenum));
+   ap_off_t(PAGE_NUM(linenum));
}
break;
case 'E':   /* Editor name */
@@ -293,7 +281,7 @@ protochar(int c, int where)
case 'l':   /* Current line number */
linenum = currline(where);
if (linenum != 0)
-   ap_linenum(linenum);
+   ap_off_t(linenum);
else
ap_quest();
break;
@@ -303,7 +291,7 @@ protochar(int c, int where)
(linenum = find_linenum(len)) <= 0)
ap_quest();
else
-   ap_linenum(linenum-1);
+   

Re: [patch] kern/exec_script: return error when the shell name is not specified

2016-01-10 Thread Michael McConville
Michael McConville wrote:
> Maxim Pugachev wrote:
> > In a case when the shell name is not specified (i.e. just "#!" without
> > a path), don't run the heavy logic that checks shell, simply return
> > ENOENT.
> 
> I'm not sure whether this is a reasonable thing to do. Someone with more
> kernel API experience will have to comment.
> 
> > Also, as a tiny improvement: avoid a loop when calculating shell's
> > args length.
> 
> It seems like there's a simpler solution to this: strlen. Also, the
> assignment that follows the for loop seems dead, as we know *cp will be
> NUL.
> 
> The below diff makes that change, removes the useless if condition you
> noticed, and bumps a few variable initializations into the declaration
> block.

I just remembered that this was probably a perfomance concern as well.
In that case, we could assign shellarg and shellarglen earlier in the
function, right?

> Index: sys/kern/exec_script.c
> ===
> RCS file: /cvs/src/sys/kern/exec_script.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 exec_script.c
> --- sys/kern/exec_script.c31 Dec 2015 18:55:33 -  1.37
> +++ sys/kern/exec_script.c10 Jan 2016 01:41:55 -
> @@ -67,9 +67,9 @@
>  int
>  exec_script_makecmds(struct proc *p, struct exec_package *epp)
>  {
> - int error, hdrlinelen, shellnamelen, shellarglen;
> + int error, hdrlinelen, shellnamelen, shellarglen = 0;
>   char *hdrstr = epp->ep_hdr;
> - char *cp, *shellname, *shellarg, *oldpnbuf;
> + char *cp, *shellname = NULL, *shellarg = NULL, *oldpnbuf;
>   char **shellargp = NULL, **tmpsap;
>   struct vnode *scriptvp;
>   uid_t script_uid = -1;
> @@ -110,10 +110,6 @@ exec_script_makecmds(struct proc *p, str
>   if (cp >= hdrstr + hdrlinelen)
>   return ENOEXEC;
>  
> - shellname = NULL;
> - shellarg = NULL;
> - shellarglen = 0;
> -
>   /* strip spaces before the shell name */
>   for (cp = hdrstr + EXEC_SCRIPT_MAGICLEN; *cp == ' ' || *cp == '\t';
>   cp++)
> @@ -122,8 +118,6 @@ exec_script_makecmds(struct proc *p, str
>   /* collect the shell name; remember its length for later */
>   shellname = cp;
>   shellnamelen = 0;
> - if (*cp == '\0')
> - goto check_shell;
>   for ( /* cp = cp */ ; *cp != '\0' && *cp != ' ' && *cp != '\t'; cp++)
>   shellnamelen++;
>   if (*cp == '\0')
> @@ -142,9 +136,8 @@ exec_script_makecmds(struct proc *p, str
>* behaviour.
>*/
>   shellarg = cp;
> - for ( /* cp = cp */ ; *cp != '\0'; cp++)
> - shellarglen++;
> - *cp++ = '\0';
> + shellarglen = strlen(shellarg);
> + cp += shellarglen + 1;
>  
>  check_shell:
>   /*
> 



Re: [patch] kern/exec_script: return error when the shell name is not specified

2016-01-09 Thread Michael McConville
Maxim Pugachev wrote:
> In a case when the shell name is not specified (i.e. just "#!" without
> a path), don't run the heavy logic that checks shell, simply return
> ENOENT.

I'm not sure whether this is a reasonable thing to do. Someone with more
kernel API experience will have to comment.

> Also, as a tiny improvement: avoid a loop when calculating shell's
> args length.

It seems like there's a simpler solution to this: strlen. Also, the
assignment that follows the for loop seems dead, as we know *cp will be
NUL.

The below diff makes that change, removes the useless if condition you
noticed, and bumps a few variable initializations into the declaration
block.


Index: sys/kern/exec_script.c
===
RCS file: /cvs/src/sys/kern/exec_script.c,v
retrieving revision 1.37
diff -u -p -r1.37 exec_script.c
--- sys/kern/exec_script.c  31 Dec 2015 18:55:33 -  1.37
+++ sys/kern/exec_script.c  10 Jan 2016 01:41:55 -
@@ -67,9 +67,9 @@
 int
 exec_script_makecmds(struct proc *p, struct exec_package *epp)
 {
-   int error, hdrlinelen, shellnamelen, shellarglen;
+   int error, hdrlinelen, shellnamelen, shellarglen = 0;
char *hdrstr = epp->ep_hdr;
-   char *cp, *shellname, *shellarg, *oldpnbuf;
+   char *cp, *shellname = NULL, *shellarg = NULL, *oldpnbuf;
char **shellargp = NULL, **tmpsap;
struct vnode *scriptvp;
uid_t script_uid = -1;
@@ -110,10 +110,6 @@ exec_script_makecmds(struct proc *p, str
if (cp >= hdrstr + hdrlinelen)
return ENOEXEC;
 
-   shellname = NULL;
-   shellarg = NULL;
-   shellarglen = 0;
-
/* strip spaces before the shell name */
for (cp = hdrstr + EXEC_SCRIPT_MAGICLEN; *cp == ' ' || *cp == '\t';
cp++)
@@ -122,8 +118,6 @@ exec_script_makecmds(struct proc *p, str
/* collect the shell name; remember its length for later */
shellname = cp;
shellnamelen = 0;
-   if (*cp == '\0')
-   goto check_shell;
for ( /* cp = cp */ ; *cp != '\0' && *cp != ' ' && *cp != '\t'; cp++)
shellnamelen++;
if (*cp == '\0')
@@ -142,9 +136,8 @@ exec_script_makecmds(struct proc *p, str
 * behaviour.
 */
shellarg = cp;
-   for ( /* cp = cp */ ; *cp != '\0'; cp++)
-   shellarglen++;
-   *cp++ = '\0';
+   shellarglen = strlen(shellarg);
+   cp += shellarglen + 1;
 
 check_shell:
/*



Re: allocation simplifications in yacc

2015-12-30 Thread Michael McConville
Christian Weisgerber wrote:
> Christian Weisgerber:
> 
> > The removal of the casts causes a problem in ports/devel/mico, where
> > the yacc output is used in C++:
> 
> Also:
>   graphics/grap
>   math/aamath
>   math/logic2cnf

ok?


Index: skeleton.c
===
RCS file: /cvs/src/usr.bin/yacc/skeleton.c,v
retrieving revision 1.36
diff -u -p -r1.36 skeleton.c
--- skeleton.c  28 Dec 2015 19:14:04 -  1.36
+++ skeleton.c  30 Dec 2015 16:28:57 -
@@ -137,14 +137,14 @@ char *body[] =
"#endif",
"if (newsize && YY_SIZE_MAX / newsize < sizeof *newss)",
"goto bail;",
-   "newss = realloc(yyss, newsize * sizeof(*newss)); /* overflow check 
above */",
+   "newss = (short *)realloc(yyss, newsize * sizeof(*newss)); /* 
overflow check above */",
"if (newss == NULL)",
"goto bail;",
"yyss = newss;",
"yyssp = newss + sslen;",
"if (newsize && YY_SIZE_MAX / newsize < sizeof *newvs)",
"goto bail;",
-   "newvs = realloc(yyvs, newsize * sizeof(*newvs)); /* overflow check 
above */",
+   "newvs = (short *)realloc(yyvs, newsize * sizeof(*newvs)); /* 
overflow check above */",
"if (newvs == NULL)",
"goto bail;",
"yyvs = newvs;",



Re: [patch] kern/exec_script: avoid invalid free() in a case of error

2015-12-29 Thread Michael McConville
Michael McConville wrote:
> Michael McConville wrote:
> > > On Sun, Dec 13, 2015 at 9:45 PM, Maxim Pugachev <pugachev...@gmail.com> 
> > > wrote:
> > > > Hi,
> > > >
> > > > In exec_script_makecmds function, when EXEC_HASFD flag was set, but
> > > > copystr/copyinstr returns an error, we need to set *tmpsap to NULL to
> > > > terminate a loop (under "fail" label) correctly.
> > 
> > I spent a while straining to see the forest through the trees, but this
> > makes sense to me. ok mmcc@
> > 
> > Is this allocation chain idiom used elsewhere in sys/kern? Maybe we
> > could break it out into ~3 functions. The current state of affairs seems
> > bug-prone.
> 
> It seems that all possible paths through this nested condition increment
> tmpsap. Why not just increment it afterward so no one else ends up with
> the headache I now have?
> 
> As always: I could be misinterpreting this.

tedu and gsoares pointed out the nop expression in my last diff. I guess
I was moving too fast...

Here's a new one:


Index: sys/kern/exec_script.c
===
RCS file: /cvs/src/sys/kern/exec_script.c,v
retrieving revision 1.36
diff -u -p -r1.36 exec_script.c
--- sys/kern/exec_script.c  10 Sep 2015 18:10:35 -  1.36
+++ sys/kern/exec_script.c  30 Dec 2015 06:53:38 -
@@ -208,24 +208,25 @@ check_shell:
 #if NSYSTRACE > 0
if (ISSET(p->p_flag, P_SYSTRACE)) {
error = systrace_scriptname(p, *tmpsap);
-   if (error == 0)
-   tmpsap++;
-   else
+   if (error != 0)
/*
 * Since systrace_scriptname() provides a
 * convenience, not a security issue, we are
 * safe to do this.
 */
-   error = copystr(epp->ep_name, *tmpsap++,
+   error = copystr(epp->ep_name, *tmpsap,
MAXPATHLEN, NULL);
} else
 #endif
-   error = copyinstr(epp->ep_name, *tmpsap++, MAXPATHLEN,
+   error = copyinstr(epp->ep_name, *tmpsap, MAXPATHLEN,
NULL);
-   if (error != 0)
+   if (error != 0) {
+   *(tmpsap + 1) = NULL;
goto fail;
+   }
} else
-   snprintf(*tmpsap++, MAXPATHLEN, "/dev/fd/%d", epp->ep_fd);
+   snprintf(*tmpsap, MAXPATHLEN, "/dev/fd/%d", epp->ep_fd);
+   tmpsap++;
*tmpsap = NULL;
 
/*



Re: [patch] kern/exec_script: avoid invalid free() in a case of error

2015-12-28 Thread Michael McConville
> On Sun, Dec 13, 2015 at 9:45 PM, Maxim Pugachev  wrote:
> > Hi,
> >
> > In exec_script_makecmds function, when EXEC_HASFD flag was set, but
> > copystr/copyinstr returns an error, we need to set *tmpsap to NULL to
> > terminate a loop (under "fail" label) correctly.

I spent a while straining to see the forest through the trees, but this
makes sense to me. ok mmcc@

Is this allocation chain idiom used elsewhere in sys/kern? Maybe we
could break it out into ~3 functions. The current state of affairs seems
bug-prone.

> > Index: sys/kern/exec_script.c
> > ===
> > RCS file: /cvs/src/sys/kern/exec_script.c,v
> > retrieving revision 1.36
> > diff -u -p -r1.36 exec_script.c
> > --- sys/kern/exec_script.c  10 Sep 2015 18:10:35 -  1.36
> > +++ sys/kern/exec_script.c  13 Dec 2015 18:33:53 -
> > @@ -222,8 +222,10 @@ check_shell:
> >  #endif
> > error = copyinstr(epp->ep_name, *tmpsap++, 
> > MAXPATHLEN,
> > NULL);
> > -   if (error != 0)
> > +   if (error != 0) {
> > +   *tmpsap = NULL;
> > goto fail;
> > +   }
> > } else
> > snprintf(*tmpsap++, MAXPATHLEN, "/dev/fd/%d", epp->ep_fd);
> > *tmpsap = NULL;
> 



Re: [patch] kern/exec_script: avoid invalid free() in a case of error

2015-12-28 Thread Michael McConville
Michael McConville wrote:
> > On Sun, Dec 13, 2015 at 9:45 PM, Maxim Pugachev <pugachev...@gmail.com> 
> > wrote:
> > > Hi,
> > >
> > > In exec_script_makecmds function, when EXEC_HASFD flag was set, but
> > > copystr/copyinstr returns an error, we need to set *tmpsap to NULL to
> > > terminate a loop (under "fail" label) correctly.
> 
> I spent a while straining to see the forest through the trees, but this
> makes sense to me. ok mmcc@
> 
> Is this allocation chain idiom used elsewhere in sys/kern? Maybe we
> could break it out into ~3 functions. The current state of affairs seems
> bug-prone.

It seems that all possible paths through this nested condition increment
tmpsap. Why not just increment it afterward so no one else ends up with
the headache I now have?

As always: I could be misinterpreting this.


Index: sys/kern/exec_script.c
===
RCS file: /cvs/src/sys/kern/exec_script.c,v
retrieving revision 1.36
diff -u -p -r1.36 exec_script.c
--- sys/kern/exec_script.c  10 Sep 2015 18:10:35 -  1.36
+++ sys/kern/exec_script.c  29 Dec 2015 01:56:11 -
@@ -209,23 +209,26 @@ check_shell:
if (ISSET(p->p_flag, P_SYSTRACE)) {
error = systrace_scriptname(p, *tmpsap);
if (error == 0)
-   tmpsap++;
+   tmpsap;
else
/*
 * Since systrace_scriptname() provides a
 * convenience, not a security issue, we are
 * safe to do this.
 */
-   error = copystr(epp->ep_name, *tmpsap++,
+   error = copystr(epp->ep_name, *tmpsap,
MAXPATHLEN, NULL);
} else
 #endif
-   error = copyinstr(epp->ep_name, *tmpsap++, MAXPATHLEN,
+   error = copyinstr(epp->ep_name, *tmpsap, MAXPATHLEN,
NULL);
-   if (error != 0)
+   if (error != 0) {
+   *(tmpsap + 1) = NULL;
goto fail;
+   }
} else
-   snprintf(*tmpsap++, MAXPATHLEN, "/dev/fd/%d", epp->ep_fd);
+   snprintf(*tmpsap, MAXPATHLEN, "/dev/fd/%d", epp->ep_fd);
+   tmpsap++;
*tmpsap = NULL;
 
/*



Re: [PATCH] vi remove custom perr from cl_main

2015-12-28 Thread Michael McConville
Todd C. Miller wrote:
> On Sun, 27 Dec 2015 10:12:23 +0100, Martijn van Duren wrote:
> 
> > This patch has been accepted by the nvi2 project.[1] There are more
> > patches, but I'm waiting till these have been reviewed by nvi2.
> 
> Committed, thanks.

Thanks for taking care of this.

There's a lot more where that came from. Martijn, Michael Reed, and I
have been chipping away at nvi2 lately. I may be soon be looking for
ok's on diffs that we got committed upstream.



Re: [patch] nlist(3): out of bounds read

2015-12-28 Thread Michael McConville
Serguey Parkhomovsky wrote:
> Ping? This is the same sanity check that's done in nm(1)'s ELF handling.

Make sense to me. Tentative ok mmcc@

Alternatively, this check could be added to __elf_is_ok__, which is
called right above where you added it. However, the definition of the
function would have to change slightly; it's documented as checking
whether the ELF header matches the target platform.

> On Thu, Dec 10, 2015 at 09:40:11AM -0800, Serguey Parkhomovsky wrote:
> > When dealing with a malformed ELF header, e_shentsize may be 0. This
> > causes an out of bounds read while finding the symbol table on line 141.
> > 
> > Found using afl.
> > 
> > Index: nlist.c
> > ===
> > RCS file: /cvs/src/lib/libc/gen/nlist.c,v
> > retrieving revision 1.65
> > diff -u -p -r1.65 nlist.c
> > --- nlist.c 16 Oct 2015 16:54:38 -  1.65
> > +++ nlist.c 10 Dec 2015 16:36:26 -
> > @@ -102,6 +102,10 @@ __fdnlist(int fd, struct nlist *list)
> > !__elf_is_okay__() || fstat(fd, ) < 0)
> > return (-1);
> >  
> > +   /* Make sure section header size is not too small */
> > +   if (ehdr.e_shentsize < sizeof(Elf_Shdr))
> > +   return (-1);
> > +
> > /* calculate section header table size */
> > shdr_size = ehdr.e_shentsize * ehdr.e_shnum;
> >  
> 



Re: [patch] nlist(3): out of bounds read

2015-12-28 Thread Michael McConville
Michael McConville wrote:
> Serguey Parkhomovsky wrote:
> > Ping? This is the same sanity check that's done in nm(1)'s ELF handling.
> 
> Make sense to me. Tentative ok mmcc@
> 
> Alternatively, this check could be added to __elf_is_ok__, which is
> called right above where you added it. However, the definition of the
> function would have to change slightly; it's documented as checking
> whether the ELF header matches the target platform.

Here's a patch for that.

I used the cleanest manner of patching in the check. __elf_is_ok__'s
logic is pretty convoluted for a function that just returns the result
of &&-ing a bunch of boolean conditions. We could turn the entire thing
into a single return statement if we were so inclined.


Index: lib/libc/gen/nlist.c
===
RCS file: /cvs/src/lib/libc/gen/nlist.c,v
retrieving revision 1.65
diff -u -p -r1.65 nlist.c
--- lib/libc/gen/nlist.c16 Oct 2015 16:54:38 -  1.65
+++ lib/libc/gen/nlist.c29 Dec 2015 05:08:09 -
@@ -77,6 +77,9 @@ __elf_is_okay__(Elf_Ehdr *ehdr)
retval = 1;
}
 
+   if (ehdr->e_shentsize < sizeof(Elf_Shdr))
+   return (0);
+
return retval;
 }
 



Re: ksh.1: Remove $ERRNO reference

2015-12-28 Thread Michael McConville
Michael Reed wrote:
> It's had the ``Not implemented'' notice since ksh.1's initial import
> 19 years ago [1], so it's probably not going to be implemented.
> 
> [1]: 
> http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/bin/ksh/ksh.1?rev=1.1=text/x-cvsweb-markup

Committed. Thanks!

bin/ksh/NOTES suggests that the sole motive for $ERRNO may have been its
presence in AT ksh.

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



Remove redundant continue

2015-12-27 Thread Michael McConville
A trivial simplification, but it's a false positive for an
inner-loop-continue bug, so it'd be nice to take care of it.

ok?


Index: sys/arch/amd64/amd64/intr.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/intr.c,v
retrieving revision 1.47
diff -u -p -r1.47 intr.c
--- sys/arch/amd64/amd64/intr.c 8 Dec 2015 19:45:55 -   1.47
+++ sys/arch/amd64/amd64/intr.c 27 Dec 2015 18:43:39 -
@@ -194,10 +194,8 @@ intr_allocate_slot_cpu(struct cpu_info *
slot = i;
break;
}
-   if (isp == NULL && slot == -1) {
+   if (isp == NULL && slot == -1)
slot = i;
-   continue;
-   }
}
if (slot == -1) {
return EBUSY;



Potential continue bug in mii(4)

2015-12-27 Thread Michael McConville
The continue statement below is a nop. It seems that it meant to
continue the outer loop. This is an educated guess based on the logic
and the inner loop's comments. Thoughts?

Also, in situations like this, should we make all continue statements
goto loop_end? In the fix I committed for pciide(4) a few weeks ago, we
only changed the offending continue.


Index: sys/dev/mii/mii.c
===
RCS file: /cvs/src/sys/dev/mii/mii.c,v
retrieving revision 1.21
diff -u -p -r1.21 mii.c
--- sys/dev/mii/mii.c   20 Apr 2010 20:42:16 -  1.21
+++ sys/dev/mii/mii.c   27 Dec 2015 18:30:36 -
@@ -95,7 +95,7 @@ mii_attach(struct device *parent, struct
 * configured at this address.
 */
offset++;
-   continue;
+   goto loop_end;
}
}
 
@@ -144,6 +144,8 @@ mii_attach(struct device *parent, struct
mii->mii_instance++;
}
offset++;
+
+   loop_end: ;
}
 }
 



Re: Mention lang/go and lang/rust in faq8.html

2015-12-26 Thread Michael McConville
Michal Mazurek wrote:
> Go and Rust are now widely used. It might be worth mentioning them in
> the FAQ.

ok mmcc@

Thanks!

> Index: faq8.html
> ===
> RCS file: /cvs/www/faq/faq8.html,v
> retrieving revision 1.268
> diff -u -p -r1.268 faq8.html
> --- faq8.html 24 Nov 2015 00:20:25 -  1.268
> +++ faq8.html 26 Dec 2015 09:13:00 -
> @@ -316,6 +316,14 @@ This new compiler is available as a subp
>  
>  
>  
> +Go
> +
> +http://cvsweb.openbsd.org/cgi-bin/cvsweb/ports/lang/go;>lang/go
> +
> +
> +
> +
> +
>  Haskell
>  
>   href="http://cvsweb.openbsd.org/cgi-bin/cvsweb/ports/lang/ghc;>lang/ghc
> @@ -430,6 +438,14 @@ Plenty of subpackages are available for 
>  Ruby
>  
>   href="http://cvsweb.openbsd.org/cgi-bin/cvsweb/ports/lang/ruby;>lang/ruby
> +
> +
> +
> +
> +
> +Rust
> +
> + href="http://cvsweb.openbsd.org/cgi-bin/cvsweb/ports/lang/rust;>lang/rust
>  
>  
>  
> 
> -- 
> Michal Mazurek
> 



iwn timeouts

2015-12-26 Thread Michael McConville
I've been running -current on a ThinkPad X201 for close to a year now. 

iwn times out after about 45-90 seconds every time I boot. This happens
very reliably, regardless of which network I'm connecting to. All I get
to the syslog is:

> Dec 26 17:05:40 thinkpad /bsd: iwn0: timeout waiting for master

This happens on both ramdisks and installed kernels. After I run
netstart again, it works until the next boot.

/etc/hostname.iwn0 is just:

> nwid mynet
> wpakey mypasswd
> 
> dhcp
> rtsol

I'm pretty sure it happens without rtsol, as I've gone without it in the
past. I'll test this on my next reboot.

I've shelved this in the past because those I've talked to with
identical laptops and cards don't have this problem. However, Theo
thinks it's likely a driver bug, so I decided to bring it up.

Thoughts? Additional info needed? I'm happy to run debug builds if it'd
help.



OpenBSD 5.9-beta (GENERIC.MP) #1778: Wed Dec 23 18:23:06 MST 2015
dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 4062691328 (3874MB)
avail mem = 3935440896 (3753MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.6 @ 0xe0010 (78 entries)
bios0: vendor LENOVO version "6QET61WW (1.31 )" date 10/26/2010
bios0: LENOVO 3626FAU
acpi0 at bios0: rev 2
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP SSDT ECDT APIC MCFG HPET ASF! SLIC BOOT SSDT TCPA SSDT 
SSDT SSDT
acpi0: wakeup devices LID_(S3) SLPB(S3) IGBE(S4) EXP1(S4) EXP2(S4) EXP3(S4) 
EXP4(S4) EXP5(S4) EHC1(S3) EHC2(S3) HDEF(S4)
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpiec0 at acpi0
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Core(TM) i5 CPU M 540 @ 2.53GHz, 2793.55 MHz
cpu0: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,POPCNT,AES,NXE,LONG,LAHF,PERF,ITSC,SENSOR,ARAT
cpu0: 256KB 64b/line 8-way L2 cache
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges
cpu0: apic clock running at 132MHz
cpu0: mwait min=64, max=64, C-substates=0.2.1.1, IBE
cpu1 at mainbus0: apid 1 (application processor)
cpu1: Intel(R) Core(TM) i5 CPU M 540 @ 2.53GHz, 2793.00 MHz
cpu1: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,POPCNT,AES,NXE,LONG,LAHF,PERF,ITSC,SENSOR,ARAT
cpu1: 256KB 64b/line 8-way L2 cache
cpu1: smt 1, core 0, package 0
cpu2 at mainbus0: apid 4 (application processor)
cpu2: Intel(R) Core(TM) i5 CPU M 540 @ 2.53GHz, 2793.00 MHz
cpu2: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,POPCNT,AES,NXE,LONG,LAHF,PERF,ITSC,SENSOR,ARAT
cpu2: 256KB 64b/line 8-way L2 cache
cpu2: smt 0, core 2, package 0
cpu3 at mainbus0: apid 5 (application processor)
cpu3: Intel(R) Core(TM) i5 CPU M 540 @ 2.53GHz, 2793.00 MHz
cpu3: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,POPCNT,AES,NXE,LONG,LAHF,PERF,ITSC,SENSOR,ARAT
cpu3: 256KB 64b/line 8-way L2 cache
cpu3: smt 1, core 2, package 0
ioapic0 at mainbus0: apid 1 pa 0xfec0, version 20, 24 pins
ioapic0: misconfigured as apic 2, remapped to apid 1
acpimcfg0 at acpi0 addr 0xe000, bus 0-255
acpihpet0 at acpi0: 14318179 Hz
acpiprt0 at acpi0: bus 0 (PCI0)
acpiprt1 at acpi0: bus -1 (PEG_)
acpiprt2 at acpi0: bus 13 (EXP1)
acpiprt3 at acpi0: bus -1 (EXP2)
acpiprt4 at acpi0: bus -1 (EXP3)
acpiprt5 at acpi0: bus 5 (EXP4)
acpiprt6 at acpi0: bus 2 (EXP5)
acpicpu0 at acpi0: C3(350@245 mwait.3@0x20), C2(500@205 mwait.3@0x10), 
C1(1000@3 mwait.1), PSS
acpicpu1 at acpi0: C3(350@245 mwait.3@0x20), C2(500@205 mwait.3@0x10), 
C1(1000@3 mwait.1), PSS
acpicpu2 at acpi0: C3(350@245 mwait.3@0x20), C2(500@205 mwait.3@0x10), 
C1(1000@3 mwait.1), PSS
acpicpu3 at acpi0: C3(350@245 mwait.3@0x20), C2(500@205 mwait.3@0x10), 
C1(1000@3 mwait.1), PSS
acpipwrres0 at acpi0: PUBS, resource for EHC1, EHC2
acpitz0 at acpi0: critical temperature is 100 degC
acpibtn0 at acpi0: LID_
acpibtn1 at acpi0: SLPB
acpibat0 at acpi0: BAT0 model "42T4694" serial  1408 type LION oem "SANYO"
acpibat1 at acpi0: BAT1 not present
acpiac0 at acpi0: AC unit online
acpithinkpad0 at acpi0
acpidock0 at acpi0: GDCK not docked (0)
cpu0: Enhanced SpeedStep 2793 MHz: speeds: 2534, 2533, 2399, 2266, 2133, 1999, 
1866, 1733, 1599, 1466, 1333, 1199 MHz
pci0 at mainbus0 bus 0
pchb0 at pci0 dev 0 function 0 "Intel Core Host" rev 0x02
inteldrm0 at pci0 dev 2 function 0 "Intel HD Graphics" rev 0x02
drm0 at 

  1   2   3   >