Re: Minor improvement to socket(2) man page
Philip Guenther wrote: > On Wed, Sep 21, 2016 at 6:23 PM, Michael McConville 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
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
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, &pf) < 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
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&r=1&w=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;
> - }
> +
> + delayed_chu
Re: midiplay: Fix out-of-bounds memory access
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
Miod Vallat wrote: > > >> 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. > > This is not the point. The point is that putting argument names in > public headers increases the risk of breaking third-party software > thanks to the preprocessor. I wasn't suggesting that that's the only reason. I was just pointing it out in case someone suggested the guideline "only specify argument names in file-local prototypes".
Re: bufcache KNF
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
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
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
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&r=1&w=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
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&r=1&w=2
[mm...@mykolab.com: Merge memleak fix from BoringSSL]
Does this look good? - Forwarded message from Michael McConville - Date: Sun, 20 Mar 2016 23:15:48 -0400 From: Michael McConville 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
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
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
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
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, &cpuset_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
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
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
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
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
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)
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
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)
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
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)
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
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)
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
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
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: /cvs/src/lib/libssl/src/doc/crypto/DH_set_method.pod,v retrieving revision 1.7 diff -u -p -r1.7 DH_set_method.pod --- lib/libssl/sr
Re: scheduler: abstract away spc_whichqs
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->ci_mwait, MWAIT_IDLING | MWAIT_ONLY);
> - if (ci->ci_schedstate.spc_whichqs == 0) {
> + if (cpu_is_idle(ci)) {
> monitor(&ci->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->ci_mwait, MWAIT_IDLING | MWAIT_ONLY);
> - if (ci->ci_schedstate.spc_whichqs == 0) {
> + if (cpu_is_idle(ci)) {
> monitor(&ci->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(&start);
> atomic_setbits_int(&ci->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
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)
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
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: Xorg stipple
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.
Re: head(1) -c
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, &errstr);
> + count = strtonum(p, 1, LONG_MAX, &errstr);
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*/
Simplify tcpdump
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
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 = &sc->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 = &sc->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->sc_sensortask);
sensordev_deinstall(&sc->sc_sensordev);
- if (&sc->sc_sensordev != NULL) {
- sensor_detach(&sc->sc_sensordev, &sc->sc_sensor.temp);
- sensor_detach(&sc->sc_sensordev, &sc->sc_sensor.humi);
- }
-
if (sc->sc_sensortask != NULL)
sensor_task_unregister(sc->sc_sensortask);
Re: df(1): replace __progname with getprogname()
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
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
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
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
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
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
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
* 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, &args, 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(&mp->mnt_stat.f_mntonname, sizeof(mp->mnt_stat.f_mntonname));
- bzero(&mp->mnt_stat.f_mntfromname, sizeof(mp->mnt_stat.f_mntfromname));
- bzero(&mp->mnt_stat.f_mntfromspec, sizeof(mp->mnt_stat.f_mntfromspec));
+ memset(&mp->mnt_stat.f_mntonname, 0,
+ sizeof(mp->mnt_stat.f_mntonname));
+ memset(&mp->mnt_stat.f_mntfromname, 0,
+ sizeof(mp->mnt_stat.f_mntfromname));
+ memset(&mp->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)
Mike Belopuhov wrote: > On 27 February 2016 at 08:21, Michael McConville 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)
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)
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)
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
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
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
Re: Integer overflow in syslogd
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, &errstr);
> + 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, &pri);
> 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, &pri);
> + 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!
Fix overflow check in sys/netinet6/in6.c
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->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
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, &errstr);
> + 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,
> &errstr);
> + 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]
}
}
Int overflow in diff(1), rcs(1), cvs(1)
It looks like a few tools in base rely on two's complement integer overflow for the hashing algorithm in readhash(). Overflow can easily be observed using a manual check or a dynamic undefined behavior tool. This function is also present in rcs(1) and cvs(1). Some code locations of these overflows are: /usr/src/usr.bin/diff/diffreg.c:1196 /usr/src/usr.bin/rcs/diff.c:1099 /usr/src/usr.bin/cvs/diff_internals.c:1169 This poses a bit of an issue because (at least in diff(1)) the value field of struct line is represented with an int and is used in many places. Changing the type of line.value to something unsigned could have unintended consequences. Thoughts? I haven't worked with these tools' code previously so I'm not sure what the best/safest way of approaching this is. Michael
Re: [patch] vi enable -pedantic
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: df(1) - unneeded headers
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: pkg_add.1
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 >
Re: iwn firmware error
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
tcpdump CDP crashes
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
Mark Kettenis wrote: > > Date: Fri, 5 Feb 2016 13:06:56 -0500 > > From: Michael McConville > > > > 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: [patch] kern/exec_script: return error when the shell name is not specified
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.
> > 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:
> > /*
> >
>
iwn firmware error
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,
Nop null checks in ti(4)
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 = &sc->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 = &sc->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) {
Re: Simplify tcpdump
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 Universi
Re: dhclient.c patch
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: [PATCH] uname, arch/machine -> %c, %a update in PKG_PATH
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
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
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: _PATH_SENDMAIL in lots of places outside of lpd stuff also
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: _PATH_SENDMAIL in lots of places outside of lpd stuff also
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, &sb))
> 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
>
Replace less(1)'s stdbool clone with the real McCoy
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", &parg);
}
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: less.h small cleanup
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.
Re: `ifstated -n' more useful error message
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
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
Re: Simplify tcpdump
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?
Simplify tcpdump
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
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 &ing 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
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
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
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)
Re: [PATCH] uname, arch/machine -> %c, %a update in PKG_PATH
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=1&query=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. > > >
Re: [patch] ls + utf-8 support
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.
Proper printf attribute in less(1)
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: gcc typo
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; > } > >
[less] start simplifying message buffer logic
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
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();
Simplify less(1) off_t formatting
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);
+ ap_off_t(linenum-
Re: [patch] kern/exec_script: return error when the shell name is not specified
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
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
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
Michael McConville wrote:
> Michael McConville wrote:
> > > 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.
>
> 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] nlist(3): out of bounds read
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: [patch] nlist(3): out of bounds read
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__(&ehdr) || fstat(fd, &st) < 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] kern/exec_script: avoid invalid free() in a case of error
Michael McConville wrote:
> > 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.
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] kern/exec_script: avoid invalid free() in a case of error
> 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: ksh.1: Remove $ERRNO reference
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&content-type=text/x-cvsweb-markup Committed. Thanks! bin/ksh/NOTES suggests that the sole motive for $ERRNO may have been its presence in AT&T 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 >
Re: [PATCH] vi remove custom perr from cl_main
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.
Remove redundant continue
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;
