Re: hidms: don't ignore mice with no x/y coordinates
On Mon, May 24, 2021 at 03:15:14PM BST, joshua stein wrote:
> A bug was reported where a Kensington USB trackball didn't work
> properly:
>
> uhidev4 at uhub0 port 6 configuration 1 interface 0 "Kensington Expert
> Wireless TB" rev 2.00/1.02 addr 9
> uhidev4: iclass 3/1, 3 report ids
> ums3 at uhidev4 reportid 1
> ums3: mouse has no X report
> ums4 at uhidev4 reportid 2: 0 button
> wsmouse3 at ums4 mux 0
>
> After looking at the HID report descriptor, this device is weird in
> that it puts the buttons and wheel on one report and the trackball
> X/Y coordinates an another. This causes uhidev to attach two ums
> devices, but the first one fails because there are no X/Y reports
> found.
>
> The proper fix is probably to make ums act like umt and use
> UHIDEV_CLAIM_MULTIPLE_REPORTID to find all of the necessary reports
> and attach to multiple at once if needed. I started working on this
> but all of the logic in hidms_setup gets tricky when it has to look
> at multiple reports. So an easier fix is to just not consider a
> mouse with no X/Y reports invalid.
>
> Now the device attaches to the first button/wheel report:
>
> uhidev4 at uhub4 port 4 configuration 1 interface 0 "Kensington Expert
> Wireless TB" rev 2.00/1.02 addr 3
> uhidev4: iclass 3/1, 3 report ids
> ums1 at uhidev4 reportid 1: 5 buttons, Z and W dir
> wsmouse1 at ums1 mux 0
> ums2 at uhidev4 reportid 2: 0 buttons
> wsmouse2 at ums2 mux 0
>
> Checking dmesglog for 'no X report' yields a lot of results, so this
> may help on other devices.
Hi all,
Just an FYI, this is the trackball in question:
https://www.kensington.com/en-gb/p/products/control/trackballs/expert-mouse-wireless-trackball-1/
In its current state, it can only be used as a paperweight as, when
plugged in, only the trackball itself (pointer) works but none of
its four buttons or scroll ring do - they aren't being recognised
at all. It works just fine on Linux, macOS, or Windows, though.
With the below patch, everything's fine - all four buttons and the
scroll ring are being recognised and work as expected.
Any chance of this getting in?
Regards,
Raf
> diff --git sys/dev/hid/hidms.c sys/dev/hid/hidms.c
> index ab9cd9c797e..92ca89537da 100644
> --- sys/dev/hid/hidms.c
> +++ sys/dev/hid/hidms.c
> @@ -76,10 +76,9 @@ hidms_setup(struct device *self, struct hidms *ms,
> uint32_t quirks,
> ms->sc_flags = quirks;
>
> if (!hid_locate(desc, dlen, HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_X), id,
> - hid_input, >sc_loc_x, )) {
> - printf("\n%s: mouse has no X report\n", self->dv_xname);
> - return ENXIO;
> - }
> + hid_input, >sc_loc_x, ))
> + ms->sc_loc_x.size = 0;
> +
> switch(flags & MOUSE_FLAGS_MASK) {
> case 0:
> ms->sc_flags |= HIDMS_ABSX;
> @@ -93,10 +92,9 @@ hidms_setup(struct device *self, struct hidms *ms,
> uint32_t quirks,
> }
>
> if (!hid_locate(desc, dlen, HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_Y), id,
> - hid_input, >sc_loc_y, )) {
> - printf("\n%s: mouse has no Y report\n", self->dv_xname);
> - return ENXIO;
> - }
> + hid_input, >sc_loc_y, ))
> + ms->sc_loc_y.size = 0;
> +
> switch(flags & MOUSE_FLAGS_MASK) {
> case 0:
> ms->sc_flags |= HIDMS_ABSY;
> @@ -292,7 +290,7 @@ hidms_attach(struct hidms *ms, const struct
> wsmouse_accessops *ops)
> #endif
>
> printf(": %d button%s",
> - ms->sc_num_buttons, ms->sc_num_buttons <= 1 ? "" : "s");
> + ms->sc_num_buttons, ms->sc_num_buttons == 1 ? "" : "s");
> switch (ms->sc_flags & (HIDMS_Z | HIDMS_W)) {
> case HIDMS_Z:
> printf(", Z dir");
>
Re: add table_procexec in smtpd
Hi,
I've attached a slightly updated patch for the procexec.
Ping for someone to take a look :)
Cheers,
Aisha
diff --git a/usr.sbin/smtpd/smtpctl/Makefile b/usr.sbin/smtpd/smtpctl/Makefile
index ef8148be8c9..2e8beff1ad1 100644
--- a/usr.sbin/smtpd/smtpctl/Makefile
+++ b/usr.sbin/smtpd/smtpctl/Makefile
@@ -48,6 +48,7 @@ SRCS+=table_static.c
SRCS+= table_db.c
SRCS+= table_getpwnam.c
SRCS+= table_proc.c
+SRCS+= table_procexec.c
SRCS+= unpack_dns.c
SRCS+= spfwalk.c
diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h
index be934112103..221f24fbdc4 100644
--- a/usr.sbin/smtpd/smtpd.h
+++ b/usr.sbin/smtpd/smtpd.h
@@ -1656,6 +1656,7 @@ int table_regex_match(const char *, const char *);
void table_open_all(struct smtpd *);
void table_dump_all(struct smtpd *);
void table_close_all(struct smtpd *);
+const char *table_service_name(enum table_service );
/* to.c */
diff --git a/usr.sbin/smtpd/smtpd/Makefile b/usr.sbin/smtpd/smtpd/Makefile
index b31d4e42224..debfc8d8ab7 100644
--- a/usr.sbin/smtpd/smtpd/Makefile
+++ b/usr.sbin/smtpd/smtpd/Makefile
@@ -63,6 +63,7 @@ SRCS+=compress_gzip.c
SRCS+= table_db.c
SRCS+= table_getpwnam.c
SRCS+= table_proc.c
+SRCS+= table_procexec.c
SRCS+= table_static.c
SRCS+= queue_fs.c
diff --git a/usr.sbin/smtpd/table.c b/usr.sbin/smtpd/table.c
index 1d82d88b81a..0c67d205065 100644
--- a/usr.sbin/smtpd/table.c
+++ b/usr.sbin/smtpd/table.c
@@ -46,8 +46,8 @@ extern struct table_backend table_backend_static;
extern struct table_backend table_backend_db;
extern struct table_backend table_backend_getpwnam;
extern struct table_backend table_backend_proc;
+extern struct table_backend table_backend_procexec;
-static const char * table_service_name(enum table_service);
static int table_parse_lookup(enum table_service, const char *, const char *,
union lookup *);
static int parse_sockaddr(struct sockaddr *, int, const char *);
@@ -59,6 +59,7 @@ static struct table_backend *backends[] = {
_backend_db,
_backend_getpwnam,
_backend_proc,
+ _backend_procexec,
NULL
};
@@ -77,7 +78,7 @@ table_backend_lookup(const char *backend)
return NULL;
}
-static const char *
+const char *
table_service_name(enum table_service s)
{
switch (s) {
diff --git a/usr.sbin/smtpd/table_procexec.c b/usr.sbin/smtpd/table_procexec.c
new file mode 100644
index 000..88bfc435fb3
--- /dev/null
+++ b/usr.sbin/smtpd/table_procexec.c
@@ -0,0 +1,346 @@
+/*
+ * Copyright (c) 2013 Eric Faurot
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#include "smtpd.h"
+#include "log.h"
+
+#define PROTOCOL_VERSION "1"
+
+static int table_procexec_open(struct table *);
+static int table_procexec_update(struct table *);
+static void table_procexec_close(struct table *);
+static int table_procexec_lookup(struct table *, enum table_service, const
char *, char **);
+static int table_procexec_fetch(struct table *, enum table_service, char **);
+
+struct table_backend table_backend_procexec = {
+ "proc-exec",
+ K_ANY,
+ NULL,
+ NULL,
+ NULL,
+ table_procexec_open,
+ table_procexec_update,
+ table_procexec_close,
+ table_procexec_lookup,
+ table_procexec_fetch,
+};
+
+struct procexec_handle {
+ FILE*backend_w;
+ FILE*backend_r;
+ pid_t pid;
+};
+
+static int
+table_procexec_open(struct table *t) {
+ struct procexec_handle *pe_handle;
+ pid_t pid;
+ int sp[2];
+ int execr;
+ char exec[_POSIX_ARG_MAX];
+
+ if (socketpair(AF_UNIX, SOCK_STREAM, PF_UNSPEC, sp) == -1){
+ fatalx("procexec - socket pair: %s", t->t_name);
+ }
+
+ pe_handle = xcalloc(1, sizeof(*pe_handle));
+
+ if ((pid = fork()) == -1) {
+ fatalx("procexec - fork: %s", t->t_name);
+ }
+
+ if (pid > 0) {
+ close(sp[0]);
+ FILE *backend_w, *backend_r;
+ if ((backend_w = fdopen(sp[1], "w")) == NULL)
+
vscsi/iscsid: wait for scsi_probe to complete after connections are established
I have been working on fixing an issue (which was partially fixed by a
diff I sent in earlier this year) with iscsid. With iscsi disks in
/etc/fstab, sometimes the devices aren't fully up and ready before fsck
tries to run - causing the machine to go into single user mode on boot.
The diff that was merged earlier in the year added a poll routine which
monitors for connection success before letting iscsictl return. This
fixed the issue in some cases, however it's still only half the fix. The
principled fix is to, additionally, wait until the scsi_probe calls are
complete - at which point we can reasonably assume the disk device are
ready for use. This requires some work to the vscsi driver to make this
happen, as well as changes to both iscsid and iscsictl. The diffs
attached here are a full implementation of this.
I was still encountering this issue on one of my machines (much slower
than my normal machine), where the connections would be up but the
scsi_probe calls would not have completed - even with my earlier diff
this would still cause the machine to go to single user mode. However,
this indeed fixes that problem completely and I've been running it for a
couple of weeks with no problems.
The diffs are designed to be applied in the order they appear. In
summary, the proposed changes are:
(1) taskq.diff adds a taskq_empty function, which lets us check to see
if a taskq is, well, empty. This is used in (2). Updates the man pages
for taskq/task_add to reflect this.
(2) vscsi.diff adds an ioctl to the vscsi driver which lets us monitor
for device event queue completion. To aid in this it also separates
calls to scsi_probe and scsi_detach to a dedicated taskq, rather than
using systq. Updates the man pages for vscsi to reflect this.
(3) iscsid.diff does the plumbing to actually call the new ioctl and
provide that information back to iscsictl during status poll.
(4) iscsictl.diff integrates the device queue information into the
polling routine. Updates the man page for iscsictl to describe the new
behavior.
Based on commmit messages around vscsi it seems there was some plan to
do this quite some years ago but it's hard for me to know what the
proposed method was (though I assume what was envisaged might be similar
to something like this).
Feedback sought and greatly welcomed. I am basically certain there are
ways this can be improved.
Thanks,
Ash
Index: sys/sys/task.h
===
RCS file: /cvs/src/sys/sys/task.h,v
retrieving revision 1.18
diff -u -p -u -p -r1.18 task.h
--- sys/sys/task.h 1 Aug 2020 08:40:20 - 1.18
+++ sys/sys/task.h 8 Jun 2021 23:42:00 -
@@ -51,6 +51,8 @@ void taskq_barrier(struct taskq *);
void taskq_del_barrier(struct taskq *, struct task *);
+int taskq_empty(struct taskq *);
+
void task_set(struct task *, void (*)(void *), void *);
int task_add(struct taskq *, struct task *);
int task_del(struct taskq *, struct task *);
Index: sys/kern/kern_task.c
===
RCS file: /cvs/src/sys/kern/kern_task.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 kern_task.c
--- sys/kern/kern_task.c 1 Aug 2020 08:40:20 - 1.31
+++ sys/kern/kern_task.c 8 Jun 2021 23:42:16 -
@@ -394,6 +394,18 @@ task_del(struct taskq *tq, struct task *
}
int
+taskq_empty(struct taskq *tq)
+{
+ int rv;
+
+ mtx_enter(>tq_mtx);
+ rv = TAILQ_EMPTY(>tq_worklist);
+ mtx_leave(>tq_mtx);
+
+ return (rv);
+}
+
+int
taskq_next_work(struct taskq *tq, struct task *work)
{
struct task *next;
Index: share/man/man9/task_add.9
===
RCS file: /cvs/src/share/man/man9/task_add.9,v
retrieving revision 1.22
diff -u -p -u -p -r1.22 task_add.9
--- share/man/man9/task_add.9 8 Jun 2020 00:29:51 - 1.22
+++ share/man/man9/task_add.9 8 Jun 2021 23:42:29 -
@@ -20,6 +20,7 @@
.Sh NAME
.Nm taskq_create ,
.Nm taskq_destroy ,
+.Nm taskq_empty ,
.Nm taskq_barrier ,
.Nm taskq_del_barrier ,
.Nm task_set ,
@@ -43,6 +44,8 @@
.Fn taskq_barrier "struct taskq *tq"
.Ft void
.Fn taskq_del_barrier "struct taskq *tq" "struct task *t"
+.Ft int
+.Fn taskq_empty "struct taskq *tq"
.Ft void
.Fn task_set "struct task *t" "void (*fn)(void *)" "void *arg"
.Ft int
@@ -86,6 +89,9 @@ argument:
The threads servicing the taskq will be run without the kernel big lock.
.El
.Pp
+.Fn taskq_empty
+indicates whether there are any queued tasks.
+.Pp
.Fn taskq_destroy
causes the resources associated with a previously created taskq to be freed.
It will wait till all the tasks in the work queue are completed before
@@ -220,6 +226,11 @@ or 0 if the task was not already on the
.Fn task_pending
will return non-zero if the task is queued to run, or 0 if the task
is not queued.
+.Pp
+.Fn taskq_empty
+will return 1 if there are no tasks queued, or 0 if there is at least
+one task queued.
+
.Sh SEE ALSO
.Xr autoconf 9 ,
.Xr spl 9
Re: [External] : Re: parallel forwarding vs. bridges
Hello David,
I'm still not sure if your change is ultimate fix, or just significantly
minimizes risk of the bug. If I understand things right, the problem we are
trying to solve:
DIOCGETSTATES we have in current, grabs NET_LOCK() and pf_state_lock
as a reader.
it then walks through the whole state list and copies out (copyout(9f))
data for each state into ioctl(2) buffer provided by calling process.
we may trip assert down in copyout(9f):
> panic: acquiring blockable sleep lock with spinlock or critical section
> held (rwlock) vmmaplk
> Stopped at db_enter+0x10: popq%rbp
> TIDPIDUID PRFLAGS PFLAGS CPU COMMAND
> *512895 28841 0 0x3 03K pfctl
> db_enter() at db_enter+0x10
> panic(81e19411) at panic+0x12a
> witness_checkorder(fd83b09b4d18,1,0) at witness_checkorder+0xbce
> rw_enter_read(fd83b09b4d08) at rw_enter_read+0x38
> uvmfault_lookup(8000238e3418,0) at uvmfault_lookup+0x8a
> uvm_fault_check(8000238e3418,8000238e3450,8000238e3478) at
> uvm_fault_check+0x32
> uvm_fault(fd83b09b4d00,e36553c000,0,2) at uvm_fault+0xfc
> kpageflttrap(8000238e3590,e36553c000) at kpageflttrap+0x131
> kerntrap(8000238e3590) at kerntrap+0x91
> alltraps_kern_meltdown() at alltraps_kern_meltdown+0x7b
> copyout() at copyout+0x53
I'm just afraid that although your change significantly reduces the risk we
will die with similar call stack as the one above, the new code is not bullet
proof. We still do copyout() while holding pf_state_list.pfs_rwl as a reader
(in pf_states_get() from your diff). I agree packets do not grab
pf_state_list.pfs_rwl at all. Your fix solves this problem in this respect.
let's take a look at this part of pf_purge_expired_states()
from your diff:
1543 NET_LOCK();
1544 rw_enter_write(_state_list.pfs_rwl);
1545 PF_LOCK();
1546 PF_STATE_ENTER_WRITE();
1547 SLIST_FOREACH(st, , gc_list) {
1548 if (st->timeout != PFTM_UNLINKED)
1549 pf_remove_state(st);
1550
1551 pf_free_state(st);
1552 }
1553 PF_STATE_EXIT_WRITE();
1554 PF_UNLOCK();
1555 rw_exit_write(_state_list.pfs_rwl);
at line 1543 we grab NET_LOCK(), at line 1544 we are trying
to grab new lock (pf_state_list.pfs_rwl) exclusively.
with your change we might be running into situation, where we do copyout() as a
reader on pf_state_list.pfs_rwl. Then we grab NET_LOCK() and attempt to acquire
pf_state_list.pfs_rwl exclusively, which is still occupied by guy, who might be
doing uvm_fault() in copyout(9f).
I'm just worried we may be trading one bug for another bug. may be my concern
is just a false alarm here. I don't know.
anyway there are few more nits in your diff.
> Index: pf.c
> ===
> RCS file: /cvs/src/sys/net/pf.c,v
> retrieving revision 1.1118
> diff -u -p -r1.1118 pf.c
> --- pf.c 1 Jun 2021 09:57:11 - 1.1118
> +++ pf.c 3 Jun 2021 06:24:48 -
> @@ -1247,7 +1278,8 @@ pf_purge_expired_rules(void)
> void
> pf_purge_timeout(void *unused)
> {
> - task_add(net_tq(0), _purge_task);
> + /* XXX move to systqmp to avoid KERNEL_LOCK */
> + task_add(systq, _purge_task);
> }
I would just clean up the comment. looks like we should be
able to get pf's ioctl operations out of KERNEL_LOCK completely.
I'll take a further look at it, while be working in pf_ioctl.c
> @@ -1280,11 +1311,10 @@ pf_purge(void *xnloops)
>* Fragments don't require PF_LOCK(), they use their own lock.
>*/
> if ((*nloops) >= pf_default_rule.timeout[PFTM_INTERVAL]) {
> - pf_purge_expired_fragments();
> + pfgpurge_expired_fragment(s);
> *nloops = 0;
> }
> NET_UNLOCK();
> - KERNEL_UNLOCK();
>
> timeout_add_sec(_purge_to, 1);
> }
I guess 'pfgpurge_expired_fragment(s);' is unintentional change, right?
> Index: pfvar_priv.h
> ===
> RCS file: /cvs/src/sys/net/pfvar_priv.h,v
> retrieving revision 1.6
> diff -u -p -r1.6 pfvar_priv.h
> --- pfvar_priv.h 9 Feb 2021 14:06:19 - 1.6
> +++ pfvar_priv.h 3 Jun 2021 06:24:48 -
> @@ -38,6 +38,115 @@
> #ifdef _KERNEL
>
> #include
> +#include
> +
> +/*
> +
> +states are linked into a global list to support the following
> +functionality:
> +
> + - garbage collection
> + - pfsync bulk send operations
> + - bulk state fetches via the DIOCGETSTATES ioctl
> + - bulk state clearing via the DIOCCLRSTATES ioctl
> +
> +states are inserted into the global pf_state_list once it has also
> +been successfully added to the various trees that make up the state
> +table. states are only removed from the pf_state_list by the garbage
> +collection process.
> +
the multiline comment does not match style, I think.
I kind of
Read/Write whole fusebufs
Hello tech@
Due to the challenges of having a large diff reviewed I've had another
think about how I can break up the FUSE changes so that they are smaller
and easier to review.
This is the first of these diffs.
The current design uses a fixed size fusebuf that consists of a header
and a union of structs that are used for different VFS operations. In
addition, there may be data of variable size associated with an
operation. e.g. the buffer passed to write(2). see fb_setup(9).
If there is additional data to be exchanged between libfuse and the
kernel then libfuse uses an ioctl on the device to read or write this
variable sized data after the fusebuf has been read or written. This is
not how the fuse protocol works on Linux. Instead, the fusebuf is read
or written in a single read(2) or write(2). This change is the first
step in setting the OpenBSD implementation up for improved compatibility
in the fuse kernel interface.
The fusebuf struct is shared between the kernel and libfuse but its
layout differs slightly between the two. The kernel has knowledge of the
size of data that it is sending or receiving (e.g. read, write, readdir,
link, lookup) and so can malloc the exact amount of memory required.
libfuse must read the entire fusebuf but doesn't know its size in
advance so must have a buffer large enough to cater for the worst case
scenario. Since libfuse now uses a fixed size fusebuf, it no longer
needs to free the variable memory previously allocated for the data.
stsp@ has been kind enough to provide initial feedback. Is it now ready
for an official OK?
Index: lib/libfuse/fuse.c
===
RCS file: /cvs/src/lib/libfuse/fuse.c,v
retrieving revision 1.51
diff -u -p -r1.51 fuse.c
--- lib/libfuse/fuse.c 28 Jun 2019 13:32:42 - 1.51
+++ lib/libfuse/fuse.c 8 Jun 2021 14:15:29 -
@@ -154,9 +154,9 @@ fuse_loop(struct fuse *fuse)
{
struct fusebuf fbuf;
struct fuse_context ctx;
- struct fb_ioctl_xch ioexch;
struct kevent event[5];
struct kevent ev;
+ ssize_t fbuf_size;
ssize_t n;
int ret;
@@ -201,29 +201,15 @@ fuse_loop(struct fuse *fuse)
strsignal(signum));
}
} else if (ret > 0) {
- n = read(fuse->fc->fd, , sizeof(fbuf));
- if (n != sizeof(fbuf)) {
+ n = read(fuse->fc->fd, , FUSEBUFSIZE);
+ fbuf_size = sizeof(fbuf.fb_hdr) + sizeof(fbuf.FD) +
+ fbuf.fb_len;
+ if (n != fbuf_size) {
fprintf(stderr, "%s: bad fusebuf read\n",
__func__);
return (-1);
}
- /* check if there is data something present */
- if (fbuf.fb_len) {
- fbuf.fb_dat = malloc(fbuf.fb_len);
- if (fbuf.fb_dat == NULL)
- return (-1);
- ioexch.fbxch_uuid = fbuf.fb_uuid;
- ioexch.fbxch_len = fbuf.fb_len;
- ioexch.fbxch_data = fbuf.fb_dat;
-
- if (ioctl(fuse->fc->fd, FIOCGETFBDAT,
- ) == -1) {
- free(fbuf.fb_dat);
- return (-1);
- }
- }
-
ctx.fuse = fuse;
ctx.uid = fbuf.fb_uid;
ctx.gid = fbuf.fb_gid;
@@ -238,26 +224,13 @@ fuse_loop(struct fuse *fuse)
return (-1);
}
- n = write(fuse->fc->fd, , sizeof(fbuf));
- if (fbuf.fb_len) {
- if (fbuf.fb_dat == NULL) {
- fprintf(stderr, "%s: fb_dat is Null\n",
- __func__);
- return (-1);
- }
- ioexch.fbxch_uuid = fbuf.fb_uuid;
- ioexch.fbxch_len = fbuf.fb_len;
- ioexch.fbxch_data = fbuf.fb_dat;
-
- if (ioctl(fuse->fc->fd, FIOCSETFBDAT, )
== -1) {
- free(fbuf.fb_dat);
- return (-1);
- }
- free(fbuf.fb_dat);
- }
+ fbuf_size = sizeof(fbuf.fb_hdr) + sizeof(fbuf.FD) +
+ fbuf.fb_len;
+ n = write(fuse->fc->fd, , fbuf_size);
+
Re: Patch: ksh: fix input handling for 4 byte UTF-8 sequences
Looks good to me, ok nicm
On Wed, Jun 02, 2021 at 09:00:16PM +0200, Ingo Schwarze wrote:
> Hi,
>
> feeling hesitant to commit into ksh without at least one proper OK,
> i'm resending this patch here, sorry if i missed private feedback.
>
> What the existing code does:
> It tries to make sure that multi-byte UTF-8 characters get passed on by
> the shell without fragmentation, not one byte at time. I heard people
> say that some software, for example tmux(1), may sometimes get confused
> when receiving a UTF-8 character in a piecemeal manner.
>
> Which problem needs fixing:
> Of the four-byte UTF-8 sequences, only a subset is identified by the
> existing code. The other four-byte UTF-8 sequences still get chopped
> up resulting in individual bytes being passed on.
>
>
> I'm also adding a few comments as suggested by jca@. Parsing of UTF-8
> is less trivial than one might think, witnessed once again by the fact
> that i got this code wrong in the first place.
>
> I also changed "cc & 0x20" to "cc > 0x9f" and "cc & 0x30" to "cc > 0x8f"
> for uniformity and readabilty - UTF-8-parsing is bad enough without
> needless micro-optimization, right?
>
>
> Note that even with the patch below, moving backward and forward
> over a blowfish icon on the command line still does not work because
> the character is width 2 and the ksh code intentionally does not
> use wcwidth(3). But maybe it improves something in tmux? Not sure.
>
> Either way, unless it causes regressions, this (or a further improved
> version) should go in because what is there is clearly wrong.
>
> OK?
> Ingo
>
>
> Index: emacs.c
> ===
> RCS file: /cvs/src/bin/ksh/emacs.c,v
> retrieving revision 1.87
> diff -u -p -r1.87 emacs.c
> --- emacs.c 8 May 2020 14:30:42 - 1.87
> +++ emacs.c 13 May 2021 18:16:13 -
> @@ -1851,11 +1851,17 @@ x_e_getu8(char *buf, int off)
> return -1;
> buf[off++] = c;
>
> - if (c == 0xf4)
> + /*
> + * In the following, comments refer to violations of
> + * the inequality tests at the ends of the lines.
> + * See the utf8(7) manual page for details.
> + */
> +
> + if ((c & 0xf8) == 0xf0 && c < 0xf5) /* beyond Unicode */
> len = 4;
> else if ((c & 0xf0) == 0xe0)
> len = 3;
> - else if ((c & 0xe0) == 0xc0 && c > 0xc1)
> + else if ((c & 0xe0) == 0xc0 && c > 0xc1) /* use single byte */
> len = 2;
> else
> len = 1;
> @@ -1865,9 +1871,10 @@ x_e_getu8(char *buf, int off)
> if (cc == -1)
> break;
> if (isu8cont(cc) == 0 ||
> - (c == 0xe0 && len == 3 && cc < 0xa0) ||
> - (c == 0xed && len == 3 && cc & 0x20) ||
> - (c == 0xf4 && len == 4 && cc & 0x30)) {
> + (c == 0xe0 && len == 3 && cc < 0xa0) || /* use 2 bytes */
> + (c == 0xed && len == 3 && cc > 0x9f) || /* surrogates */
> + (c == 0xf0 && len == 4 && cc < 0x90) || /* use 3 bytes */
> + (c == 0xf4 && len == 4 && cc > 0x8f)) { /* beyond Uni. */
> x_e_ungetc(cc);
> break;
> }
ssh/sshd configuration parsing
Hi, I just committed some changes to ssh/sshd configuration parsing that have been in snaps for the last few days. These changes switch parsing from using a naive tokeniser to one that better follows shell-style rules for quoting and comments. This does make config parsing stricter in a number of cases, e.g. it was previously possible for sshd_config to have a AllowUsers option alone on a line with no arguments (it was pretty nonsensical to do so, since it had zero effect), but the new parser will reject this as well as a few other weird cases. The benefits of the new code are better handling of quoted strings, e.g. with escaped quotes and a fix for a regression caused by adding support for comments in ssh_config a few releases ago. These changes do touch something that is likely used in ways that I haven't thought of and the regress tests don't cover :) If you spot weirdness, regressions or your previously-valid configurations do not parse afterwards, then please let let bugs@ or openssh@ know ASAP. Thanks, Damien
