Re: POLLHUP vs EVFILT_EXCEPT semantic

2021-10-22 Thread Todd C . Miller
On Fri, 22 Oct 2021 14:25:18 +0100, Martin Pieuchot wrote:

> Last year we added the new EVFILT_EXCEPT filter type to kqueue in
> order to report conditions currently available via POLLPRI/POLLRDBAND
> in poll(2) and select(2).
>
> This new filter has been implemented in tty and socket by re-using the
> existing kqueue's "read" filter.  This has a downside which is the filter
> will also trigger if any data is available for reading.
>
> This "feature" makes it impossible to correctly implement poll(2)'s
> "empty" condition mode.  If no bit are set in the `events' pollfd
> structure we still need to return POLLHUP.  But if the filter triggers
> when there's data to read, it means POLLIN not POLLHUP.
>
> So I'd like to change the existing EVFILT_EXCEPT filters to no longer
> fire if there is something to read.  Diff below does that and adds a
> new filter for FIFOs necessary for poll(2) support.

This looks good to me.  Separating out the OOB from the read filter
is a nice bonus.

 - todd



Re: patch: nullify v_data with NULL (and not with 0)

2021-10-19 Thread Todd C . Miller
On Tue, 19 Oct 2021 18:08:04 +1100, Jonathan Gray wrote:

> There are many others along those lines in the kernel, for example
> sparse complains about these in vfs_subr.c
>
> /sys/kern/vfs_subr.c:274:64: warning: Using plain integer as NULL pointer
> /sys/kern/vfs_subr.c:275:64: warning: Using plain integer as NULL pointer
> /sys/kern/vfs_subr.c:430:32: warning: Using plain integer as NULL pointer
> /sys/kern/vfs_subr.c:467:22: warning: Using plain integer as NULL pointer
> /sys/kern/vfs_subr.c:533:50: warning: Using plain integer as NULL pointer
> /sys/kern/vfs_subr.c:1150:77: warning: Using plain integer as NULL pointer
> /sys/kern/vfs_subr.c:1430:42: warning: Using plain integer as NULL pointer
> /sys/kern/vfs_subr.c:1483:19: warning: Using plain integer as NULL pointer
> /sys/kern/vfs_subr.c:1752:25: warning: Using plain integer as NULL pointer
>
> If this is something worth changing should it be a larger diff?

I think it is worth changing.  Using NULL in place of 0 makes the
code easier to parse for humans too.

 - todd



Re: lrint(3) and llrint(3) implementation

2021-10-14 Thread Todd C . Miller
On Thu, 14 Oct 2021 23:06:01 +0200, Mark Kettenis wrote:

> This doesn't work because of the hidden symbol madness.  The way
> things currently are we need one copy (s_lrint.c) with:
>
> DEF_STD(fn);
> LDBL_MAYBE_CLONE(fn);
>
> another version (s_lrintf.c) with
>
> DEF_STD(fn);
>
> and a final version (s_lrintl.c) without any magic.
>
> I suppose I could have the s_lrint.c and s_lrintf.c include
> s_lrintfl.c if you think that would be better?

Don't worry about it.  I forgot about the symbol hiding magic^Wmadness.

 - todd



Re: lrint(3) and llrint(3) implementation

2021-10-14 Thread Todd C . Miller
On Thu, 14 Oct 2021 01:15:56 +0200, Mark Kettenis wrote:

> Currently the lib/libm/msun/run-lrint_test regress fails on powerpc64
> and other platforms.  Our implementation came from NetBSD, but NetBSD
> switched to the implementation from FreeBSD some time ago.  That is
> the same implementation that we already use for lrintl(3) and
> llrintl(3).
>
> Diff below makes us use that implementation for lrint(3), lrintf(3),
> llrint(3) and llrintf(3) as well.  This makes the regress test pass on
> powerpc64.

Doesn't this mean we end up with three copies of what is essentially
the same code in s_lrint.c, s_lrintf.c and s_lrintl.c?

I know it's not much but why not just move the actual code to
s_lrint.c and include that in the others?

 - todd



Re: head(1): increase line count maximum to LLONG_MAX

2021-10-11 Thread Todd C . Miller
On Sun, 10 Oct 2021 19:13:55 -0500, Scott Cheloha wrote:

> This unifies the input maximums on 32-bit and 64-bit platforms.

This really feels like a solution in search of a problem.

 - todd



Re: head(1): validate all line count arguments

2021-10-10 Thread Todd C . Miller
On Sat, 09 Oct 2021 20:43:12 -0500, Scott Cheloha wrote:

> head(1) currently only validates the last count argument given.  I
> think we ought to be stricter.  You can specify the -n option an
> arbitrary number of times.

Yes, it is better to perform the check during argument processing.
OK millert@

 - todd



Re: less: merge upstream bugfixes

2021-10-09 Thread Todd C . Miller
On Sat, 09 Oct 2021 13:15:39 +0200, Tobias Stoeckmann wrote:

> this merges latest bugfixes from upstream to our version of less.
> No new features introduced. Upstream commits and issues are linked as
> references.

OK millert@

 - todd



Re: Variable type fix in parse.y (all of them)

2021-09-30 Thread Todd C . Miller
On Thu, 30 Sep 2021 22:15:39 +0200, Christian Weisgerber wrote:

> E.g. like this?

Yes, that looks correct.

However, this is a bit ugly:

if (pushback_index < MAXPUSHBACK-1)
return (unsigned char)(pushback_buffer[pushback_index++] = c);
else
return (EOF);

Maybe it would be clearer to do:

if (pushback_index < MAXPUSHBACK-1) {
pushback_buffer[pushback_index++] = c;
return (c);
}
return (EOF);

There's no need for an else clause.  You could even turn it around like:

if (pushback_index + 1 >= MAXPUSHBACK)
return (EOF);
pushback_buffer[pushback_index++] = c;
return (c);

 - todd



Re: Variable type fix in parse.y (all of them)

2021-09-30 Thread Todd C . Miller
On Thu, 30 Sep 2021 21:37:06 +0200, Christian Weisgerber wrote:

> Unfortunately that also affects the parsebuf/pushback_buffer complex
> used in some parser.y files.

That would require a few extra casts but it is straightforward.

 - todd



Re: Variable type fix in parse.y (all of them)

2021-09-30 Thread Todd C . Miller
On Thu, 30 Sep 2021 12:33:58 +0100, Stuart Henderson wrote:

> 
> revision 1.628
> date: 2013/11/25 12:52:45;  author: benno;  state: Exp;  lines: +7 -7;
> use u_char for buffers in yylex, for ctype calls
> found by millert@, ok deraadt@
> 

Thanks, I had forgotten about that.  The issue is that if we have
a char > 127, when that gets assigned to an int it will end up with
the wrong value.  Making the buffer unsigned was the least invasive
fix, though perhaps not the best one.

I think adding a cast for things like:

lungetc(*p);
...
c = *--p;

would actually make the intent clearer.  E.g.

lungetc((u_char)*p);
...
c = (u_char)*--p;

Since we use casts sparingly, when they are present they indicate
something you need to pay attention to.  That is not the case when
we simply change the type.

 - todd



Re: smtpd: srs and ruleset evaluation

2021-09-22 Thread Todd C . Miller
On Wed, 22 Sep 2021 15:46:13 +0200, Eric Faurot wrote:

> A user reported that decoded SRS addresses are not correctly evaluated
> against the ruleset. That's because the ruleset always matches against
> the expanded address ("dest") and not the original address ("rcpt").
> This diff should fix it.

Thanks for looking into this.  OK millert@

 - todd



Re: syslogd domain name

2021-09-18 Thread Todd C . Miller
On Fri, 17 Sep 2021 17:30:24 +0200, Alexander Bluhm wrote:

> The LocalDomain in syslogd(8) is not used, remove variable.
> Use RFC 5424 NILVALUE as fallback for LocalHostName.

OK millert@

 - todd



EVP_DigestInit.3: OpenSSL_add_all_digests() not needed

2021-09-10 Thread Todd C . Miller
The man page for OpenSSL_add_all_digests() says it is deprecated
and should not be called by user code.  However, EVP_DigestInit.3
says it must be used.

One of those is wrong...

 - todd

Index: lib/libcrypto/man/EVP_DigestInit.3
===
RCS file: /cvs/src/lib/libcrypto/man/EVP_DigestInit.3,v
retrieving revision 1.20
diff -u -p -u -r1.20 EVP_DigestInit.3
--- lib/libcrypto/man/EVP_DigestInit.3  5 Jan 2021 06:51:31 -   1.20
+++ lib/libcrypto/man/EVP_DigestInit.3  10 Sep 2021 16:30:23 -
@@ -506,9 +506,6 @@ return an
 .Vt EVP_MD
 structure when passed a digest name, a digest NID, or an ASN1_OBJECT
 structure respectively.
-The digest table must be initialized using, for example,
-.Xr OpenSSL_add_all_digests 3
-for these functions to work.
 .Pp
 .Fn EVP_MD_CTX_size ,
 .Fn EVP_MD_CTX_block_size ,
@@ -640,8 +637,6 @@ main(int argc, char *argv[])
const char mess2[] = "Hello World\en";
unsigned char md_value[EVP_MAX_MD_SIZE];
int md_len, i;
-
-   OpenSSL_add_all_digests();
 
if (argc <= 1) {
printf("Usage: mdtest digestname\en");



Re: Change vm_dsize to vsize_t

2021-09-08 Thread Todd C . Miller
On Tue, 07 Sep 2021 21:38:27 +0200, Mark Kettenis wrote:

> I'm not convinced the original diff is right:
>
> * We have several places in the kernel where we store numbers of pages
>   in a (32-bit) int.  Changing just one of these places is dangerous.
>
> * Changing the type of just vm_dsize makes no sense.  We should change
>   them all (but see the point above).
>
> * Does ASAN really need to reserve that much VA space?

The oddity here is that p_vm_dsize in kinfo_proc actually corresponds
to vm_dused, not vm_dsize.  So it is not actually the size of the
data segment alone.

Since uvmspace_dused() returns vsize_t it does seem like vm_dused
should be sized similarly.  As things stand, vm_dused could wrap.

 - todd



Re: Change vm_dsize to vsize_t

2021-09-07 Thread Todd C . Miller
On Tue, 07 Sep 2021 09:46:02 +0200, Claudio Jeker wrote:

> From my understanding this is not how struct kinfo_proc should be modified.
> Instead the code should add the u_int64_t version at the end and leave the
> old in place. This way old userland still works with new kernel.

Correct.

 - todd



Re: ASan checkpoint

2021-09-04 Thread Todd C . Miller
On Sat, 04 Sep 2021 11:36:33 +0200, Greg Steuck wrote:

> This brings me to the "what's next" part. Obviously the mmap
> interception needs to be fixed to get anywhere. MmapNamed invokes
> internal_mmap which is defined in sanitizer_openbsd.cpp as:
>
> uptr internal_mmap(void *addr, size_t length, int prot, int flags, int fd,
>u64 offset) {
>   return (uptr)mmap(addr, length, prot, flags, fd, offset);
> }
>
> This sadly can't work because mmap in sanitizer_common_interceptors.inc
> calls internal_mmap if !asan_inited and hence the endless
> recursion. Other systems have alternative ways of reaching the original
> libc mmap. E.g. directly calling mmap syscall in sanitizer_linux.cpp or
> calling __mmap in sanitizer_netbsd.cpp. Neither of these seems to be
> available on OpenBSD. So, what can we do to interpose mmap?

The simplest approach is probably to use _thread_sys_mmap() instead.
That is a strong alias for the hidden _libc_mmap() function.

 - todd



Re: mark getsubopt(3) as part of POSIX

2021-09-03 Thread Todd C . Miller
On Fri, 03 Sep 2021 16:51:06 +0200, Ingo Schwarze wrote:

> So i propose the larger patch shown below instead.

Also OK millert@

 - todd



Re: mark getsubopt(3) as part of POSIX

2021-09-03 Thread Todd C . Miller
On Fri, 03 Sep 2021 14:40:34 +0200, Theo Buehler wrote:

> Found this in my tree. Our version of getsubopt matches NetBSD's up to
> some DIAGASSERTs and they do mention POSIX in their manual, so I suspect
> we inherited the specified behavior. I copied the phrasing used for
> other functions, but haven't checked in detail.

OK millert@

 - todd



Re: timeout: Prettify man page and usage

2021-09-02 Thread Todd C . Miller
On Thu, 02 Sep 2021 11:05:24 +0200, Martijn van Duren wrote:

> There are a few cases where we don't set errno, but do use err(3).

OK millert@

 - todd



Re: diff(1)ing hardlinks

2021-09-01 Thread Todd C . Miller
On Wed, 01 Sep 2021 01:33:34 +0200, Alexander Hall wrote:

> If two files to be compared share the same inode, it should
> be reasonable to consider them identical.
>
> This gives a substantial speedup when comparing directory
> structures with many hardlinked files, e.g. when using
> rsnapshot for incremental backup.

OK millert@

 - todd



Re: sed(1): line addresses and next cycle

2021-08-16 Thread Todd C . Miller
On Sun, 15 Aug 2021 23:56:54 +0200, Andreas Kusalananda =?utf-8?B?S8OkaMOkcmk=?
= wrote:

> But note that this comes out of a discussion on how to do '0,/re/'
> addressing with OpenBSD sed.  Your changes appears to remove one way of
> actually handling a match of '/re/' on the first line without giving us
> another.  It would be better to have a clean way of doing the equivalent
> of '0,/re/' than to remove a way to do this.  Interestingly (?), the sed
> in plan9port works the same as our native sed.

As does AT sed on Solaris.  Since our sed currently matches the
behavior of the "canonical" sed, I don't see a reason to change it.

 - todd



Re: systat(1) iostat cumulative mode scaling issue

2021-08-14 Thread Todd C . Miller
On Wed, 28 Jul 2021 12:13:23 -0700, Anindya Mukherjee wrote:

> While running systat(1)'s iostat view in cumulative mode (activated by pressi
> ng
> 'b') I noticed that it divides the results by the refresh interval. This is
> because the cumulative mode is implemented by setting last = 0, and then it d
> oes
> (current - last) / etime, thereby giving wrong cumulative results. This can b
> e
> verified by comparing with `iostat -dI`. The attached diff fixes this.

Committed, thanks.  Sorry for the delay.

 - todd



Re: libedit: stop playing hopeless games with FIONBIO

2021-08-11 Thread Todd C . Miller
On Wed, 11 Aug 2021 18:50:10 +0200, Ingo Schwarze wrote:

> in its current state, the editline(3) library is completely unfit
> to work with non-blocking I/O.  Neither the API nor the implementation
> are even designed for that purpose.
>
> Consequently, i propose that we document the facts up front and
> simply delete some code that isn't going to do any good in practice.
> For programs not using non-blocking I/O on the editline input file
> descriptor, this patch makes no difference.  Programs that do set
> FIONBIO on a file descriptor and then call editline(3) on it anyway
> can't possibly work as intended as far as i can see.  Either setting
> FIONBIO is needless in the first place, or el_gets(3) clearing it
> will ruin the rest of the program's functionality.

Fine with me.

 - todd



Re: libedit: stop ignoring SIGINT

2021-08-09 Thread Todd C . Miller
On Mon, 09 Aug 2021 13:19:08 +0200, Ingo Schwarze wrote:

> Currently, when read(2) from the terminal gets interrupted by a
> signal, editline(3) ignores the (first) signal and unconditionally
> calls read(2) a second time.  That seems wrong: if the user hits
> Ctrl-C, it is sane to assume that they meant it, not that they
> want it ignored.
>
> The following patch causes el_gets(3) and el_wgets(3) to return
> failure when read(2)ing from the terminal is interrupted by a
> signal other than SIGCONT or SIGWINCH.  That allows the calling
> program to decide what to do, usually either exit the program or
> provide a fresh prompt to the user.

No objection from me.

 - todd



Re: time.3: miscellaneous cleanup and rewrites

2021-07-30 Thread Todd C . Miller
On Fri, 30 Jul 2021 13:01:21 -0500, Scott Cheloha wrote:

> NAME
>
> - get "the" time of day.

I don't like having "the" in there.  I'd suggest leaving this as
it is or simply shortening to either "get time" or "get the time".

> SYNOPSIS
>
> - Not a fan of the "tloc" variable name.  Use "now" as we do in other
>   pages to reinforce the meaning of its contents.

Sure.

> DESCRIPTION
>
> - Clean this up.  In particular, format the Epoch date like we do in
>   other pages.  We don't need two paragraphs, either.
>
> - Mention that we call gettimeofday(2).  Not strictly necessary, but
>   useful for people tracing system calls to know what the library
>   is doing.

This seems like the kind of implementation detail that shouldn't
be in the man page.  For instance, there's no reason why time(3)
couldn't be implemented via clock_gettime(2).  If someone is really
curious the source is available.

> Part of me wants to tell the reader not to use time() to measure
> elapsed time.  Then again, time() is low-res so it isn't misused
> nearly as often as gettimeofday(2).

Yeah, I wouldn't worry about it.

> I'm also leaning toward omitting the full description of the UTC clock
> and its behavior.  Not completely sure.

I think it is fine to have that info present.

> RETURN VALUES
>
> - There are no relevant error cases, remove them.
>
>   The underlying system call, gettimeofday(2), will not fail in this
>   context unless the stack is corrupted, which we cannot account for
>   anyway.
>
> - Just say that time() always succeeds.

Right, looks good.

> I cribbed this bit from another page to put something in the Return
> Values section, but the return value is already described in the
> Description.
>
> Maybe we don't need this section at all anymore?

I think it is useful to have the section.  I know I search by the
section to find this information.

> STANDARDS
>
> - This section is missing.  Our time() conforms to the latest
>   standard.

Great.

> HISTORY
>
> - Rework this a bit.
>
> - Note the fate of the historical time() system call.

Fine.

 - todd



Re: gettimeofday.2: miscellaneous cleanup and rewrites

2021-07-30 Thread Todd C . Miller
On Fri, 30 Jul 2021 11:48:27 -0500, Scott Cheloha wrote:

> Sure, changed to "get or set the time of day".
>
> FWIW, the clock_gettime(2) one-liner is "get or set the time".

Thanks.

> > Instead of saying that settimeofday ignores tz if not NULL why not
> > just say that settimeofday ignores tz.  Its value is not really
> > relevant.
>
> We can't say we ignore tz because we don't.  We still copyin() the
> timezone struct if tz is not NULL and set EFAULT if the copy fails.
> The contents are ignored, but the pointer given as argument is
> validated when it isn't NULL.
>
> We talked about ignoring the tz argument completely when we removed
> the timezone from the kernel but ultimately decided against it.

Ah, right, for EFAULT.  I misremembered the outcome of that discussion.

OK millert@

 - todd



Re: gettimeofday.2: miscellaneous cleanup and rewrites

2021-07-30 Thread Todd C . Miller
I really don't like "get or set the UTC time" in the NAME section.
It's not like we have a "get or set the local time" function.
I would prefer if you left the UTC information for the DESCRIPTION.

Instead of saying that settimeofday ignores tz if not NULL why not
just say that settimeofday ignores tz.  Its value is not really
relevant.

Otherwise looks good to me.

 - todd



Re: sleep.3: miscellaneous cleanup and rewrites

2021-07-29 Thread Todd C . Miller
On Tue, 27 Jul 2021 17:15:09 -0500, Scott Cheloha wrote:

> We do it for other library functions where some part of the
> functionality is implemented with a system call.  In other manpages we
> say something like:
>
> The
> .Fn foo
> function may fail and set
> .Va errno
> for any of the errors specified for the
> .Fn bar 2
> routine.

I think this is overkill.

> Then again, there's really only one error.  Maybe we should just
> duplicate the information in sleep.3.
>
> What about this?
>
> The
> .Fn sleep
> function sets
> .Va errno
> to
> .Dv EINTR
> if it is interrupted by the delivery of a signal.

Sure.

 - todd



Re: sleep.3: miscellaneous cleanup and rewrites

2021-07-26 Thread Todd C . Miller
On Mon, 26 Jul 2021 07:55:00 -0500, Scott Cheloha wrote:

> We don't know if it is still in the wild or not.
>
> There are lots of libc implementations.  We can't account for all of
> them.

Right.  We don't know who is still using a signal or itimer-based
sleep.

> > Being proscriptive in OpenBSD manual pages isn't going to stop someone
> > from creating the precise problem you describe in some other body of
> > code.  Except, you are saying they don't create that problem.
>
> Right, we can't prevent it, which is why I wanted to say "beware".

POSIX doesn't allow a SIGALRM-based implementation when threads are
used anyway.  A future version of POSIX will likely make a SIGALRM-based
implementation non-conforming event for single-threaded processes.

> > So why foam at the mouth over it?
>
> Because up until this very moment we referenced the historical
> implementation approach in the manpage.

The 4.4BSD man page had even more detail about this :-)

> Anyway, updated patch.  No mention of SIGALRM or alarm(3) except in
> the History section.

I think this is OK for now.

> Still wondering whether we need an Errors section to mention that
> sleep(3) can set errno.

POSIX doesn't define any errors.  Of the errors returned by nanosleep,
only EINTR is really possible.  I guess we could save & restore
errno in sleep.c but I don't see any implementations that actually
do that.  I only checked illumos, glibc and musl though.

 - todd



Re: fstat: remove setpassent leftovers

2021-07-17 Thread Todd C . Miller
On Sat, 17 Jul 2021 03:48:21 -, Klemens Nanni wrote:

> Should've gone with 
>
>   revision 1.95
>   date: 2018/09/16 02:44:06;  author: millert;  state: Exp;  lines: +10 -
> 5;
>   Use uid_from_user(3) and gid_from_group(3) in utilities that
>   do repeated lookups.  OK tb@

OK millert@

 - todd



Re: cron(8): add '@' interval time specifier

2021-07-11 Thread Todd C . Miller
On Sat, 10 Jul 2021 15:54:51 -, Job Snijders wrote:

> > > I borrowed the idea from FreeBSD's cron [1]. A difference between
> > > the below changeset and the freebsd implementation is that they
> > > specify the interval in seconds, while the below specifies in
> > > minutes.
> > 
> > Why be incompatible?  Better have a strong justification.
>
> OpenBSD cron would need to be extended to support 'up to 1 second
> precise' scheduling, perhaps some code can be borrowed from 
> https://github.com/freebsd/freebsd-src/commit/7a5c30c5b67555c9f76a053812ba80a
> e258b8874#diff-f8f4ba67b7df93eb4b436203d0c70670560029bd8c5b9a642691a914aa33d4
> 00
> to accomplish this.

We use ppoll(2) when waiting for the next event so it should be
simple to add support for sub-minute resolution.  Our code differs
greatly from FreeBSD's in this way.

> Alternatively, I can remove the below '* SECONDS_PER_MINUTE' multiplier,
> so that from a syntax perspective we are equivalent to FreeBSD, but our
> cron will have 'one minute precision' regarding when the job is
> executed.

I think maintaining compatibility with FreeBSD is the way to go
since that is where this feature originates.  We can add sub-minute
precision separately.

> entry.c:
> + } else if (*cmd != '\0' &&
> + (interval = strtol(cmd, , 10)) > 0 &&
> + *endptr == '\0') {
> + e->interval = interval * SECONDS_PER_MINUTE;
> + e->flags |= INTERVAL | SINGLE_JOB;
>
> For me 'one minute' precision is good enough, but if others think it is
> worth putting in the effort to get to '1 second' precision scheduling
> I'm happy to try to hack it.

Currently, cron_sleep() takes a target time in minutes but it doesn't
have to be that way.  We could set the target time based the first
job in the queue or now + 1 minute, whichever is smaller.

 - todd



Re: Add f_modify and f_process callbacks to FIFO filterops

2021-07-11 Thread Todd C . Miller
On Sun, 11 Jul 2021 14:27:00 -, Visa Hankala wrote:

> This patch adds f_modify and f_process callbacks to FIFO event filters.
> The general idea is the same as with sockets, discussed in thread
> "Add f_modify and f_process callbacks to socket filterops" on tech@.
>
> I think it is best to add the callbacks now, before further changes to
> socket locking.

This matches the socket changes and looks correct to me.
OK millert@

 - todd



Re: Correct minor lie in re_format(7)

2021-07-06 Thread Todd C . Miller
On Tue, 06 Jul 2021 11:01:06 +0200, Martijn van Duren wrote:

> There are equivalents for '+' and '?' in BRE.

OK millert@

 - todd



Re: ksh: require expression in while loop

2021-07-05 Thread Todd C . Miller
On Mon, 05 Jul 2021 16:30:49 +0959, Reuben ua =?UTF-8?Q?Br=C3=AD=C4=A1?= wrote:

> if i might suggest a slight variation, how about only requiring that
> at least one of the lists is non-empty, in the case of while, and
> leaving for, until, etc. as they are?
>
> this way nothing is broken.
>
> your diff might then look something like:
>
>   t->left = c_list(true);
>   t->right = dogroup();
> + if (t->left == NULL && t->right == NULL)
> + syntaxerr(NULL);

That would result in an error like:

ksh: syntax error: `done' unexpected

instead of:

ksh: syntax error: `do' unexpected

But perhaps this is not important.  It is also possible to call
yyerror() directly to display the error.

 - todd



Re: ksh: require expression in while loop

2021-07-05 Thread Todd C . Miller
On Mon, 05 Jul 2021 15:08:27 +0200, Jeremie Courreges-Anglas wrote:

> LGTM as a first step, ok jca@

Committed.

> I'd prefer to fix the odd uses we have in tree and end up with your
> initial diff in a third step.
>
> Here are the three cases pointed out by halex@, mechanical diff.
>
> ok? / Todd, feel free to pick it up with my ok.

OK millert@

 - todd



Re: ksh: require expression in while loop

2021-07-05 Thread Todd C . Miller
On Mon, 05 Jul 2021 14:47:08 +0200, Alexander Hall wrote:

> Please note that I'm not really opposing the initial suggestion per se. I'm n
> ot sure "do done" is even valid in ksh93.

It is not.

> Nowadays I try to avoid "do done", and my personal scripts breaking I can han
> dle. I was just pointing out that it would break some of the ones in-tree if 
> we don't fix them first.

Yes, we should fix the in-tree scripts to avoid creating a portability
trap.

 - todd



Re: ksh: require expression in while loop

2021-07-04 Thread Todd C . Miller
On Sun, 04 Jul 2021 23:25:25 +0200, Alexander Hall wrote:

> The "... do done" variant has been frequently used by me, and seems to appear
>  at least three times in install.sub, so if this goes in, please scan the scr
> ipts in our tree first, at least for trivial cases.

Fair enough, let's just require a non-empty expression but still
allow an empty loop body.  It would be a good idea to call this out
as a portability issue in the ksh manual but that can be done
separately.

 - todd

Index: bin/ksh/syn.c
===
RCS file: /cvs/src/bin/ksh/syn.c,v
retrieving revision 1.39
diff -u -p -u -r1.39 syn.c
--- bin/ksh/syn.c   24 Apr 2018 08:25:16 -  1.39
+++ bin/ksh/syn.c   4 Jul 2021 22:21:39 -
@@ -331,6 +331,8 @@ get_command(int cf)
nesting_push(_nesting, c);
t = newtp((c == WHILE) ? TWHILE : TUNTIL);
t->left = c_list(true);
+   if (t->left == NULL)
+   syntaxerr(NULL);
t->right = dogroup();
nesting_pop(_nesting);
break;



Re: ksh: require expression in while loop

2021-07-02 Thread Todd C . Miller
Updated diff that also requires a non-empty command list for "for"
loops and adjusts the NOTES file to match.

 - todd

Index: bin/ksh/NOTES
===
RCS file: /cvs/src/bin/ksh/NOTES,v
retrieving revision 1.16
diff -u -p -u -r1.16 NOTES
--- bin/ksh/NOTES   12 Jan 2018 14:20:57 -  1.16
+++ bin/ksh/NOTES   2 Jul 2021 18:45:11 -
@@ -195,10 +195,6 @@ Known differences between pdksh & at k
 - co-processes: if ksh93, the write portion of the co-process output is
   closed when the most recently started co-process exits. pdksh closes
   it when all the co-processes using it have exited.
-- pdksh accepts empty command lists for while and for statements, while
-  at ksh (and sh) don't.  Eg., pdksh likes
-   while false ; do done
-  but ksh88 doesn't like it.
 - pdksh bumps RANDOM in parent after a fork, at ksh bumps it in both
   parent and child:
RANDOM=1
Index: bin/ksh/syn.c
===
RCS file: /cvs/src/bin/ksh/syn.c,v
retrieving revision 1.39
diff -u -p -u -r1.39 syn.c
--- bin/ksh/syn.c   24 Apr 2018 08:25:16 -  1.39
+++ bin/ksh/syn.c   2 Jul 2021 18:44:39 -
@@ -323,6 +323,8 @@ get_command(int cf)
nesting_push(_nesting, c);
t->vars = wordlist();
t->left = dogroup();
+   if (t->left == NULL)
+   syntaxerr(NULL);
nesting_pop(_nesting);
break;
 
@@ -331,7 +333,11 @@ get_command(int cf)
nesting_push(_nesting, c);
t = newtp((c == WHILE) ? TWHILE : TUNTIL);
t->left = c_list(true);
+   if (t->left == NULL)
+   syntaxerr(NULL);
t->right = dogroup();
+   if (t->right == NULL)
+   syntaxerr(NULL);
nesting_pop(_nesting);
break;
 



ksh: require expression in while loop

2021-07-02 Thread Todd C . Miller
Currently, our ksh/sh accepts things like:

while do done

which cannot be interrupted via ^C by default.

If, However, the expression is not empty everything is fine.  E.g.

while :; do done

Most other shells require a non-empty expression which avoids this
problem (zsh is the outlier here).  AT ksh and other shells also
require a non-empty loop body so I've added a check for that too.
So now the expression has to be at least:

while :; do :; done

There are other places we could tighten up the syntax checking but
I'll leave that for someone else ;-)

The new behavior is:

$ while do done
ksh: syntax error: `do' unexpected

$ while :; do done
ksh: syntax error: `done' unexpected

Which is similar to the error produced by bash and AT ksh.
The ksh regress still passes.

 - todd

Index: bin/ksh/syn.c
===
RCS file: /cvs/src/bin/ksh/syn.c,v
retrieving revision 1.39
diff -u -p -u -r1.39 syn.c
--- bin/ksh/syn.c   24 Apr 2018 08:25:16 -  1.39
+++ bin/ksh/syn.c   2 Jul 2021 18:14:21 -
@@ -331,7 +331,11 @@ get_command(int cf)
nesting_push(_nesting, c);
t = newtp((c == WHILE) ? TWHILE : TUNTIL);
t->left = c_list(true);
+   if (t->left == NULL)
+   syntaxerr(NULL);
t->right = dogroup();
+   if (t->right == NULL)
+   syntaxerr(NULL);
nesting_pop(_nesting);
break;
 



Re: usleep(3): always call nanosleep(2)

2021-07-02 Thread Todd C . Miller
On Fri, 02 Jul 2021 10:53:23 -0500, Scott Cheloha wrote:

> As with sleep(3), usleep(3) is a wrapper around nanosleep(2).  I'd
> prefer it if we always call nanosleep(), even when the input is zero.
>
> This makes the code simpler, makes reasoning about behavior simpler,
> and guarantees you get hit the ktrace for nanosleep if the program
> calls usleep().

OK millert@

 - todd



Re: ualarm.3: cleanup

2021-07-01 Thread Todd C . Miller
On Thu, 01 Jul 2021 08:14:51 -0600, "Todd C. Miller" wrote:

> Aha, I see.  It is because delay can be a float.

Here's a diff to use nanosleep/setitimer instead of usleep/ualarm.
Is it worth it?

 - todd

Index: usr.bin/systat/engine.c
===
RCS file: /cvs/src/usr.bin/systat/engine.c,v
retrieving revision 1.28
diff -u -p -u -r1.28 engine.c
--- usr.bin/systat/engine.c 2 Jun 2021 08:32:22 -   1.28
+++ usr.bin/systat/engine.c 1 Jul 2021 14:49:41 -
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 /* XXX These are defined in term.h and conflict with our variable names */
@@ -50,7 +51,9 @@ struct view_ent {
TAILQ_ENTRY(view_ent) entries;
 };
 
-useconds_t udelay = 500;
+static struct timespec ts_delay = { 5, 0 };
+static struct itimerval it_delay = { { 0, 0 }, { 5, 0 } };
+
 int dispstart = 0;
 int humanreadable = 0;
 int interactive = 1;
@@ -1355,7 +1358,7 @@ engine_loop(int countmax)
read_view();
need_sort = 1;
gotsig_alarm = 0;
-   ualarm(udelay, 0);
+   setitimer(ITIMER_REAL, _delay, NULL);
}
 
if (need_sort) {
@@ -1366,7 +1369,7 @@ engine_loop(int countmax)
/* XXX if sort took too long */
if (gotsig_alarm) {
gotsig_alarm = 0;
-   ualarm(udelay, 0);
+   setitimer(ITIMER_REAL, _delay, NULL);
}
}
 
@@ -1408,7 +1411,7 @@ engine_loop(int countmax)
if (interactive && need_update == 0)
keyboard();
else if (interactive == 0)
-   usleep(udelay);
+   nanosleep(_delay, NULL);
}
 
if (rawmode == 0)
@@ -1456,4 +1459,17 @@ check_termcap(void)
return(1);
 
return(0);
+}
+
+void
+refresh_delay(double delay)
+{
+   double secs, frac;
+
+   frac = modf(delay, );
+   ts_delay.tv_sec = secs;
+   ts_delay.tv_nsec = frac * 10.0;
+   if (!timespecisset(_delay))
+   ts_delay.tv_nsec = 10;
+   TIMESPEC_TO_TIMEVAL(_delay.it_value, _delay);
 }
Index: usr.bin/systat/engine.h
===
RCS file: /cvs/src/usr.bin/systat/engine.h,v
retrieving revision 1.13
diff -u -p -u -r1.13 engine.h
--- usr.bin/systat/engine.h 2 Jun 2021 08:32:22 -   1.13
+++ usr.bin/systat/engine.h 1 Jul 2021 14:30:40 -
@@ -145,6 +145,7 @@ void show_order(void);
 
 void setup_term(int maxpr);
 int check_termcap(void);
+void refresh_delay(double delay);
 
 void engine_initialize(void);
 void engine_loop(int countmax);
@@ -156,7 +157,6 @@ const char *message_set(const char *msg)
 void foreach_view(void (*callback)(field_view *));
 
 extern int sortdir;
-extern useconds_t udelay;
 extern int dispstart;
 extern int humanreadable;
 extern int interactive;
Index: usr.bin/systat/main.c
===
RCS file: /cvs/src/usr.bin/systat/main.c,v
retrieving revision 1.74
diff -u -p -u -r1.74 main.c
--- usr.bin/systat/main.c   2 Jun 2021 08:32:22 -   1.74
+++ usr.bin/systat/main.c   1 Jul 2021 14:43:30 -
@@ -332,7 +332,7 @@ cmd_delay(const char *buf)
if (errstr != NULL)
error("s: \"%s\": delay value is %s", buf, errstr);
else {
-   udelay = (useconds_t)(del * 100);
+   refresh_delay(del);
gotsig_alarm = 1;
naptime = del;
}
@@ -557,11 +557,8 @@ main(int argc, char *argv[])
errx(1, "\"%s\": delay value is %s", argv[1], errstr);
}
 
-   udelay = (useconds_t)(delay * 100.0);
-   if (udelay < 1)
-   udelay = 1;
-
-   naptime = (double)udelay / 100.0;
+   refresh_delay(delay);
+   naptime = delay;
 
gethostname(hostname, sizeof (hostname));
gethz();



Re: ualarm.3: cleanup

2021-07-01 Thread Todd C . Miller
On Thu, 01 Jul 2021 08:10:49 -0600, "Todd C. Miller" wrote:

> It looks like systat is the only consumer of ualarm(3) in the tree.
> I don't think it even makes sense there since the actual delay is
> in seconds, not microseconds.  The change to ualarm/usleep was made
> by canacar@ in 2008.  I don't see any reason why it cannot use
> alarm/sleep like it used to but I've only taken a quick look.

Aha, I see.  It is because delay can be a float.

 - todd



Re: ualarm.3: cleanup

2021-07-01 Thread Todd C . Miller
On Wed, 30 Jun 2021 17:24:28 -0600, "Theo de Raadt" wrote:

> Scott Cheloha  wrote:
>
> > FWIW, the manpage has literally always led with this bolded
> > implementation detail.  From January of 1986:
> > 
> > https://svnweb.freebsd.org/csrg/lib/libc/gen/ualarm.3?revision=25789=m
> arkup
>
> I'm just saying it is uninformative and distracting.

I think we should actively discourage the use of ularm(3) and tell
people to use setitimer(2) instead.  It was removed from POSIX which
makes it a liability in terms of portability.  E.g.

This interface is obsoleted by setitimer(2).

It looks like systat is the only consumer of ualarm(3) in the tree.
I don't think it even makes sense there since the actual delay is
in seconds, not microseconds.  The change to ualarm/usleep was made
by canacar@ in 2008.  I don't see any reason why it cannot use
alarm/sleep like it used to but I've only taken a quick look.

 - todd



Re: ualarm.3: cleanup

2021-06-30 Thread Todd C . Miller
On Thu, 01 Jul 2021 00:14:12 +0200, Ingo Schwarze wrote:

> Can the current wording below CAVEATS, "confusing behavior", be made
> more precise, explaining what actually happens rather than merely
> calling it "confusing"?  For example, saying that they all override
> each other or something like that?  Or saying that behavior is
> undefined when more than one of them is used (isn't that what POSIX
> says)?

Yes, POSIX says interactions between them are unspecified.

 - todd



Re: ualarm.3: cleanup

2021-06-30 Thread Todd C . Miller
On Thu, 01 Jul 2021 00:14:12 +0200, Ingo Schwarze wrote:

> I'm confused.  It seems i fail to find useconds_t in POSIX.
> I only see suseconds_t there, in , which must be signed,
> include [-1, 100], and not be wider than long.  Also, our manual
> says ualarm(3) is -xpg4.2.  That's basically -susv1, isn't it?
> So i would expect ualarm(3) to appear in modern POSIX, too.
> Was it deleted at some point?

Yes, it was removed in -susv4.

> I think it would be ideal for the manual page text to state
> which values can be used in a *portable* way.  If, for some reason,
> that is not desirable, then the main text should say which values
> can be used on OpenBSD, and STANDARDS should say that using values
> above XYZ is an extension.

The standard didn't ever specify things in that level of detail as
far as I can tell.

 - todd



Re: smtpd: unnecessary "no certificate presented" log message

2021-06-30 Thread Todd C . Miller
On Wed, 30 Jun 2021 14:37:44 +0200, Eric Faurot wrote:

> Except for specific cases, SMTP servers do not expect client
> certificates for TLS sessions. The log message for missing certificate
> is not very useful in practice (handshake fails before if it was
> required anyway), and it is even confusing for people.
> I think it can go away.

OK millert@

 - todd



Re: recvmsg returns MSG_DONTWAIT

2021-06-27 Thread Todd C . Miller
On Sun, 27 Jun 2021 15:59:48 -, Klemens Nanni wrote:

> On Sun, Jun 27, 2021 at 05:28:09PM +0200, Mark Kettenis wrote:
> > I think this points out that diff wasn't quite right.  I mean,
> > changing the man page doesn't fix the Haskell test does it?
>
> Of course it doesn't fix the test but it documents the status quo so
> kernel and test hackers can actually use the manual, possibly helping
> them adjust recv(2) semantics and/or tests.

Please don't.  This should be treated as a bug and not just documented.

 - todd



Re: recvmsg returns MSG_DONTWAIT

2021-06-27 Thread Todd C . Miller
On Sun, 27 Jun 2021 18:16:34 +0200, Claudio Jeker wrote:

> On Sun, Jun 27, 2021 at 05:28:09PM +0200, Mark Kettenis wrote:
> > > Date: Sun, 27 Jun 2021 13:36:03 +
> > > From: Klemens Nanni 
> > > 
> > > On Sat, Jun 12, 2021 at 11:54:58PM -0700, Greg Steuck wrote:
> > > > I started with a failing test for Haskell network package on 6.9-curren
> t amd64
> > > > (cabal get network-3.1.2.1 && cabal v2-test)
> > > > 
> > > > network-3.1.2.1/build/spec/spec --match "/Network.Socket.ByteString/rec
> vMsg/works well/"
> > > > 
> > > >   tests/Network/Socket/ByteStringSpec.hs:209:21: 
> > > >   1) Network.Socket.ByteString.recvMsg works well
> > > >expected: MsgFlag {fromMsgFlag = 0}
> > > > but got: MsgFlag {fromMsgFlag = 128}
> > > > 
> > > > ktrace says:
> > > > 
> > > >  47649 spec CALL  sendto(14,0x8a2126e838c,0x16,0,0x8a4a3622df0,0x10
> )
> > > >  47649 spec STRU  struct sockaddr { AF_INET, 127.0.0.1:9486 }
> > > >  47649 spec GIO   fd 14 wrote 22 bytes
> > > >"This is a test message"
> > > >  47649 spec RET   sendto 22/0x16
> > > >  47649 spec CALL  futex(0x8a4aef6f930,0x81 _FLAG>,1,0,0)
> > > >  47649 spec STRU  struct kevent { ident=13, filter=EVFILT_READ, fla
> gs=0x11, fflags=0<>, data=38, udata=0x0 }
> > > >  47649 spec RET   kevent 1
> > > >  47649 spec CALL  recvmsg(13,0x8a4a3622c50,0)
> > > >  47649 spec GIO   fd 13 read 22 bytes
> > > >"This is a test message"
> > > >  47649 spec STRU  struct sockaddr { AF_INET, 127.0.0.1:12293 }
> > > >  47649 spec STRU  struct msghdr { name=0x8a4a3622b70, namelen=16, i
> ov=0x8a4a3622c30, iovlen=1, control=0x8a4a3622c10, controllen=0, flags=0x80 SG_DONTWAIT> }
> > > >  47649 spec STRU  struct iovec { base=0x8a4a3622666, len=1002 }
> > > >  47649 spec RET   recvmsg 22/0x16
> > > > 
> > > > This seems to contradict recvmsg(2) which doesn't list MSG_DONTWAIT as 
> a
> > > > possible value of the flags. Would this be useful as a C regress test?
> > > 
> > > Looks like this was missed in sys/kern/uipc_syscalls.c revision 1.178
> > > 
> > >   date: 2018/07/30 12:22:14;  author: mpi;  state: Exp;  lines: +10 -18; 
>  commitid: K43aQe66cQkEOSbc;
> > >   Use FNONBLOCK instead of SS_NBIO to check/indicate that the I/O mode
> > >   for sockets is non-blocking.
> > > 
> > >   This allows us to G/C SS_NBIO.  Having to keep the two flags in sync
> > >   in a mp-safe way is complicated.
> > > 
> > >   This change introduce a behavior change in sosplice(), it can now
> > >   always block.  However this should not matter much due to the socket
> > >   lock being taken beforhand.
> > > 
> > >   bluhm@, benno@, visa@
> > > 
> > > Wording taken from mpi's commit message which is exactly what recvit()
> > > does in one place, i.e.
> > > 
> > >   if (fp->f_flag & FNONBLOCK)
> > >   mp->msg_flags |= MSG_DONTWAIT;
> > > 
> > > 
> > > Here's the documentation bits for it?
> > > Feedback? Objections? OK?
> > 
> > I think this points out that diff wasn't quite right.  I mean,
> > changing the man page doesn't fix the Haskell test does it?
>
> I agree, I think MSG_DONTWAIT should not be returend to userland.

Agreed, it should be cleared before return to userland.

 - todd



Re: rdist(1): reduce usage()

2021-06-22 Thread Todd C . Miller
On Tue, 22 Jun 2021 13:56:46 +0100, Jason McIntyre wrote:

> this one is more problematic. it uses two functions, getdistoptlist and
> msgprusage, which i think are only used within usage. so i killed them
> too.

OK millert@

 - todd



Re: locate(1): reduce usage()

2021-06-22 Thread Todd C . Miller
On Tue, 22 Jun 2021 13:49:35 +0100, Jason McIntyre wrote:

> diff to reduce verbosity in locate(1) usage. before:

OK millert@

 - todd



Re: ypmatch(1): reduce usage()

2021-06-22 Thread Todd C . Miller
On Tue, 22 Jun 2021 13:44:27 +0100, Jason McIntyre wrote:

> diff to reduce verbosity in ypmatch(1) usage. before:

OK millert@

 - todd



Re: crontab(1): reduce usage()

2021-06-22 Thread Todd C . Miller
On Tue, 22 Jun 2021 13:42:19 +0100, Jason McIntyre wrote:

> diff to reduce verbosity in crontab(1) usage. before:

OK millert@

 - todd



Re: disklabel(8): reduce usage()

2021-06-22 Thread Todd C . Miller
On Tue, 22 Jun 2021 13:34:43 +0100, Jason McIntyre wrote:

> diff to reduce verbosity in disklabel(8) usage. before:

OK.  Should we also remove SEEALSO from sbin/disklabel/Makefile
and distrib/special/disklabel/Makefile?

 - todd



Re: can alarm(3) fail?

2021-06-20 Thread Todd C . Miller
On Sat, 19 Jun 2021 17:15:43 +0200, Alexander Bluhm wrote:

> In regress is some code
> if ((int)alarm(10) == -1)
> or
> if (alarm(30) == (unsigned int)-1)
>
> The man page alarm(3) says "If an error occurs, the value -1 is
> returned".  Such things could be written.
>
> Should we avoid to return UINT_MAX in cases if there was no error?
> Can that happen now?
>
> Or should we just remove the error handling in regress?

You should remove the error handling in regress.

 - todd



Re: alarm.3: misc. cleanup, rewriting

2021-06-18 Thread Todd C . Miller
On Fri, 18 Jun 2021 16:27:36 -0500, Scott Cheloha wrote:

> Maybe this?
>
> The
> .Fn alarm
> function schedules the
> .Dv SIGALRM
> signal for delivery to the calling process after the given number of
> .Fa seconds
> have elapsed.
>
> I've also added a CAVEATS section.
>
> I've also tweaked the .Nd summary:
>
> .Nd schedule SIGALRM delivery

That looks good to me.

 - todd



Re: can alarm(3) fail?

2021-06-18 Thread Todd C . Miller
On Fri, 18 Jun 2021 15:22:05 -0600, "Theo de Raadt" wrote:

> Todd C. Miller  wrote:
>
> > On Fri, 18 Jun 2021 15:17:47 -0600, "Theo de Raadt" wrote:
> > 
> > > OK.  How any pieces of code were found which do that?
> > >
> > > I mean, code search for ' = alarm('
> > 
> > In our tree?  Just ksh.
> > 
> > static void
> > alarm_catcher(int sig)
> > {
> > int errno_ = errno;
> > 
> > if (ksh_tmout_state == TMOUT_READING) {
> > int left = alarm(0);
> > 
> > if (left == 0) {
> > ksh_tmout_state = TMOUT_LEAVING;
> > intrsig = 1;
> > } else
> > alarm(left);
> > }
> > errno = errno_;
> > }
>
> Would the proposal to return UINT_MAX not break this code (which
> stores it in int), and does the current return value of 0 not
> actually make this code do the right thing?

Yes :-)

 - todd



Re: can alarm(3) fail?

2021-06-18 Thread Todd C . Miller
On Fri, 18 Jun 2021 15:17:47 -0600, "Theo de Raadt" wrote:

> OK.  How any pieces of code were found which do that?
>
> I mean, code search for ' = alarm('

In our tree?  Just ksh.

static void
alarm_catcher(int sig)
{
int errno_ = errno;

if (ksh_tmout_state == TMOUT_READING) {
int left = alarm(0);

if (left == 0) {
ksh_tmout_state = TMOUT_LEAVING;
intrsig = 1;
} else
alarm(left);
}
errno = errno_;
}

 - todd



Re: can alarm(3) fail?

2021-06-18 Thread Todd C . Miller
On Fri, 18 Jun 2021 15:05:52 -0600, "Theo de Raadt" wrote:

> Without considering the cases where an incorrect value is passed in...
>
> How many pieces of code have you found that inspect the return value?

Very few.  My concern is for those that use the return value when
determining how much time is "left" to sleep.  E.g.

u_int sleep_time = 300;
u_int slept;

while (sleep_time > 0) {
slept = alarm(sleep_time);
sleep_time -= slept;
}

If alarm() spuriously returns UINT_MAX that would be unhelpful.  Of
course, if alarm() _always_ fails for some reason and returns 0 on
error that is probably just as bad if not worse. :-)

So perhaps it doesn't really matter either way.

 - todd



Re: can alarm(3) fail?

2021-06-18 Thread Todd C . Miller
On Fri, 18 Jun 2021 16:02:20 -0500, Scott Cheloha wrote:

> If we're going to just return an arbitrary value shouldn't we keep
> returning UINT_MAX as we always have?  This would preserve compat with
> the other BSD libc implementations, too.

I think that a return value of 0 is less likely to cause actual
harm but if you'd prefer to use UINT_MAX that is OK with me.

> I'd still like to remove the superfluous pointer (itp), but otherwise
> there's nothing else we can improve here.

Sure, there's no need for itp.

 - todd



Re: alarm.3: misc. cleanup, rewriting

2021-06-18 Thread Todd C . Miller
On Fri, 18 Jun 2021 12:02:18 -0600, "Theo de Raadt" wrote:

> But I dislike this text since it doesn't speak to the underlying
> mechanism.
>
> The
> .Fn alarm
> function configures the process
> .Va ITIMER_REAL
> interval timer with
> .Xr setitimer 2
> to deliver a
> .Va SIGALRM
> signal in the future.
>
> (This also hints that parallel use of the underlying mechanism,
> ITIMER_REAL, could break expectations.)

This is an implementation detail that is not reflected by the
standard.  Historically alarm() was a system call and there are
still OSes where that is the case.

On the other hand, it is good to document that on OpenBSD trying
to use both alarm() and ITIMER_REAL would interact in surprising
ways.

To me, this seems like information that belongs in a CAVEAT section.

 - todd



Re: can alarm(3) fail?

2021-06-18 Thread Todd C . Miller
On Fri, 18 Jun 2021 12:13:54 -0600, "Theo de Raadt" wrote:

> I don't understand what you are solving.
>
> The way I look at it... you want to convert one kind of bug into a
> different kind of bug?
>
> In the end, the program quits, noone looks at the corefile, or is it
> in a privsep program and there is no corefile, and noone is the wiser
> and it never gets fixed.

The problem is that alarm(3) is not allowed to fail and so there
is no standard way to check for failure if one were to occur.  But
we either have to return *something* in this case or abort the
process.

Personally, I think just returning either 0 or UINT_MAX in this
case is fine.  I lean toward returning 0 in this case which is what
musl and glibc do.

 - todd



Re: can alarm(3) fail?

2021-06-18 Thread Todd C . Miller
On Tue, 15 Jun 2021 20:22:53 -0500, Scott Cheloha wrote:

> After that is committed, I'm unclear whether alarm(3) can fail anymore
> on OpenBSD.  The only remaining plausible failure case I can see is
> EFAULT.  But I'm unsure if that's possible.  Can we get an EFAULT if
> both arguments point to stack memory?

I don't think it is possible for alarm(3) to fail in this case.
The only way I see it failing is if the stack gets corrupted.  I
think we can just initialize oitv to { 0 } and ignore the setitimer()
return value.

 - todd



Re: ftpd(8): Convert K function definitions to modern C

2021-05-30 Thread Todd C . Miller
On Sun, 30 May 2021 17:47:34 +0200, Jan Klemkow wrote:

> Convert K function definitions to modern C.

OK millert@

 - todd



Re: libm: implicit double to long conversion warnings

2021-05-27 Thread Todd C . Miller
On Thu, 27 May 2021 16:50:16 +0200, Theo Buehler wrote:

> The fix from FreeBSD silences both of them and looks correct to me.

OK millert@

 - todd



Re: smtpd: includes cleanup

2021-05-27 Thread Todd C . Miller
On Thu, 27 May 2021 13:14:30 +0200, Eric Faurot wrote:

> New diff with small tweaks.

It looks like you are relying on sys/queue.h being included implicitly.
Since smtpd.h uses the TAILQ macros, should it include sys/queue.h
itself?

 - todd



Re: smtpd: err/errx -> fatal/fatalx

2021-05-26 Thread Todd C . Miller
On Wed, 26 May 2021 16:24:42 +0200, Eric Faurot wrote:

> This diff replaces calls to err(3)/errx(3) with fatal()/fatalx() from
> log.c for code that runs in the deamon (we want errors logged to
> syslog, not stderr).  It's pretty mechanical, with two things to note:
>
> The call to default_config() has to be done after log_init() in smtpd.c,
> and log_init() is called early in smtpctl.c, since it uses files (iobuf.c)
> that uses the log api. It still logs to stderr though.

OK millert@

 - todd



Re: smtpd: unused code

2021-05-25 Thread Todd C . Miller
On Tue, 25 May 2021 22:50:32 +0200, Eric Faurot wrote:

> This diff removes more unused code.

OK millert@

 - todd



Re: citrus_none: don't allow codepoints > 0x7f

2021-05-07 Thread Todd C . Miller
On Fri, 07 May 2021 16:47:19 +0200, Martijn van Duren wrote:

> ping

Restricting things to 7-bit ASCII in the C/POSIX locale seems like
the correct thing to do.  OK millert@

 - todd



Re: printf(1): support \u and \U

2021-05-07 Thread Todd C . Miller
On Fri, 07 May 2021 16:39:34 +0200, Martijn van Duren wrote:

> Any takers?
>
> Updated diff after previous commit below.

OK millert@

 - todd



Re: patch: make ld.so aware of pthread_key_create destructor - Re: multimedia/mpv debug vo=gpu crash on exit

2021-05-06 Thread Todd C . Miller
On Thu, 06 May 2021 09:32:28 +0200, Sebastien Marie wrote:

> We already take care of such situation with __cxa_thread_atexit_impl
> (in libc/stdlib/thread_atexit.c), by keeping an additionnal reference
> on object loaded (it makes ld.so aware that it is still used and so
> dlclose() doesn't unload it).
>
> I used the same idiom for pthread_key_create() and used dlctl(3) in
> the same way with the destructor address.

This will set STAT_NODELETE so the DSO will never really get unloaded.
That's not a problem for atexit() since the process is headed for
the exit.

I'm less sure about using it here since we don't have a way to
unreference the DSO upon pthread_key_delete().

 - todd



Re: Correct name for size_t argument in strlcpy.3

2021-04-30 Thread Todd C . Miller
On Fri, 30 Apr 2021 11:15:21 -0600, "Theo de Raadt" wrote:

> I disagree.
>
> "dstsize" is conceptually easier for readers to understand.
>
> Secondly, there is nothing which says the library code has to match the
> manual page.  Implementation does not need to match documentation.

My feelings exactly.

 - todd



Re: smtpd: more unused code

2021-04-20 Thread Todd C . Miller
On Tue, 20 Apr 2021 18:38:08 +0200, Eric Faurot wrote:

> On Sun, Apr 11, 2021 at 01:54:32PM +0200, Eric Faurot wrote:
> > Certificate verification is done by libtls. The former code is not used
> > anymore and can be unplugged.
>
> Anyone willing to ok this?

OK millert@

 - todd



Re: vi: apply expandtab to the output of a ! command

2021-04-19 Thread Todd C . Miller
On Tue, 13 Apr 2021 09:51:01 -0600, Todd C. Miller wrote:

> This is consistent with vim's expandtab behavior.
>
> From nvi2 (Craig Leres):
> https://github.com/lichray/nvi2/commit/7d3f43559026e0927032a105b217850feaefec
> 66
>
> I know most of us don't use expandtab but it is helpful when dealing
> with a codebase that is space-indented.  Since the option originated
> with vim we should be as compatible as possible...

Any objections?  This change brings our vi in line with both nvi2
and vim when it comes to expandtab behavior.

 - todd

> Index: docs/USD.doc/vi.man/vi.1
> ===
> RCS file: /cvs/src/usr.bin/vi/docs/USD.doc/vi.man/vi.1,v
> retrieving revision 1.79
> diff -u -p -u -r1.79 vi.1
> --- docs/USD.doc/vi.man/vi.1  8 Mar 2021 02:47:29 -   1.79
> +++ docs/USD.doc/vi.man/vi.1  13 Apr 2021 15:44:26 -
> @@ -2356,8 +2356,12 @@ characters to
>  when inserting, replacing or shifting text, autoindenting,
>  indenting with
>  .Aq Ic control-T ,
> -or outdenting with
> -.Aq Ic control-D .
> +outdenting with
> +.Aq Ic control-D ,
> +or
> +when filtering lines with the
> +.Cm !\&
> +command.
>  .It Cm exrc , ex Bq off
>  Read the startup files in the local directory.
>  .It Cm extended Bq off
> Index: ex/ex_bang.c
> ===
> RCS file: /cvs/src/usr.bin/vi/ex/ex_bang.c,v
> retrieving revision 1.11
> diff -u -p -u -r1.11 ex_bang.c
> --- ex/ex_bang.c  18 Apr 2017 01:45:35 -  1.11
> +++ ex/ex_bang.c  13 Apr 2021 15:44:26 -
> @@ -171,6 +171,10 @@ ex_bang(SCR *sp, EXCMD *cmdp)
>   if (!F_ISSET(sp, SC_VI) && !F_ISSET(sp, SC_EX_SILENT))
>   (void)ex_puts(sp, "!\n");
>  
> + /* Apply expandtab to the new text */
> + if (O_ISSET(sp, O_EXPANDTAB))
> + ex_retab(sp, cmdp);
> +
>   /*
>* XXX
>* The ! commands never return an error, so that autoprint always
> Index: ex/ex_shift.c
> ===
> RCS file: /cvs/src/usr.bin/vi/ex/ex_shift.c,v
> retrieving revision 1.9
> diff -u -p -u -r1.9 ex_shift.c
> --- ex/ex_shift.c 30 Apr 2020 10:40:21 -  1.9
> +++ ex/ex_shift.c 13 Apr 2021 15:44:26 -
> @@ -21,7 +21,7 @@
>  
>  #include "../common/common.h"
>  
> -enum which {LEFT, RIGHT};
> +enum which {RETAB, LEFT, RIGHT};
>  static int shift(SCR *, EXCMD *, enum which);
>  
>  /*
> @@ -48,6 +48,18 @@ ex_shiftr(SCR *sp, EXCMD *cmdp)
>  }
>  
>  /*
> + * ex_retab -- Expand tabs (if enabled)
> + *
> + *
> + * PUBLIC: int ex_retab(SCR *, EXCMD *);
> + */
> +int
> +ex_retab(SCR *sp, EXCMD *cmdp)
> +{
> + return (shift(sp, cmdp, RETAB));
> +}
> +
> +/*
>   * shift --
>   *   Ex shift support.
>   */
> @@ -109,7 +121,9 @@ shift(SCR *sp, EXCMD *cmdp, enum which r
>   break;
>  
>   /* Calculate the new indent amount. */
> - if (rl == RIGHT)
> + if (rl == RETAB)
> + newcol = oldcol;
> + else if (rl == RIGHT)
>   newcol = oldcol + sw;
>   else {
>   newcol = oldcol < sw ? 0 : oldcol - sw;
> Index: include/ex_extern.h
> ===
> RCS file: /cvs/src/usr.bin/vi/include/ex_extern.h,v
> retrieving revision 1.16
> diff -u -p -u -r1.16 ex_extern.h
> --- include/ex_extern.h   27 May 2016 09:18:12 -  1.16
> +++ include/ex_extern.h   13 Apr 2021 15:44:26 -
> @@ -76,6 +76,7 @@ int ex_set(SCR *, EXCMD *);
>  int ex_shell(SCR *, EXCMD *);
>  int ex_exec_proc(SCR *, EXCMD *, char *, const char *, int);
>  int proc_wait(SCR *, pid_t, const char *, int, int);
> +int ex_retab(SCR *, EXCMD *);
>  int ex_shiftl(SCR *, EXCMD *);
>  int ex_shiftr(SCR *, EXCMD *);
>  int ex_source(SCR *, EXCMD *);
>



Re: another slowcgi cleanup

2021-04-16 Thread Todd C . Miller
On Fri, 16 Apr 2021 17:34:38 +0200, Claudio Jeker wrote:

> Every code using SLIST_REMOVE() should just switch to proper LISTs.
> I realized slowcgi was using SLIST where it should use LIST but that was
> only the start of the rabbit hole. There is also no need to double wrap
> the request. So this diff make this all nice and shiny.

Looks good to me.  OK millert@

 - todd



Re: printf(1) Fix hex numbers

2021-04-15 Thread Todd C . Miller
On Fri, 16 Apr 2021 03:34:04 +0200, Martijn van Duren wrote:

> We currently have NetBSD's behaviour when it comes to to no characters,
> so no need to change there imho, but the 2 character limit seems like
> a good place to stop, especially since there's no way to break out of
> this hungry hungry hippo.
>
> limiter inspiration taken from octal notation.
>
> While here, also document \x notation.

No objection from me but I'm not sure it needs to make 6.9.

 - todd



Re: Manpage fix: passwd.1

2021-04-14 Thread Todd C . Miller
On Wed, 14 Apr 2021 11:09:02 +0200, Benjamin Baier wrote:

> don't mention _PASSWORD_LEN in passwd manpage.
> It's also no longer true, since Rev1.52, local_passwd.c uses 1024
> bytes for the password array.

Committed, thanks.

 - todd



vi: apply expandtab to the output of a ! command

2021-04-13 Thread Todd C . Miller
This is consistent with vim's expandtab behavior.

>From nvi2 (Craig Leres):
https://github.com/lichray/nvi2/commit/7d3f43559026e0927032a105b217850feaefec66

I know most of us don't use expandtab but it is helpful when dealing
with a codebase that is space-indented.  Since the option originated
with vim we should be as compatible as possible...

 - todd

Index: docs/USD.doc/vi.man/vi.1
===
RCS file: /cvs/src/usr.bin/vi/docs/USD.doc/vi.man/vi.1,v
retrieving revision 1.79
diff -u -p -u -r1.79 vi.1
--- docs/USD.doc/vi.man/vi.18 Mar 2021 02:47:29 -   1.79
+++ docs/USD.doc/vi.man/vi.113 Apr 2021 15:44:26 -
@@ -2356,8 +2356,12 @@ characters to
 when inserting, replacing or shifting text, autoindenting,
 indenting with
 .Aq Ic control-T ,
-or outdenting with
-.Aq Ic control-D .
+outdenting with
+.Aq Ic control-D ,
+or
+when filtering lines with the
+.Cm !\&
+command.
 .It Cm exrc , ex Bq off
 Read the startup files in the local directory.
 .It Cm extended Bq off
Index: ex/ex_bang.c
===
RCS file: /cvs/src/usr.bin/vi/ex/ex_bang.c,v
retrieving revision 1.11
diff -u -p -u -r1.11 ex_bang.c
--- ex/ex_bang.c18 Apr 2017 01:45:35 -  1.11
+++ ex/ex_bang.c13 Apr 2021 15:44:26 -
@@ -171,6 +171,10 @@ ex_bang(SCR *sp, EXCMD *cmdp)
if (!F_ISSET(sp, SC_VI) && !F_ISSET(sp, SC_EX_SILENT))
(void)ex_puts(sp, "!\n");
 
+   /* Apply expandtab to the new text */
+   if (O_ISSET(sp, O_EXPANDTAB))
+   ex_retab(sp, cmdp);
+
/*
 * XXX
 * The ! commands never return an error, so that autoprint always
Index: ex/ex_shift.c
===
RCS file: /cvs/src/usr.bin/vi/ex/ex_shift.c,v
retrieving revision 1.9
diff -u -p -u -r1.9 ex_shift.c
--- ex/ex_shift.c   30 Apr 2020 10:40:21 -  1.9
+++ ex/ex_shift.c   13 Apr 2021 15:44:26 -
@@ -21,7 +21,7 @@
 
 #include "../common/common.h"
 
-enum which {LEFT, RIGHT};
+enum which {RETAB, LEFT, RIGHT};
 static int shift(SCR *, EXCMD *, enum which);
 
 /*
@@ -48,6 +48,18 @@ ex_shiftr(SCR *sp, EXCMD *cmdp)
 }
 
 /*
+ * ex_retab -- Expand tabs (if enabled)
+ *
+ *
+ * PUBLIC: int ex_retab(SCR *, EXCMD *);
+ */
+int
+ex_retab(SCR *sp, EXCMD *cmdp)
+{
+   return (shift(sp, cmdp, RETAB));
+}
+
+/*
  * shift --
  * Ex shift support.
  */
@@ -109,7 +121,9 @@ shift(SCR *sp, EXCMD *cmdp, enum which r
break;
 
/* Calculate the new indent amount. */
-   if (rl == RIGHT)
+   if (rl == RETAB)
+   newcol = oldcol;
+   else if (rl == RIGHT)
newcol = oldcol + sw;
else {
newcol = oldcol < sw ? 0 : oldcol - sw;
Index: include/ex_extern.h
===
RCS file: /cvs/src/usr.bin/vi/include/ex_extern.h,v
retrieving revision 1.16
diff -u -p -u -r1.16 ex_extern.h
--- include/ex_extern.h 27 May 2016 09:18:12 -  1.16
+++ include/ex_extern.h 13 Apr 2021 15:44:26 -
@@ -76,6 +76,7 @@ int ex_set(SCR *, EXCMD *);
 int ex_shell(SCR *, EXCMD *);
 int ex_exec_proc(SCR *, EXCMD *, char *, const char *, int);
 int proc_wait(SCR *, pid_t, const char *, int, int);
+int ex_retab(SCR *, EXCMD *);
 int ex_shiftl(SCR *, EXCMD *);
 int ex_shiftr(SCR *, EXCMD *);
 int ex_source(SCR *, EXCMD *);



Re: vi: don't expandtab when we're in command mode.

2021-04-13 Thread Todd C . Miller
On Tue, 13 Apr 2021 16:56:52 +0200, Martijn van Duren wrote:

> When adding a TRACE() I see that that case is only called incidentally
> and / works for me as is. However, it still doesn't work when
> in command-mode (e.g. ":%s// /g" still requires ^V escape).
>
> So either I'm not clear on which case this is trying to solve or it
> doesn't work as advertised.

There is a local change we have that requires ^V before 
in command mode.  However, without this change you cannot search
for a literal tab via / or ? when expandtab is set.

 - todd



vi: don't expandtab when we're in command mode.

2021-04-13 Thread Todd C . Miller
Otherwise you can't search for a tab unless you ^V escape it.

>From nvi2:
https://github.com/lichray/nvi2/commit/0ef1c824648bcfe9f831a9607cb465b18e313d1d

Index: vi/v_txt.c
===
RCS file: /cvs/src/usr.bin/vi/vi/v_txt.c,v
retrieving revision 1.34
diff -u -p -u -r1.34 v_txt.c
--- vi/v_txt.c  30 Apr 2020 10:40:21 -  1.34
+++ vi/v_txt.c  13 Apr 2021 14:18:23 -
@@ -1214,7 +1214,8 @@ leftmargin:   tp->lb[tp->cno - 1] = ' ';
hexcnt = 1;
goto insq_ch;
case K_TAB:
-   if (quote != Q_VTHIS && O_ISSET(sp, O_EXPANDTAB)) {
+   if (sp->showmode != SM_COMMAND && quote != Q_VTHIS &&
+   O_ISSET(sp, O_EXPANDTAB)) {
if (txt_dent(sp, tp, O_TABSTOP, 1))
goto err;
goto ebuf_chk;



Re: fix diff3/merge for files lacking \n at EOF

2021-04-12 Thread Todd C . Miller
On Mon, 12 Apr 2021 21:18:41 +0200, Stefan Sperling wrote:

> The problem is that the code scanning a common chunk of lines doesn't
> account for the possibility that EOF may occur before a final newline.
>
> This patch fixes the problem in all affected tools in the tree. OK?

The return value of getc() should be checked against EOF, not -1.
I know this is not a new problem in that code base so if you'd
rather leave that change for a cleanup diff that is OK by me.

 - todd



Re: libressl pc files

2021-04-12 Thread Todd C . Miller
On Mon, 12 Apr 2021 13:25:06 -0600, "Todd C. Miller" wrote:

> This is a bit of a mess.  LibreSSL portable puts the LibreSSL version
> number in the pc files.  In-tree LibreSSL uses 1.0.0 which is clearly
> wrong--using SHLIB_VERSION_NUMBER for the version makes absolutely
> no sense to me.  The pc files used by OpenSSL also use the release
> version.  The simplest thing would be for us to use the LibreSSL
> version too.
>
> For third-party code that uses "pkg-config --atleast-version=foo",
> it might be more useful to list what version of OpenSSL we are
> "compatible" with but I don't think that is really workable since
> there is no one-to-one comparison.  My preference would be to just
> extract the version from LIBRESSL_VERSION_TEXT in opensslv.h.

As Stu points out this is the wrong time in the release cycle to
change this.  We should revisit it post-6.9.

 - todd



Re: libressl pc files

2021-04-12 Thread Todd C . Miller
This is a bit of a mess.  LibreSSL portable puts the LibreSSL version
number in the pc files.  In-tree LibreSSL uses 1.0.0 which is clearly
wrong--using SHLIB_VERSION_NUMBER for the version makes absolutely
no sense to me.  The pc files used by OpenSSL also use the release
version.  The simplest thing would be for us to use the LibreSSL
version too.

For third-party code that uses "pkg-config --atleast-version=foo",
it might be more useful to list what version of OpenSSL we are
"compatible" with but I don't think that is really workable since
there is no one-to-one comparison.  My preference would be to just
extract the version from LIBRESSL_VERSION_TEXT in opensslv.h.

 - todd



Re: smtpd: default mta ciphers

2021-04-01 Thread Todd C . Miller
On Thu, 01 Apr 2021 16:34:33 +0200, Eric Faurot wrote:

> If not cipher list is specified for a relay rule, fallback to
> the global cipher list if defined, rather than libtls default.
> This is closer to the previous behavior.

OK millert@

 - todd



Re: btrace: add dry run mode

2021-03-19 Thread Todd C . Miller
On Fri, 19 Mar 2021 13:22:35 +0100, Klemens Nanni wrote:

> I argue it should be `-n' like all the daemons, e.g. vmd(8) and other
> parsers such as pfctl(8) do.

Yes, please.  I was about to make the same point.

 - todd



Re: pppoe(4): describe what happens if RFC 4638 is not supported

2021-03-16 Thread Todd C . Miller
On Tue, 16 Mar 2021 13:59:20 +0100, Klemens Nanni wrote:

> It doesn't hurt however to mention the specific message such that users
> can tell *when* that happens;  perhaps add it under DIAGNOSTICS as the
> various wifi driver manuals, e.g. athn(4) do?

I think it makes more sense to have the information in the MTU/MSS
ISSUES section where people are more likely to look for it.

There are other surprising things about how pppoe(4) behaves with
respect to MTU that could use documenting.  For example, it stashes
the MTU of pppoedev and doesn't refresh it when the MTU of pppoedev
changes.

 - todd



pppoe(4): describe what happens if RFC 4638 is not supported

2021-03-16 Thread Todd C . Miller
I think it is helpful to tell the user what happens if the remote
endpoint doesn't support RFC 4638.

OK?

 - todd

Index: share/man/man4/pppoe.4
===
RCS file: /cvs/src/share/man/man4/pppoe.4,v
retrieving revision 1.34
diff -u -p -u -r1.34 pppoe.4
--- share/man/man4/pppoe.4  16 Jun 2017 10:58:43 -  1.34
+++ share/man/man4/pppoe.4  16 Mar 2021 12:05:40 -
@@ -201,6 +201,14 @@ With this, the previously mentioned MSS 
 .Xr pf.conf 5
 are no longer necessary.
 .Pp
+If you attemp to use an MTU larger than 1492 where the remote equipment does
+.Em not
+support RFC 4638,
+.Nm
+will write
+.Dq No valid PPP-Max-Payload tag received in PADO
+to the kernel message buffer and the MTU will be clamped to 1492.
+.Pp
 See
 .Xr pf.conf 5
 for more information on MTU, MSS, and NAT.



Re: ksh vi.c: define CTRL locally

2021-03-11 Thread Todd C . Miller
On Thu, 11 Mar 2021 12:44:18 +0100, Benjamin Baier wrote:

> lets's sync up vi.c with emacs.c and define CTRL locally.
> Makes the portable version work on FreeBSD https://github.com/ibara/oksh/issu
> es/59
> And as a bonus makes ksh -DSMALL compile with -DVI.

Thanks, I committed this without the special case for '?' which we
don't need for vi.c.

 - todd



Re: quiz: Fix multi-line questions (trailing newline)

2021-03-10 Thread Todd C . Miller
On Wed, 10 Mar 2021 22:49:13 -0500, Alex Karle wrote:

> > This would now be clearer as:
> > 
> > if (qlen > 0 && qp->q_text[qlen - 1] == '\\') {
>
> I agree this is clearer, but I think we'll need a `qlen = 0`
> initialization up above the while loop if we go with this variation
> (otherwise an uninitialized qlen could be > 0, causing indexing a null
> q_text, right?). Apologies if this was implied by your comment, thought
> it couldn't hurt to flag!

You are absolutely correct.

 - todd



Re: Remove extra pointer to gzFile in grep(1)

2021-03-10 Thread Todd C . Miller
On Wed, 10 Mar 2021 16:46:32 -0500, Josh Rickmar wrote:

> The compress(3) family of utility functions return and operate on
> gzFile, a typedef for void*.  The extra pointer to this gzFile in
> grep(1) can be removed.  Other uses of these compress(3) functions in
> tree (spamd, mandoc, smtpd) do not use the extra pointer.

Thanks, committed.

 - todd



Re: quiz: Fix multi-line questions (trailing newline)

2021-03-10 Thread Todd C . Miller
On Wed, 10 Mar 2021 21:15:13 +0100, Christian Weisgerber wrote:

> Right.  Next iteration:

Looks better, one minor nit inline.

 - todd

> Index: quiz.c
> ===
> RCS file: /cvs/src/games/quiz/quiz.c,v
> retrieving revision 1.30
> diff -u -p -r1.30 quiz.c
> --- quiz.c24 Aug 2018 11:14:49 -  1.30
> +++ quiz.c10 Mar 2021 20:04:37 -
> @@ -48,7 +48,6 @@ static QE qlist;
>  static int catone, cattwo, tflag;
>  static u_int qsize;
>  
> -char *appdstr(char *, const char *, size_t);
>  void  downcase(char *);
>  void  get_cats(char *, char *);
>  void  get_file(const char *);
> @@ -110,7 +109,8 @@ get_file(const char *file)
>  {
>   FILE *fp;
>   QE *qp;
> - size_t len;
> + ssize_t len;
> + size_t qlen, size;
>   char *lp;
>  
>   if ((fp = fopen(file, "r")) == NULL)
> @@ -123,26 +123,36 @@ get_file(const char *file)
>*/
>   qp = 
>   qsize = 0;
> - while ((lp = fgetln(fp, )) != NULL) {
> + lp = NULL;
> + size = 0;
> + while ((len = getline(, , fp)) != -1) {
>   if (lp[len - 1] == '\n')
> - --len;
> + lp[--len] = '\0';
> + if (qp->q_text)
> + qlen = strlen(qp->q_text);
>   if (qp->q_text && qp->q_text[0] != '\0' &&

This would now be clearer as:

if (qlen > 0 && qp->q_text[qlen - 1] == '\\') {

> - qp->q_text[strlen(qp->q_text) - 1] == '\\')
> - qp->q_text = appdstr(qp->q_text, lp, len);
> - else {
> + qp->q_text[qlen - 1] == '\\') {
> + qp->q_text[--qlen] = '\0';
> + qlen += len;
> + qp->q_text = realloc(qp->q_text, qlen + 1);
> + if (qp->q_text == NULL)
> + errx(1, "realloc");
> + strlcat(qp->q_text, lp, qlen + 1);
> + } else {
>   if ((qp->q_next = malloc(sizeof(QE))) == NULL)
>   errx(1, "malloc");
>   qp = qp->q_next;
> - if ((qp->q_text = malloc(len + 1)) == NULL)
> - errx(1, "malloc");
> - /* lp may not be zero-terminated; cannot use strlcpy */
> - strncpy(qp->q_text, lp, len);
> - qp->q_text[len] = '\0';
> + qp->q_text = strdup(lp);
> + if (qp->q_text == NULL)
> + errx(1, "strdup");
>   qp->q_asked = qp->q_answered = FALSE;
>   qp->q_next = NULL;
>   ++qsize;
>   }
>   }
> + free(lp);
> + if (ferror(fp))
> + err(1, "getline");
>   (void)fclose(fp);
>  }
>  
> @@ -316,32 +326,6 @@ next_cat(const char *s)
>   esc = 0;
>   break;
>   }
> -}
> -
> -char *
> -appdstr(char *s, const char *tp, size_t len)
> -{
> - char *mp;
> - const char *sp;
> - int ch;
> - char *m;
> -
> - if ((m = malloc(strlen(s) + len + 1)) == NULL)
> - errx(1, "malloc");
> - for (mp = m, sp = s; (*mp++ = *sp++) != '\0'; )
> - ;
> - --mp;
> - if (*(mp - 1) == '\\')
> - --mp;
> -
> - while ((ch = *mp++ = *tp++) && ch != '\n')
> - ;
> - if (*(mp - 2) == '\\')
> - mp--;
> - *mp = '\0';
> -
> - free(s);
> - return (m);
>  }
>  
>  void
> -- 
> Christian "naddy" Weisgerber  na...@mips.inka.de



Re: ksh: [vi.c] "clear-screen" bug + patch

2021-03-10 Thread Todd C . Miller
Now the the clear screen change has been committed, here's the
insert mode ^R (redraw) diff again with a man page update.  Note
that ^R is already supported in command mode.

OK?

 - todd

Index: bin/ksh/ksh.1
===
RCS file: /cvs/src/bin/ksh/ksh.1,v
retrieving revision 1.212
diff -u -p -u -r1.212 ksh.1
--- bin/ksh/ksh.1   8 Mar 2021 06:20:50 -   1.212
+++ bin/ksh/ksh.1   10 Mar 2021 20:08:21 -
@@ -5060,6 +5060,8 @@ See the
 command in
 .Sx Emacs editing mode
 for more information.
+.It ^R
+Redraw the current line.
 .It ^V
 Literal next.
 The next character typed is not treated specially (can be used
Index: bin/ksh/vi.c
===
RCS file: /cvs/src/bin/ksh/vi.c,v
retrieving revision 1.58
diff -u -p -u -r1.58 vi.c
--- bin/ksh/vi.c10 Mar 2021 20:06:04 -  1.58
+++ bin/ksh/vi.c10 Mar 2021 20:08:21 -
@@ -658,6 +658,10 @@ vi_insert(int ch)
do_clear_screen();
break;
 
+   case CTRL('r'):
+   redraw_line(1, 0);
+   break;
+
case CTRL('i'):
if (Flag(FVITABCOMPLETE)) {
complete_word(0, 0);



Re: quiz: Fix multi-line questions (trailing newline)

2021-03-09 Thread Todd C . Miller
On Tue, 09 Mar 2021 22:04:42 +0100, Christian Weisgerber wrote:

> Thanks a lot for figuring this out!  I finally got around to looking
> at your patch.  Once we have nul-terminated lines, appdstr() can
> be replaced with realloc() and strlcat().

I don't think your use of qlen is safe since it is initialized
to zero.  Specifically, it looks like "qp->q_text[qlen - 1]"
would be an out of bounds read.  Should qlen be initialized
to strlen(qp->q_text) if qp->q_text != NULL?

 - todd



Re: mg: Fix Coverity Scan warning: Insecure data handling

2021-03-09 Thread Todd C . Miller
On Tue, 09 Mar 2021 20:14:19 +, Mark Lumsden wrote:

> Here is a diff from Joachim Wiberg's version of mg.
>
> "The strlcpy() function is guaranteed to never copy more than 'len - 1'
> bytes, so there is no need to check if we copied more.  This is a bogus
> warning since the introduction of strlcpy()."

That looks wrong to me.  strlcpy() returns the number of bytes it
would have copied if there was space.  But if there was insufficient
space then the return value can be larger.  It is not safe to blindly
use the return value without checking it first.

 - todd



Re: ksh: [vi.c] "clear-screen" bug + patch

2021-03-09 Thread Todd C . Miller
On Tue, 09 Mar 2021 17:53:06 +0100, Benjamin Baier wrote:

> Ping

I think that in do_clear_screen() full should not be set unless
neednl is 0.  That is, we should only print the entire prompt if
the screen was actually cleared.  Otherwise looks good to me.

 - todd

Index: bin/ksh/vi.c
===
RCS file: /cvs/src/bin/ksh/vi.c,v
retrieving revision 1.57
diff -u -p -u -r1.57 vi.c
--- bin/ksh/vi.c20 Sep 2020 14:40:45 -  1.57
+++ bin/ksh/vi.c9 Mar 2021 17:02:14 -
@@ -55,7 +55,7 @@ static intEndword(int);
 static int grabhist(int, int);
 static int grabsearch(int, int, int, char *);
 static voiddo_clear_screen(void);
-static voidredraw_line(int);
+static voidredraw_line(int, int);
 static voidrefresh_line(int);
 static int outofwin(void);
 static voidrewindow(void);
@@ -719,7 +719,7 @@ vi_cmd(int argcnt, const char *cmd)
break;
 
case CTRL('r'):
-   redraw_line(1);
+   redraw_line(1, 0);
break;
 
case '@':
@@ -1737,18 +1737,19 @@ do_clear_screen(void)
neednl = 0;
}
 #endif
-   redraw_line(neednl);
+   /* Only print the full prompt if we cleared the screen. */
+   redraw_line(neednl, !neednl);
 }
 
 static void
-redraw_line(int neednl)
+redraw_line(int neednl, int full)
 {
(void) memset(wbuf[win], ' ', wbuf_len);
if (neednl) {
x_putc('\r');
x_putc('\n');
}
-   vi_pprompt(0);
+   vi_pprompt(full);
cur_col = pwidth;
morec = ' ';
 }
@@ -2109,7 +2110,7 @@ complete_word(int command, int count)
vi_error();
x_print_expansions(nwords, words, is_command);
x_free_words(nwords, words);
-   redraw_line(0);
+   redraw_line(0, 0);
return -1;
}
/*
@@ -2183,7 +2184,7 @@ print_expansions(struct edstate *e)
}
x_print_expansions(nwords, words, is_command);
x_free_words(nwords, words);
-   redraw_line(0);
+   redraw_line(0, 0);
return 0;
 }
 



Re: smtpd: use mx name for sni

2021-03-07 Thread Todd C . Miller
On Sun, 07 Mar 2021 21:47:45 +0100, Eric Faurot wrote:

> As spotted by krw@, the mta should use the mx hostname for sni, not
> the reverse dns for the peer address.

Yes, this matches the previous behavior with ssl_check_name().
OK millert@

 - todd



Re: if calloc() needs nmemb and size, why doesn't freezero()?

2021-02-19 Thread Todd C . Miller
On Fri, 19 Feb 2021 10:38:13 -0600, Luke Small wrote:

> In malloc(3):
> “If you use smaller integer types than size_t for ‘nmemb’ and ‘size’, then
> multiplication in freezero() may need to be cast to size_t to avoid integer
> overflow:
> freezero(ptr, (size_t)nmemb * (size_t)size);”
> Or maybe even: freezero(ptr, (size_t)nmemb * size);

This is bad advice.  The product of two size_t values can exceed
SIZE_MAX, at which point you would get integer overflow.  This is
why the malloc(3) man page warns against it.  Note that on 64-bit
platforms like amd64, size_t is already 64-bit so casting to unsigned
long long or uint64_t is not effective.

On OpenBSD, calloc(3) and reallocarray(3) check for integer overflow
for you, which is why they are preferred over malloc(nmemb * size).
You can examing the code yourself:
http://cvsweb.openbsd.org/src/lib/libc/stdlib/reallocarray.c?rev=1.3

 - todd



Re: disklabel: make use of getline(3)

2021-02-01 Thread Todd C . Miller
On Sun, 31 Jan 2021 15:43:32 +0100, Christian Weisgerber wrote:

> Replace fgetln(3) with getline(3).
>
> Since getline() returns a C string, we don't need to carry around
> the length separately.

OK millert@

 - todd



Re: fdisk: make use of getline(3)

2021-01-30 Thread Todd C . Miller
On Sat, 30 Jan 2021 18:02:10 +0100, Christian Weisgerber wrote:

> Replace fgetln(3) with getline(3).

OK millert@

 - todd



Re: disklabel: pointer deref fix

2021-01-30 Thread Todd C . Miller
On Sat, 30 Jan 2021 01:01:37 +0100, Christian Weisgerber wrote:

> Fix a pointer dereference in disklabel(8).
>
> This looks like somebody wrote *s[0] in place of (*s)[0].
> Which in this case happens to be equivalent, but it still looks
> wrong.

Probably my bug.  OK millert@

 - todd



Re: sed: make use of getline(3)

2021-01-30 Thread Todd C . Miller
On Sat, 30 Jan 2021 00:43:02 +0100, Christian Weisgerber wrote:

> Replace fgetln(3) with getline(3) in sed.
>
> The mf_fgets() part is from Johann Oskarsson for Illumos/FreeBSD.
>
> Passes our sed regression tests.

OK millert@

 - todd



  1   2   3   4   5   6   7   8   9   10   >