Re: [PATCH netcat] Only force fd's to -1 once
On Sun, Sep 27, 2020 at 08:27:24PM -0600, Theo de Raadt wrote: > Duncan Roe wrote: > > > > I disagree with your plan. This kind of "force it off" programming is > > > typical. > > > > > Might be typical, but I think it's sloppy. > > I don't think it is sloppy. > > Instead, I think your additional logic complicates review. > > Never mind, v2 is much simpler but I won't post unless / until I can verify the behaviour on the OpenBSD VM. Discard v1 that I posted earlier, Cheers ... Duncan.
Re: [PATCH netcat] Only force fd's to -1 once
Duncan Roe wrote: > > I disagree with your plan. This kind of "force it off" programming is > > typical. > > > Might be typical, but I think it's sloppy. I don't think it is sloppy. Instead, I think your additional logic complicates review.
Re: [PATCH netcat] Only force fd's to -1 once
On Sun, Sep 27, 2020 at 07:53:27PM -0600, Theo de Raadt wrote: > Duncan Roe wrote: > > > On Sun, Sep 27, 2020 at 02:18:21PM -0600, Bob Beck wrote: > > > On Sun, Sep 27, 2020 at 02:46:39PM +1000, Duncan Roe wrote: > > > > The motivation for this is to make debug logs less confusing. > > > > > > What is this fixing and what behavior are you changing? > > > > You can only see the difference when running nc under gdb, if you choose to > > breakpoint those lines that set pfd[POLL_xx].fd to -1. > > > > I was doing this because I wanted to track when file units got closed. > > But sometimes an fd was getting set to -1 when it was -1 already, hence the > > patch. > > I disagree with your plan. This kind of "force it off" programming is > typical. > Might be typical, but I think it's sloppy. Will send v2 anyway, take it or leave it. No feedback about my previous (more useful) patch "Use correct preposition in verbose reports". One more to come. Cheers ... Duncan.
Re: [PATCH netcat] Only force fd's to -1 once
Duncan Roe wrote: > On Sun, Sep 27, 2020 at 02:18:21PM -0600, Bob Beck wrote: > > On Sun, Sep 27, 2020 at 02:46:39PM +1000, Duncan Roe wrote: > > > The motivation for this is to make debug logs less confusing. > > > > What is this fixing and what behavior are you changing? > > You can only see the difference when running nc under gdb, if you choose to > breakpoint those lines that set pfd[POLL_xx].fd to -1. > > I was doing this because I wanted to track when file units got closed. > But sometimes an fd was getting set to -1 when it was -1 already, hence the > patch. I disagree with your plan. This kind of "force it off" programming is typical.
Re: [PATCH netcat] Only force fd's to -1 once
On Sun, Sep 27, 2020 at 02:18:21PM -0600, Bob Beck wrote:
> On Sun, Sep 27, 2020 at 02:46:39PM +1000, Duncan Roe wrote:
> > The motivation for this is to make debug logs less confusing.
>
> What is this fixing and what behavior are you changing?
You can only see the difference when running nc under gdb, if you choose to
breakpoint those lines that set pfd[POLL_xx].fd to -1.
I was doing this because I wanted to track when file units got closed.
But sometimes an fd was getting set to -1 when it was -1 already, hence the
patch.
Loking at the patch again, I see that the first part of hunk 3 is unnecessary
so I'll send a v2.
Cheers ... Duncan.
>
> >
> > All changed lines have previously demonstrated the problem.
> >
> > Signed-off-by: Duncan Roe
> > ---
> > usr.bin/nc/netcat.c | 27 ++-
> > 1 file changed, 14 insertions(+), 13 deletions(-)
> >
> > diff --git a/usr.bin/nc/netcat.c b/usr.bin/nc/netcat.c
> > index 528dbeea678..b152cfaf635 100644
> > --- a/usr.bin/nc/netcat.c
> > +++ b/usr.bin/nc/netcat.c
> > @@ -1196,7 +1196,7 @@ readwrite(int net_fd, struct tls *tls_ctx)
> > !(pfd[POLL_NETIN].revents & POLLIN))
> > pfd[POLL_NETIN].fd = -1;
> >
> > - if (pfd[POLL_NETOUT].revents & POLLHUP) {
> > + if ((pfd[POLL_NETOUT].revents & POLLHUP) && pfd[POLL_NETOUT].fd
> > != -1) {
> > if (Nflag)
> > shutdown(pfd[POLL_NETOUT].fd, SHUT_WR);
> > pfd[POLL_NETOUT].fd = -1;
> > @@ -1205,14 +1205,14 @@ readwrite(int net_fd, struct tls *tls_ctx)
> > if (pfd[POLL_STDOUT].revents & POLLHUP)
> > pfd[POLL_STDOUT].fd = -1;
> > /* if no net out, stop watching stdin */
> > - if (pfd[POLL_NETOUT].fd == -1)
> > + if (pfd[POLL_NETOUT].fd == -1 && pfd[POLL_STDIN].fd != -1)
> > pfd[POLL_STDIN].fd = -1;
> > /* if no stdout, stop watching net in */
> > - if (pfd[POLL_STDOUT].fd == -1) {
> > - if (pfd[POLL_NETIN].fd != -1)
> > - shutdown(pfd[POLL_NETIN].fd, SHUT_RD);
> > - pfd[POLL_NETIN].fd = -1;
> > - }
> > + if (pfd[POLL_STDOUT].fd == -1 &&
> > + pfd[POLL_NETIN].fd != -1) {
> > + shutdown(pfd[POLL_NETIN].fd, SHUT_RD);
> > + pfd[POLL_NETIN].fd = -1;
> > + }
> >
> > /* try to read from stdin */
> > if (pfd[POLL_STDIN].revents & POLLIN && stdinbufpos < BUFSIZE) {
> > @@ -1299,15 +1299,16 @@ readwrite(int net_fd, struct tls *tls_ctx)
> > }
> >
> > /* stdin gone and queue empty? */
> > - if (pfd[POLL_STDIN].fd == -1 && stdinbufpos == 0) {
> > - if (pfd[POLL_NETOUT].fd != -1 && Nflag)
> > - shutdown(pfd[POLL_NETOUT].fd, SHUT_WR);
> > + if (pfd[POLL_STDIN].fd == -1 && stdinbufpos == 0 &&
> > + pfd[POLL_NETOUT].fd != -1) {
> > + if (Nflag)
> > + shutdown(pfd[POLL_NETOUT].fd, SHUT_WR);
> > pfd[POLL_NETOUT].fd = -1;
> > }
> > /* net in gone and queue empty? */
> > - if (pfd[POLL_NETIN].fd == -1 && netinbufpos == 0) {
> > - pfd[POLL_STDOUT].fd = -1;
> > - }
> > + if (pfd[POLL_NETIN].fd == -1 && netinbufpos == 0 &&
> > + pfd[POLL_STDOUT].fd != -1)
> > + pfd[POLL_STDOUT].fd = -1;
> > }
> > }
> >
> > --
> > 2.17.5
> >
Re: apmd(8) and hw.perfpolicy quirks
On 2020-09-27, Jeremie Courreges-Anglas wrote: > The diff below teaches apmd(8) -H to set hw.perfpolicy="manual" and > hw.setperf=100, instead of setting hw.perfpolicy="high". sure. if you would like to own this, by all means. :)
[PATCH ssh] VisualHostKey: unknown keys
After using VisualHostKey ssh client option for some time, I've found
out that for me it's quite useful for manual host key verification
(where DNS-signed host keys are not available, for example), but it's
too noisy for casual usage when it outputs ASCII art on every login. So,
I've implemented a new value for VisualHostKey option, "unknown", that
only displays ASCII art when prompting for unknown host key, but is
silent for already known ones.
I'm not an OpenBSD user myself, but ported SSH is used world wide and I
find this feature useful, so I propose my patch for it here. I have
tested the patch on the portable repository. I'll happily fix any issues
if you find any problems while finding the contribution a welcome one,
sorry in advance if I've done something wrong.
In case the patch is good as it is now,
Signed-off-by: Dmitry Lapshin
---
readconf.c | 11 ++-
readconf.h | 4
ssh.1| 4 +++-
ssh_config.5 | 4
sshconnect.c | 2 +-
5 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/usr.bin/ssh/readconf.c b/usr.bin/ssh/readconf.c
index 554efd7..dd2c720 100644
--- a/usr.bin/ssh/readconf.c
+++ b/usr.bin/ssh/readconf.c
@@ -876,6 +876,14 @@ static const struct multistate
multistate_compression[] = {
{ "no", COMP_NONE },
{ NULL, -1 }
};
+static const struct multistate multistate_visualhostkey[] = {
+ { "true", SSH_VISUAL_HOSTKEY_YES },
+ { "false", SSH_VISUAL_HOSTKEY_NO },
+ { "yes",SSH_VISUAL_HOSTKEY_YES },
+ { "no", SSH_VISUAL_HOSTKEY_NO },
+ { "unknown",SSH_VISUAL_HOSTKEY_UNKNOWN },
+ { NULL, -1 }
+};
static int
parse_multistate_value(const char *arg, const char *filename, int linenum,
@@ -1607,7 +1615,8 @@ parse_keytypes:
case oVisualHostKey:
intptr = &options->visual_host_key;
- goto parse_flag;
+ multistate_ptr = multistate_visualhostkey;
+ goto parse_multistate;
case oInclude:
if (cmdline)
diff --git a/usr.bin/ssh/readconf.h b/usr.bin/ssh/readconf.h
index d6a1555..06ebd4d 100644
--- a/usr.bin/ssh/readconf.h
+++ b/usr.bin/ssh/readconf.h
@@ -200,6 +200,10 @@ typedef struct {
#define SSH_STRICT_HOSTKEY_YES 2
#define SSH_STRICT_HOSTKEY_ASK 3
+#define SSH_VISUAL_HOSTKEY_NO 0
+#define SSH_VISUAL_HOSTKEY_YES 1
+#define SSH_VISUAL_HOSTKEY_UNKNOWN 2
+
const char *kex_default_pk_alg(void);
char *ssh_connection_hash(const char *thishost, const char *host,
const char *portstr, const char *user);
diff --git a/usr.bin/ssh/ssh.1 b/usr.bin/ssh/ssh.1
index 5553178..44ab019 100644
--- a/usr.bin/ssh/ssh.1
+++ b/usr.bin/ssh/ssh.1
@@ -1240,7 +1240,9 @@ By setting the
option to
.Dq yes ,
a small ASCII graphic gets displayed on every login to a server, no matter
-if the session itself is interactive or not.
+if the session itself is interactive or not. Option value
+.Dq unknown
+will display random art only for unknown keys.
By learning the pattern a known server produces, a user can easily
find out that the host key has changed when a completely different pattern
is displayed.
diff --git a/usr.bin/ssh/ssh_config.5 b/usr.bin/ssh/ssh_config.5
index 6be1f1a..97f97fa 100644
--- a/usr.bin/ssh/ssh_config.5
+++ b/usr.bin/ssh/ssh_config.5
@@ -1785,6 +1785,10 @@ an ASCII art representation of the remote host
key fingerprint is
printed in addition to the fingerprint string at login and
for unknown host keys.
If this flag is set to
+.Cm unknown ,
+no fingerprint strings are printed at login, but an ASCII art and
+a fingerprint string are printed for unknown host keys.
+If this flag is set to
.Cm no
(the default),
no fingerprint strings are printed at login and
diff --git a/usr.bin/ssh/sshconnect.c b/usr.bin/ssh/sshconnect.c
index 9ec0618..fe59ac5 100644
--- a/usr.bin/ssh/sshconnect.c
+++ b/usr.bin/ssh/sshconnect.c
@@ -830,7 +830,7 @@ check_host_key(char *hostname, struct sockaddr
*hostaddr, u_short port,
logit("Warning: Permanently added the %s host "
"key for IP address '%.128s' to the list "
"of known hosts.", type, ip);
- } else if (options.visual_host_key) {
+ } else if (options.visual_host_key == SSH_VISUAL_HOSTKEY_YES) {
fp = sshkey_fingerprint(host_key,
options.fingerprint_hash, SSH_FP_DEFAULT);
ra = sshkey_fingerprint(host_key,
Re: [PATCH netcat] Only force fd's to -1 once
On Sun, Sep 27, 2020 at 02:46:39PM +1000, Duncan Roe wrote:
> The motivation for this is to make debug logs less confusing.
What is this fixing and what behavior are you changing?
>
> All changed lines have previously demonstrated the problem.
>
> Signed-off-by: Duncan Roe
> ---
> usr.bin/nc/netcat.c | 27 ++-
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/usr.bin/nc/netcat.c b/usr.bin/nc/netcat.c
> index 528dbeea678..b152cfaf635 100644
> --- a/usr.bin/nc/netcat.c
> +++ b/usr.bin/nc/netcat.c
> @@ -1196,7 +1196,7 @@ readwrite(int net_fd, struct tls *tls_ctx)
> !(pfd[POLL_NETIN].revents & POLLIN))
> pfd[POLL_NETIN].fd = -1;
>
> - if (pfd[POLL_NETOUT].revents & POLLHUP) {
> + if ((pfd[POLL_NETOUT].revents & POLLHUP) && pfd[POLL_NETOUT].fd
> != -1) {
> if (Nflag)
> shutdown(pfd[POLL_NETOUT].fd, SHUT_WR);
> pfd[POLL_NETOUT].fd = -1;
> @@ -1205,14 +1205,14 @@ readwrite(int net_fd, struct tls *tls_ctx)
> if (pfd[POLL_STDOUT].revents & POLLHUP)
> pfd[POLL_STDOUT].fd = -1;
> /* if no net out, stop watching stdin */
> - if (pfd[POLL_NETOUT].fd == -1)
> + if (pfd[POLL_NETOUT].fd == -1 && pfd[POLL_STDIN].fd != -1)
> pfd[POLL_STDIN].fd = -1;
> /* if no stdout, stop watching net in */
> - if (pfd[POLL_STDOUT].fd == -1) {
> - if (pfd[POLL_NETIN].fd != -1)
> - shutdown(pfd[POLL_NETIN].fd, SHUT_RD);
> - pfd[POLL_NETIN].fd = -1;
> - }
> + if (pfd[POLL_STDOUT].fd == -1 &&
> + pfd[POLL_NETIN].fd != -1) {
> + shutdown(pfd[POLL_NETIN].fd, SHUT_RD);
> + pfd[POLL_NETIN].fd = -1;
> + }
>
> /* try to read from stdin */
> if (pfd[POLL_STDIN].revents & POLLIN && stdinbufpos < BUFSIZE) {
> @@ -1299,15 +1299,16 @@ readwrite(int net_fd, struct tls *tls_ctx)
> }
>
> /* stdin gone and queue empty? */
> - if (pfd[POLL_STDIN].fd == -1 && stdinbufpos == 0) {
> - if (pfd[POLL_NETOUT].fd != -1 && Nflag)
> - shutdown(pfd[POLL_NETOUT].fd, SHUT_WR);
> + if (pfd[POLL_STDIN].fd == -1 && stdinbufpos == 0 &&
> + pfd[POLL_NETOUT].fd != -1) {
> + if (Nflag)
> + shutdown(pfd[POLL_NETOUT].fd, SHUT_WR);
> pfd[POLL_NETOUT].fd = -1;
> }
> /* net in gone and queue empty? */
> - if (pfd[POLL_NETIN].fd == -1 && netinbufpos == 0) {
> - pfd[POLL_STDOUT].fd = -1;
> - }
> + if (pfd[POLL_NETIN].fd == -1 && netinbufpos == 0 &&
> + pfd[POLL_STDOUT].fd != -1)
> + pfd[POLL_STDOUT].fd = -1;
> }
> }
>
> --
> 2.17.5
>
Re: RFC: kern.video.record
On Sun, Sep 13, 2020 at 09:23:36AM +0100, Laurence Tratt wrote:
> Since I recently opened my big fat mouth and suggested that
> "kern.video.record" (analogous to kern.audio.record) might be a good idea, I
> decided to put together a quick prototype (heavily based on the
> kern.audio.record code). This at least roughly works for me but raises some
> questions such as:
>
> * Is uvideo the only driver that can capture video? [I imagine not, but I
> don't really know.]
>
> * I've taken the same approach as kern.audio.record which is to keep on
> handing out data even when kern.video.record=0 *but* it's harder to work
> out what data we should hand out. At the moment we just pass on whatever
> happened to be in the buffer beforehand (which is clearly not a good
> idea in the long term, but proves the point for now). For uncompressed
> video, handing over (say) an entirely black image is probably easy; for
> compressed video we'd have to think about whether we also want to
> manipulate frame headers etc. It would probably be easier to simply
> intercept the "opening video" event but that would mean that if
> something is already streaming video, setting kern.video.record=0 would
> allow it to keep recording.
>
> Comments welcome, including "this is a terrible idea full stop"!
It seems that no-one yet thinks this is a terrible idea, and most people
who've expressed an opinion prefer kern.audio.record and kern.video.record
to be separate rather than combined.
The two "big" questions above remain unanswered but, in the meantime, I'm
attaching a minor update which fixes a small bug in the patch in sysctl.c
that Edd Barrett found.
Laurie
Index: sbin/sysctl/sysctl.c
===
RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
retrieving revision 1.252
diff -u -p -u -r1.252 sysctl.c
--- sbin/sysctl/sysctl.c15 Jul 2020 07:13:56 - 1.252
+++ sbin/sysctl/sysctl.c27 Sep 2020 16:49:54 -
@@ -130,6 +130,7 @@ struct ctlname machdepname[] = CTL_MACHD
#endif
struct ctlname ddbname[] = CTL_DDB_NAMES;
struct ctlname audioname[] = CTL_KERN_AUDIO_NAMES;
+struct ctlname videoname[] = CTL_KERN_VIDEO_NAMES;
struct ctlname witnessname[] = CTL_KERN_WITNESS_NAMES;
char names[BUFSIZ];
int lastused;
@@ -219,6 +220,7 @@ void print_sensor(struct sensor *);
int sysctl_chipset(char *, char **, int *, int, int *);
#endif
int sysctl_audio(char *, char **, int *, int, int *);
+int sysctl_video(char *, char **, int *, int, int *);
int sysctl_witness(char *, char **, int *, int, int *);
void vfsinit(void);
@@ -517,6 +519,11 @@ parse(char *string, int flags)
if (len < 0)
return;
break;
+ case KERN_VIDEO:
+ len = sysctl_video(string, &bufp, mib, flags, &type);
+ if (len < 0)
+ return;
+ break;
case KERN_WITNESS:
len = sysctl_witness(string, &bufp, mib, flags, &type);
if (len < 0)
@@ -1766,6 +1773,7 @@ struct list shmlist = { shmname, KERN_SH
struct list watchdoglist = { watchdogname, KERN_WATCHDOG_MAXID };
struct list tclist = { tcname, KERN_TIMECOUNTER_MAXID };
struct list audiolist = { audioname, KERN_AUDIO_MAXID };
+struct list videolist = { videoname, KERN_VIDEO_MAXID };
struct list witnesslist = { witnessname, KERN_WITNESS_MAXID };
/*
@@ -2813,6 +2821,25 @@ sysctl_audio(char *string, char **bufpp,
return (-1);
mib[2] = indx;
*typep = audiolist.list[indx].ctl_type;
+ return (3);
+}
+
+/*
+ * Handle video support
+ */
+int
+sysctl_video(char *string, char **bufpp, int mib[], int flags, int *typep)
+{
+ int indx;
+
+ if (*bufpp == NULL) {
+ listall(string, &videolist);
+ return (-1);
+ }
+ if ((indx = findname(string, "third", bufpp, &videolist)) == -1)
+ return (-1);
+ mib[2] = indx;
+ *typep = videolist.list[indx].ctl_type;
return (3);
}
Index: sys/dev/usb/uvideo.c
===
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
retrieving revision 1.209
diff -u -p -u -r1.209 uvideo.c
--- sys/dev/usb/uvideo.c31 Jul 2020 10:49:33 - 1.209
+++ sys/dev/usb/uvideo.c27 Sep 2020 16:49:55 -
@@ -386,6 +386,12 @@ struct uvideo_devs {
#define uvideo_lookup(v, p) \
((struct uvideo_devs *)usb_lookup(uvideo_devs, v, p))
+/*
+ * Global flag to control if video recording is enabled when the
+ * kern.video.record setting is set to 1.
+ */
+int video_record_enable = 0;
+
int
uvideo_enable(void *v)
{
@@ -2210,7 +2216,8 @@ uvideo_vs_decode_stream_header(struct uv
/* save sample */
sample_len = frame_size - sh->bLength;
if ((fb->offs
Re: apmd(8) and hw.perfpolicy quirks
On Thu, Sep 24 2020, Jeremie Courreges-Anglas wrote:
> On Wed, Sep 23 2020, "Ted Unangst" wrote:
>> On 2020-09-23, Jeremie Courreges-Anglas wrote:
>>
>>> ok?
>>
>> Seems fine.
>>
>>
>>> Note: I inlined the apmd(8)->apm(8) perfpolicy conversion for now, which
>>> brings a question. I find it weird that there is a special "high"
>>> perfpolicy (effectively similar to perfpolicy=manual + setperf=100) but
>>> no "low" perfpolicy. Is "high" useful to anyone?
>>
>> If you're benchmarking or something, it's convenient to toggle between
>> high and auto. There's less use for low, since that's what auto defaults to.
>
> Well you can do auto->high easily with apm(8) -H or sysctl
> hw.perfpolicy=manual hw.setperf=100. I'm not sure a special "high"
> hw.perfpolicy choice brings much value and I would appreciate a simple
> "manual" vs "auto" situation.
>
> This has an impact on documentation. sysctl(2) doesn't mention that
> setting hw.perfpolicy=high also locks hw.setperf=100. The apm(8)
> manpage only talks about -H setting hw.setperf=100. The description for
> apmd(8) -H says
>
> Start apmd in *manual* performance adjustment mode,
> initialising hw.setperf to 100.
>
> which is not the whole story. If you use apm/apmd -H you can't later
> just run sysctl hw.setperf=50:
>
> sysctl: hw.setperf: Operation not permitted
The diff below teaches apmd(8) -H to set hw.perfpolicy="manual" and
hw.setperf=100, instead of setting hw.perfpolicy="high".
- matches the manpage
- apm(8) reporting becomes accurate
- more symmetry with -L ("low")
- avoids the "sysctl: hw.setperf: Operation not permitted" quirk
Teaching apm(8) how to print hw.perfpolicy="high" then becomes low
priority.
While here, simplify the sysctl(2) calls: in this function we don't care
about the old values.
ok?
Index: apmd.c
===
--- apmd.c.orig
+++ apmd.c
@@ -650,27 +650,27 @@ void
setperfpolicy(char *policy)
{
int hw_perfpol_mib[] = { CTL_HW, HW_PERFPOLICY };
- char oldpolicy[32];
- size_t oldsz = sizeof(oldpolicy);
- int setlo = 0;
+ int hw_perf_mib[] = { CTL_HW, HW_SETPERF };
+ int new_perf = -1;
if (strcmp(policy, "low") == 0) {
policy = "manual";
- setlo = 1;
+ new_perf = 0;
+ } else if (strcmp(policy, "high") == 0) {
+ policy = "manual";
+ new_perf = 100;
}
- if (sysctl(hw_perfpol_mib, 2, oldpolicy, &oldsz,
+ if (sysctl(hw_perfpol_mib, 2, NULL, NULL,
policy, strlen(policy) + 1) == -1)
logmsg(LOG_INFO, "cannot set hw.perfpolicy");
- if (setlo == 1) {
- int hw_perf_mib[] = {CTL_HW, HW_SETPERF};
- int perf;
- int new_perf = 0;
- size_t perf_sz = sizeof(perf);
- if (sysctl(hw_perf_mib, 2, &perf, &perf_sz, &new_perf, perf_sz)
== -1)
- logmsg(LOG_INFO, "cannot set hw.setperf");
- }
+ if (new_perf == -1)
+ return;
+
+ if (sysctl(hw_perf_mib, 2, NULL, NULL,
+ &new_perf, sizeof(new_perf)) == -1)
+ logmsg(LOG_INFO, "cannot set hw.setperf");
}
void
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
