Re: riscv64 pmap: sync memory writes before remote sfence.vma
> From: Jeremie Courreges-Anglas
> Date: Mon, 31 Oct 2022 22:44:01 +0100
>
>
> Here's the diff I've been using for the last bulk build on riscv64. It
> resulted in a single kernel crash:
>
> cpu1: pool_do_get: pted free list modified: page 0xffc22cc0a000; item
> addr
> 0xffc22cc0a6d0; offset 0x0=0xa06137b98e6f6767 != 0xa06137b98ed06767
>
> and 5 userland clang crashes. Yes, that is an improvement over the
> 18, 27 and 22 clang crashes in the three previous bulk builds.
> But it doesn't fix the problem(s) we're still chasing.
>
> The rationale is basically the text from the spec, I added rather
> verbose comments quoting said spec, much like what exists in
> icache_flush(). But we can make it shorter if wanted. Maybe just
> "SFENCE.VMA orders only the local hart's implicit references to the
> memory-management data structures."?
>
> ok?
ok kettenis@
> Bonus: sifive_cip_1200_errata is a remnant, cpu_errata_sifive_cip_1200
> is the actual variable used on affected CPUs. To be committed
> separately.
>
>
> Index: pmap.c
> ===
> RCS file: /cvs/src/sys/arch/riscv64/riscv64/pmap.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 pmap.c
> --- pmap.c17 Oct 2022 19:51:54 - 1.24
> +++ pmap.c31 Oct 2022 21:15:07 -
> @@ -42,8 +42,6 @@ pmap_is_active(struct pmap *pm, struct c
>
> #endif
>
> -int sifive_cip_1200_errata;
> -
> void
> do_tlb_flush_page(pmap_t pm, vaddr_t va)
> {
> @@ -59,8 +57,23 @@ do_tlb_flush_page(pmap_t pm, vaddr_t va)
> hart_mask |= (1UL << ci->ci_hartid);
> }
>
> - if (hart_mask != 0)
> + /*
> + * From the RISC-V privileged spec:
> + *
> + * SFENCE.VMA orders only the local hart's implicit references
> + * to the memory-management data structures. Consequently, other
> + * harts must be notified separately when the memory-management
> + * data structures have been modified. One approach is to use 1)
> + * a local data fence to ensure local writes are visible
> + * globally, then 2) an interprocessor interrupt to the other
> + * thread, then 3) a local SFENCE.VMA in the interrupt handler
> + * of the remote thread, and finally 4) signal back to
> + * originating thread that operation is complete.
> + */
> + if (hart_mask != 0) {
> + membar_sync();
> sbi_remote_sfence_vma(_mask, va, PAGE_SIZE);
> + }
> #endif
>
> sfence_vma_page(va);
> @@ -81,8 +94,23 @@ do_tlb_flush(pmap_t pm)
> hart_mask |= (1UL << ci->ci_hartid);
> }
>
> - if (hart_mask != 0)
> + /*
> + * From the RISC-V privileged spec:
> + *
> + * SFENCE.VMA orders only the local hart's implicit references
> + * to the memory-management data structures. Consequently, other
> + * harts must be notified separately when the memory-management
> + * data structures have been modified. One approach is to use 1)
> + * a local data fence to ensure local writes are visible
> + * globally, then 2) an interprocessor interrupt to the other
> + * thread, then 3) a local SFENCE.VMA in the interrupt handler
> + * of the remote thread, and finally 4) signal back to
> + * originating thread that operation is complete.
> + */
> + if (hart_mask != 0) {
> + membar_sync();
> sbi_remote_sfence_vma(_mask, 0, -1);
> + }
> #endif
>
> sfence_vma();
>
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
>
>
Re: libX11 patch for X*IfEvent() API issues
Hello Matthieu,
On Nov 01 2022, Matthieu Herrb wrote:
> Hi,
>
> here's a libX11 patch that needs some wide testing, especially from
> people who have experienced issues with various applications (fvwm[23]
> from ports, Motif applications with Drag'n'Drop,..) with the upgrade
> to libX11 1.8.1, before I added the --disable-thread-safety-constructor
> option to the build.
>
> This patch allows the callback functions from X*IfEvent() class of
> Xlib functions to re-enter libX11, by making the
> XlockDisplay()/XunlockDisplay() functions void while running the
> callbacks.
>
> I've tested fvwm2 with this patch and it seems to fix it (but since
> the issue was never easy to reproduce I'm not 100% confident).
>
> Please apply and report
For me fvwm2 and fvwm3 crash again.
The bug is the one I explained to you in a private message:
> I'm able to make fvwm2 and fvwm3 to abort, in this case with a clean X
> exit, by including any xapp with the -iconic option in the InitFunction or
> the StartFuncion:
>
> AddToFunc InitFunction I Exec exec xconsole -iconic
>
> Just removing the "-iconic" option it doesn't happen. Besides,
> iconifying to FvwmIconMan doesn't do the trick, it's necessary to NOT
> have the following option enabled (as is the case in latest default
> fvwm2 config):
>
> #Style * !Icon
>
>
> So, unless it happens only to me, as always, you should reproduce it
> just coping the default fvwm2 config to your home directory and adding
> three lines.
>
> $ cp -r /usr/locar/share/fvwm/default-config ~/.fvwm
> $ echo "AddToFunc InitFunction I Exec exec xconsole -iconic" >> ~/.fvwm/config
> $ echo "Style * Icon" >> ~/.fvwm/config
***
>
> cd /usr/xenocara/lib/libX11
> patch -p0 -E < /path/to/this/patch
> doas make -f Makefile.bsd-wrapper obj
> doas make -f Makefile.bsd-wrapper build
>
> Then restart X with the applications that where crashing last august.
>
> Index: Makefile.bsd-wrapper
> ===
> RCS file: /cvs/OpenBSD/xenocara/lib/libX11/Makefile.bsd-wrapper,v
> retrieving revision 1.29
> diff -u -p -u -r1.29 Makefile.bsd-wrapper
> --- Makefile.bsd-wrapper 3 Sep 2022 06:55:25 - 1.29
> +++ Makefile.bsd-wrapper 1 Nov 2022 09:21:34 -
> @@ -14,7 +14,6 @@ SHARED_LIBS=X11 18.0 X11_xcb 2.0
>
> CONFIGURE_ARGS= --enable-tcp-transport --enable-unix-transport --enable-ipv6
> \
> --disable-composecache \
> - --disable-thread-safety-constructor \
> --without-xmlto --without-fop --without-xsltproc
>
> .include
> Index: include/X11/Xlibint.h
> ===
> RCS file: /cvs/OpenBSD/xenocara/lib/libX11/include/X11/Xlibint.h,v
> retrieving revision 1.15
> diff -u -p -u -r1.15 Xlibint.h
> --- include/X11/Xlibint.h 21 Feb 2022 08:01:24 - 1.15
> +++ include/X11/Xlibint.h 1 Nov 2022 09:21:34 -
> @@ -207,6 +207,7 @@ struct _XDisplay
>
> XIOErrorExitHandler exit_handler;
> void *exit_handler_data;
> +Bool in_ifevent;
> };
>
> #define XAllocIDs(dpy,ids,n) (*(dpy)->idlist_alloc)(dpy,ids,n)
> Index: src/ChkIfEv.c
> ===
> RCS file: /cvs/OpenBSD/xenocara/lib/libX11/src/ChkIfEv.c,v
> retrieving revision 1.4
> diff -u -p -u -r1.4 ChkIfEv.c
> --- src/ChkIfEv.c 30 May 2011 19:19:38 - 1.4
> +++ src/ChkIfEv.c 1 Nov 2022 09:21:35 -
> @@ -50,6 +50,7 @@ Bool XCheckIfEvent (
> int n; /* time through count */
>
> LockDisplay(dpy);
> +dpy->in_ifevent = True;
> prev = NULL;
> for (n = 3; --n >= 0;) {
> for (qelt = prev ? prev->next : dpy->head;
> @@ -60,6 +61,7 @@ Bool XCheckIfEvent (
> *event = qelt->event;
> _XDeq(dpy, prev, qelt);
> _XStoreEventCookie(dpy, event);
> +dpy->in_ifevent = False;
> UnlockDisplay(dpy);
> return True;
> }
> @@ -78,6 +80,7 @@ Bool XCheckIfEvent (
> /* another thread has snatched this event */
> prev = NULL;
> }
> +dpy->in_ifevent = False;
> UnlockDisplay(dpy);
> return False;
> }
> Index: src/IfEvent.c
> ===
> RCS file: /cvs/OpenBSD/xenocara/lib/libX11/src/IfEvent.c,v
> retrieving revision 1.4
> diff -u -p -u -r1.4 IfEvent.c
> --- src/IfEvent.c 30 May 2011 19:19:38 - 1.4
> +++ src/IfEvent.c 1 Nov 2022 09:21:35 -
> @@ -49,6 +49,7 @@ XIfEvent (
> unsigned long qe_serial = 0;
>
> LockDisplay(dpy);
> +dpy->in_ifevent = True;
> prev = NULL;
> while (1) {
> for (qelt = prev ? prev->next : dpy->head;
> @@ -59,6 +60,7 @@ XIfEvent (
> *event = qelt->event;
> _XDeq(dpy, prev, qelt);
>
Re: ber.c: Fix some minor issues in ober_read_element()
On Wed, 2022-11-02 at 18:34 +0100, Claudio Jeker wrote:
> On Wed, Nov 02, 2022 at 05:56:21PM +0100, Martijn van Duren wrote:
> > On Wed, 2022-11-02 at 17:47 +0100, Claudio Jeker wrote:
> > > On Wed, Nov 02, 2022 at 05:25:12PM +0100, Martijn van Duren wrote:
> > > > On Wed, 2022-11-02 at 17:00 +0100, Claudio Jeker wrote:
> > > > > On Wed, Nov 02, 2022 at 07:33:14AM +0100, Martijn van Duren wrote:
> > > > > > I found 2 minor issues in the handling of sequences/sets in
> > > > > > ober_read_element():
> > > > > > 1) An empty sequence/set (which is basically always) unconditionally
> > > > > >creates an (uninitialised) sub-element. Add the same length check
> > > > > >used to check the next element
> > > > > > 2) For each sub-element r is only checked for -1, but not if it
> > > > > >overflows the length of the sequence itself. This is not a big
> > > > > > risk
> > > > > >since each sub-element is length-checked against the buffer
> > > > > >availability and simply returns an ECANCELED, which would be no
> > > > > >worse memory-wise than sending an extremely large packet.
> > > > > >
> > > > > > While here, only having the NULL of the comparison on the next line
> > > > > > annoyed me.
> > > > > >
> > > > > > OK?
> > > > >
> > > > > See below.
> > > > >
> > > > > > martijn@
> > > > > >
> > > > > > Index: ber.c
> > > > > > ===
> > > > > > RCS file: /cvs/src/lib/libutil/ber.c,v
> > > > > > retrieving revision 1.23
> > > > > > diff -u -p -r1.23 ber.c
> > > > > > --- ber.c 21 Oct 2021 08:17:33 - 1.23
> > > > > > +++ ber.c 2 Nov 2022 06:28:33 -
> > > > > > @@ -1375,7 +1375,7 @@ ober_read_element(struct ber *ber, struc
> > > > > > break;
> > > > > > case BER_TYPE_SEQUENCE:
> > > > > > case BER_TYPE_SET:
> > > > > > - if (elm->be_sub == NULL) {
> > > > > > + if (len > 0 && elm->be_sub == NULL) {
> > > > > > if ((elm->be_sub = ober_get_element(0)) == NULL)
> > > > > > return -1;
> > > > > > }
> > > > >
> > > > > OK, be_sub == NULL is something checked and better than an empty
> > > > > object.
> > > >
> > > > I'm not sure I understand what you mean here.
> > >
> > > I just wanted to say that elm->be_sub == NULL is a condition that most
> > > other code handles well. While having an empty element in elm->be_sub is
> > > something other code may not handle well.
> >
> > Ah ok, so confirming that the change is OK. Thanks.
> > >
> > > > >
> > > > > > @@ -1390,13 +1390,21 @@ ober_read_element(struct ber *ber, struc
> > > > > > return -1;
> > > > > > }
> > > > > > r = ober_read_element(ber, next);
> > > > > > - if (r == -1)
> > > > > > + if (r == -1) {
> > > > > > + /* sub-element overflows sequence/set */
> > > > >
> > > > > This comment is not quite right. I think the proper comment here is:
> > > >
> > > > I'm not sure. Yes the sub-element overflows the buffer, so it is more
> > > > accurate in the literal sense. However, since the sequence/set already
> > > > has its entire length in the buffer it implies that an ECANCELED is an
> > > > overflow of the sub-element of the sequence and therefore makes it
> > > > clearer why the errno is overwritten.
> > >
> > > Actually why do we need to reset the errno anyway?
> > > Is anything in userland looking at ECANCELED and retries after receiving
> > > more data?
> >
> > Yes, aldap.c and snmpe.c do this. And even if it didn't, going with
> > EINVAL is more concise than ECANCELED.
>
> I really dislike such errno magic but fixing that is a much bigger
> project.
>
> > So is the comment OK as is, do you still want to go with your comment,
> > or do you want to suggest something else?
>
> My question is should we actually write this as:
>
> if (r == -1 || r > len) {
> errno = EINVAL;
> return -1;
> }
>
> In the end any error of a subelement can be interpreted as an illegal seq/set.
There are a few ERANGE cases in the ber_read_elements code, which are
valid ber, but too big for our ber.c code. So I don't think that
statement is definitionally true.
I agree that the errno magic isn't pretty, but I don't see a clean way
to implement the fix without the errno magic.
So unless we want a too wide brush I guess my original diff still
stands?
>
> > > > >
> > > > > /* sub-element overflows buffer */
> > > > > > + if (errno == ECANCELED)
> > > > > > + errno = EINVAL;
> > > > > > return -1;
> > > > > > + }
> > > > > > + if (r > len) {
> > > > >
> > >
Re: ber.c: Fix some minor issues in ober_read_element()
On Wed, Nov 02, 2022 at 05:56:21PM +0100, Martijn van Duren wrote:
> On Wed, 2022-11-02 at 17:47 +0100, Claudio Jeker wrote:
> > On Wed, Nov 02, 2022 at 05:25:12PM +0100, Martijn van Duren wrote:
> > > On Wed, 2022-11-02 at 17:00 +0100, Claudio Jeker wrote:
> > > > On Wed, Nov 02, 2022 at 07:33:14AM +0100, Martijn van Duren wrote:
> > > > > I found 2 minor issues in the handling of sequences/sets in
> > > > > ober_read_element():
> > > > > 1) An empty sequence/set (which is basically always) unconditionally
> > > > >creates an (uninitialised) sub-element. Add the same length check
> > > > >used to check the next element
> > > > > 2) For each sub-element r is only checked for -1, but not if it
> > > > >overflows the length of the sequence itself. This is not a big risk
> > > > >since each sub-element is length-checked against the buffer
> > > > >availability and simply returns an ECANCELED, which would be no
> > > > >worse memory-wise than sending an extremely large packet.
> > > > >
> > > > > While here, only having the NULL of the comparison on the next line
> > > > > annoyed me.
> > > > >
> > > > > OK?
> > > >
> > > > See below.
> > > >
> > > > > martijn@
> > > > >
> > > > > Index: ber.c
> > > > > ===
> > > > > RCS file: /cvs/src/lib/libutil/ber.c,v
> > > > > retrieving revision 1.23
> > > > > diff -u -p -r1.23 ber.c
> > > > > --- ber.c 21 Oct 2021 08:17:33 - 1.23
> > > > > +++ ber.c 2 Nov 2022 06:28:33 -
> > > > > @@ -1375,7 +1375,7 @@ ober_read_element(struct ber *ber, struc
> > > > > break;
> > > > > case BER_TYPE_SEQUENCE:
> > > > > case BER_TYPE_SET:
> > > > > - if (elm->be_sub == NULL) {
> > > > > + if (len > 0 && elm->be_sub == NULL) {
> > > > > if ((elm->be_sub = ober_get_element(0)) == NULL)
> > > > > return -1;
> > > > > }
> > > >
> > > > OK, be_sub == NULL is something checked and better than an empty object.
> > >
> > > I'm not sure I understand what you mean here.
> >
> > I just wanted to say that elm->be_sub == NULL is a condition that most
> > other code handles well. While having an empty element in elm->be_sub is
> > something other code may not handle well.
>
> Ah ok, so confirming that the change is OK. Thanks.
> >
> > > >
> > > > > @@ -1390,13 +1390,21 @@ ober_read_element(struct ber *ber, struc
> > > > > return -1;
> > > > > }
> > > > > r = ober_read_element(ber, next);
> > > > > - if (r == -1)
> > > > > + if (r == -1) {
> > > > > + /* sub-element overflows sequence/set */
> > > >
> > > > This comment is not quite right. I think the proper comment here is:
> > >
> > > I'm not sure. Yes the sub-element overflows the buffer, so it is more
> > > accurate in the literal sense. However, since the sequence/set already
> > > has its entire length in the buffer it implies that an ECANCELED is an
> > > overflow of the sub-element of the sequence and therefore makes it
> > > clearer why the errno is overwritten.
> >
> > Actually why do we need to reset the errno anyway?
> > Is anything in userland looking at ECANCELED and retries after receiving
> > more data?
>
> Yes, aldap.c and snmpe.c do this. And even if it didn't, going with
> EINVAL is more concise than ECANCELED.
I really dislike such errno magic but fixing that is a much bigger
project.
> So is the comment OK as is, do you still want to go with your comment,
> or do you want to suggest something else?
My question is should we actually write this as:
if (r == -1 || r > len) {
errno = EINVAL;
return -1;
}
In the end any error of a subelement can be interpreted as an illegal seq/set.
> > > >
> > > > /* sub-element overflows buffer */
> > > > > + if (errno == ECANCELED)
> > > > > + errno = EINVAL;
> > > > > return -1;
> > > > > + }
> > > > > + if (r > len) {
> > > >
> > > > This check here is actually checking that the sub_element does not
> > > > overflow the sequence/set. So maybe move the abcve comment down here.
> > >
> > > I think it does exactly the same thing, with the only difference that
> > > the ECANCELED case didn't reside fully in the buffer. The reason I
> > > didn't add the comment here was because I think it's clear from the
> > > context, where that context might be less obvious at ECANCELED.
> >
> > Sure. It is kind of same same. The check is important though.
>
> Yes, but the question still remains: Do I need to add a comment
Re: ber.c: Fix some minor issues in ober_read_element()
On Wed, 2022-11-02 at 17:47 +0100, Claudio Jeker wrote:
> On Wed, Nov 02, 2022 at 05:25:12PM +0100, Martijn van Duren wrote:
> > On Wed, 2022-11-02 at 17:00 +0100, Claudio Jeker wrote:
> > > On Wed, Nov 02, 2022 at 07:33:14AM +0100, Martijn van Duren wrote:
> > > > I found 2 minor issues in the handling of sequences/sets in
> > > > ober_read_element():
> > > > 1) An empty sequence/set (which is basically always) unconditionally
> > > >creates an (uninitialised) sub-element. Add the same length check
> > > >used to check the next element
> > > > 2) For each sub-element r is only checked for -1, but not if it
> > > >overflows the length of the sequence itself. This is not a big risk
> > > >since each sub-element is length-checked against the buffer
> > > >availability and simply returns an ECANCELED, which would be no
> > > >worse memory-wise than sending an extremely large packet.
> > > >
> > > > While here, only having the NULL of the comparison on the next line
> > > > annoyed me.
> > > >
> > > > OK?
> > >
> > > See below.
> > >
> > > > martijn@
> > > >
> > > > Index: ber.c
> > > > ===
> > > > RCS file: /cvs/src/lib/libutil/ber.c,v
> > > > retrieving revision 1.23
> > > > diff -u -p -r1.23 ber.c
> > > > --- ber.c 21 Oct 2021 08:17:33 - 1.23
> > > > +++ ber.c 2 Nov 2022 06:28:33 -
> > > > @@ -1375,7 +1375,7 @@ ober_read_element(struct ber *ber, struc
> > > > break;
> > > > case BER_TYPE_SEQUENCE:
> > > > case BER_TYPE_SET:
> > > > - if (elm->be_sub == NULL) {
> > > > + if (len > 0 && elm->be_sub == NULL) {
> > > > if ((elm->be_sub = ober_get_element(0)) == NULL)
> > > > return -1;
> > > > }
> > >
> > > OK, be_sub == NULL is something checked and better than an empty object.
> >
> > I'm not sure I understand what you mean here.
>
> I just wanted to say that elm->be_sub == NULL is a condition that most
> other code handles well. While having an empty element in elm->be_sub is
> something other code may not handle well.
Ah ok, so confirming that the change is OK. Thanks.
>
> > >
> > > > @@ -1390,13 +1390,21 @@ ober_read_element(struct ber *ber, struc
> > > > return -1;
> > > > }
> > > > r = ober_read_element(ber, next);
> > > > - if (r == -1)
> > > > + if (r == -1) {
> > > > + /* sub-element overflows sequence/set */
> > >
> > > This comment is not quite right. I think the proper comment here is:
> >
> > I'm not sure. Yes the sub-element overflows the buffer, so it is more
> > accurate in the literal sense. However, since the sequence/set already
> > has its entire length in the buffer it implies that an ECANCELED is an
> > overflow of the sub-element of the sequence and therefore makes it
> > clearer why the errno is overwritten.
>
> Actually why do we need to reset the errno anyway?
> Is anything in userland looking at ECANCELED and retries after receiving
> more data?
Yes, aldap.c and snmpe.c do this. And even if it didn't, going with
EINVAL is more concise than ECANCELED.
So is the comment OK as is, do you still want to go with your comment,
or do you want to suggest something else?
>
> > >
> > > /* sub-element overflows buffer */
> > > > + if (errno == ECANCELED)
> > > > + errno = EINVAL;
> > > > return -1;
> > > > + }
> > > > + if (r > len) {
> > >
> > > This check here is actually checking that the sub_element does not
> > > overflow the sequence/set. So maybe move the abcve comment down here.
> >
> > I think it does exactly the same thing, with the only difference that
> > the ECANCELED case didn't reside fully in the buffer. The reason I
> > didn't add the comment here was because I think it's clear from the
> > context, where that context might be less obvious at ECANCELED.
>
> Sure. It is kind of same same. The check is important though.
Yes, but the question still remains: Do I need to add a comment here?
>
> > >
> > > > + errno = EINVAL;
> > > > + return -1;
> > > > + }
> > > > elements++;
> > > > len -= r;
> > > > if (len > 0 && next->be_next == NULL) {
> > > > - if ((next->be_next =
> > > > ober_get_element(0)) ==
> > > > - NULL)
> > > > + next->be_next = ober_get_element(0);
> > > > + if (next->be_next == NULL)
> > > >
Re: ber.c: Fix some minor issues in ober_read_element()
On Wed, Nov 02, 2022 at 05:25:12PM +0100, Martijn van Duren wrote:
> On Wed, 2022-11-02 at 17:00 +0100, Claudio Jeker wrote:
> > On Wed, Nov 02, 2022 at 07:33:14AM +0100, Martijn van Duren wrote:
> > > I found 2 minor issues in the handling of sequences/sets in
> > > ober_read_element():
> > > 1) An empty sequence/set (which is basically always) unconditionally
> > >creates an (uninitialised) sub-element. Add the same length check
> > >used to check the next element
> > > 2) For each sub-element r is only checked for -1, but not if it
> > >overflows the length of the sequence itself. This is not a big risk
> > >since each sub-element is length-checked against the buffer
> > >availability and simply returns an ECANCELED, which would be no
> > >worse memory-wise than sending an extremely large packet.
> > >
> > > While here, only having the NULL of the comparison on the next line
> > > annoyed me.
> > >
> > > OK?
> >
> > See below.
> >
> > > martijn@
> > >
> > > Index: ber.c
> > > ===
> > > RCS file: /cvs/src/lib/libutil/ber.c,v
> > > retrieving revision 1.23
> > > diff -u -p -r1.23 ber.c
> > > --- ber.c 21 Oct 2021 08:17:33 - 1.23
> > > +++ ber.c 2 Nov 2022 06:28:33 -
> > > @@ -1375,7 +1375,7 @@ ober_read_element(struct ber *ber, struc
> > > break;
> > > case BER_TYPE_SEQUENCE:
> > > case BER_TYPE_SET:
> > > - if (elm->be_sub == NULL) {
> > > + if (len > 0 && elm->be_sub == NULL) {
> > > if ((elm->be_sub = ober_get_element(0)) == NULL)
> > > return -1;
> > > }
> >
> > OK, be_sub == NULL is something checked and better than an empty object.
>
> I'm not sure I understand what you mean here.
I just wanted to say that elm->be_sub == NULL is a condition that most
other code handles well. While having an empty element in elm->be_sub is
something other code may not handle well.
> >
> > > @@ -1390,13 +1390,21 @@ ober_read_element(struct ber *ber, struc
> > > return -1;
> > > }
> > > r = ober_read_element(ber, next);
> > > - if (r == -1)
> > > + if (r == -1) {
> > > + /* sub-element overflows sequence/set */
> >
> > This comment is not quite right. I think the proper comment here is:
>
> I'm not sure. Yes the sub-element overflows the buffer, so it is more
> accurate in the literal sense. However, since the sequence/set already
> has its entire length in the buffer it implies that an ECANCELED is an
> overflow of the sub-element of the sequence and therefore makes it
> clearer why the errno is overwritten.
Actually why do we need to reset the errno anyway?
Is anything in userland looking at ECANCELED and retries after receiving
more data?
> >
> > /* sub-element overflows buffer */
> > > + if (errno == ECANCELED)
> > > + errno = EINVAL;
> > > return -1;
> > > + }
> > > + if (r > len) {
> >
> > This check here is actually checking that the sub_element does not
> > overflow the sequence/set. So maybe move the abcve comment down here.
>
> I think it does exactly the same thing, with the only difference that
> the ECANCELED case didn't reside fully in the buffer. The reason I
> didn't add the comment here was because I think it's clear from the
> context, where that context might be less obvious at ECANCELED.
Sure. It is kind of same same. The check is important though.
> >
> > > + errno = EINVAL;
> > > + return -1;
> > > + }
> > > elements++;
> > > len -= r;
> > > if (len > 0 && next->be_next == NULL) {
> > > - if ((next->be_next = ober_get_element(0)) ==
> > > - NULL)
> > > + next->be_next = ober_get_element(0);
> > > + if (next->be_next == NULL)
> > > return -1;
> > > }
> > > next = next->be_next;
> > >
> >
> > Apart from that OK claudio@
> >
>
--
:wq Claudio
Re: rpki-client unify http logging a bit
On Wed, Nov 02, 2022 at 04:38:00PM +0100, Claudio Jeker wrote: > Based on Job's work lets introduce conn_info() which prints the URI / > host plus the IP address. This may be helpful to better understand errors. > > With this ip_info() becomes much simpler. I also decided to not check > snprintf returns because the buffer is big enough and afaik encoding > errors can't happen with %s. Yes, it should just copy the bytes over. > In http_connect() when hitting the error case after the for () loop needs > a to set conn->res to the last valid res used else it will print (unknown) > for the IP (since conn->res is NULL at that time). All makes sense. I like it. ok tb This could go on a single line: > @@ -818,7 +825,7 @@ http_do(struct http_connection *conn, en > break; > default: > errx(1, "%s: unexpected function return", > - http_info(conn->host)); > + conn_info(conn)); > } > } >
Re: ber.c: Fix some minor issues in ober_read_element()
On Wed, 2022-11-02 at 17:00 +0100, Claudio Jeker wrote:
> On Wed, Nov 02, 2022 at 07:33:14AM +0100, Martijn van Duren wrote:
> > I found 2 minor issues in the handling of sequences/sets in
> > ober_read_element():
> > 1) An empty sequence/set (which is basically always) unconditionally
> >creates an (uninitialised) sub-element. Add the same length check
> >used to check the next element
> > 2) For each sub-element r is only checked for -1, but not if it
> >overflows the length of the sequence itself. This is not a big risk
> >since each sub-element is length-checked against the buffer
> >availability and simply returns an ECANCELED, which would be no
> >worse memory-wise than sending an extremely large packet.
> >
> > While here, only having the NULL of the comparison on the next line
> > annoyed me.
> >
> > OK?
>
> See below.
>
> > martijn@
> >
> > Index: ber.c
> > ===
> > RCS file: /cvs/src/lib/libutil/ber.c,v
> > retrieving revision 1.23
> > diff -u -p -r1.23 ber.c
> > --- ber.c 21 Oct 2021 08:17:33 - 1.23
> > +++ ber.c 2 Nov 2022 06:28:33 -
> > @@ -1375,7 +1375,7 @@ ober_read_element(struct ber *ber, struc
> > break;
> > case BER_TYPE_SEQUENCE:
> > case BER_TYPE_SET:
> > - if (elm->be_sub == NULL) {
> > + if (len > 0 && elm->be_sub == NULL) {
> > if ((elm->be_sub = ober_get_element(0)) == NULL)
> > return -1;
> > }
>
> OK, be_sub == NULL is something checked and better than an empty object.
I'm not sure I understand what you mean here.
>
> > @@ -1390,13 +1390,21 @@ ober_read_element(struct ber *ber, struc
> > return -1;
> > }
> > r = ober_read_element(ber, next);
> > - if (r == -1)
> > + if (r == -1) {
> > + /* sub-element overflows sequence/set */
>
> This comment is not quite right. I think the proper comment here is:
I'm not sure. Yes the sub-element overflows the buffer, so it is more
accurate in the literal sense. However, since the sequence/set already
has its entire length in the buffer it implies that an ECANCELED is an
overflow of the sub-element of the sequence and therefore makes it
clearer why the errno is overwritten.
>
> /* sub-element overflows buffer */
> > + if (errno == ECANCELED)
> > + errno = EINVAL;
> > return -1;
> > + }
> > + if (r > len) {
>
> This check here is actually checking that the sub_element does not
> overflow the sequence/set. So maybe move the abcve comment down here.
I think it does exactly the same thing, with the only difference that
the ECANCELED case didn't reside fully in the buffer. The reason I
didn't add the comment here was because I think it's clear from the
context, where that context might be less obvious at ECANCELED.
>
> > + errno = EINVAL;
> > + return -1;
> > + }
> > elements++;
> > len -= r;
> > if (len > 0 && next->be_next == NULL) {
> > - if ((next->be_next = ober_get_element(0)) ==
> > - NULL)
> > + next->be_next = ober_get_element(0);
> > + if (next->be_next == NULL)
> > return -1;
> > }
> > next = next->be_next;
> >
>
> Apart from that OK claudio@
>
Re: ber.c: Fix some minor issues in ober_read_element()
On Wed, Nov 02, 2022 at 07:33:14AM +0100, Martijn van Duren wrote:
> I found 2 minor issues in the handling of sequences/sets in
> ober_read_element():
> 1) An empty sequence/set (which is basically always) unconditionally
>creates an (uninitialised) sub-element. Add the same length check
>used to check the next element
> 2) For each sub-element r is only checked for -1, but not if it
>overflows the length of the sequence itself. This is not a big risk
>since each sub-element is length-checked against the buffer
>availability and simply returns an ECANCELED, which would be no
>worse memory-wise than sending an extremely large packet.
>
> While here, only having the NULL of the comparison on the next line
> annoyed me.
>
> OK?
See below.
> martijn@
>
> Index: ber.c
> ===
> RCS file: /cvs/src/lib/libutil/ber.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 ber.c
> --- ber.c 21 Oct 2021 08:17:33 - 1.23
> +++ ber.c 2 Nov 2022 06:28:33 -
> @@ -1375,7 +1375,7 @@ ober_read_element(struct ber *ber, struc
> break;
> case BER_TYPE_SEQUENCE:
> case BER_TYPE_SET:
> - if (elm->be_sub == NULL) {
> + if (len > 0 && elm->be_sub == NULL) {
> if ((elm->be_sub = ober_get_element(0)) == NULL)
> return -1;
> }
OK, be_sub == NULL is something checked and better than an empty object.
> @@ -1390,13 +1390,21 @@ ober_read_element(struct ber *ber, struc
> return -1;
> }
> r = ober_read_element(ber, next);
> - if (r == -1)
> + if (r == -1) {
> + /* sub-element overflows sequence/set */
This comment is not quite right. I think the proper comment here is:
/* sub-element overflows buffer */
> + if (errno == ECANCELED)
> + errno = EINVAL;
> return -1;
> + }
> + if (r > len) {
This check here is actually checking that the sub_element does not
overflow the sequence/set. So maybe move the abcve comment down here.
> + errno = EINVAL;
> + return -1;
> + }
> elements++;
> len -= r;
> if (len > 0 && next->be_next == NULL) {
> - if ((next->be_next = ober_get_element(0)) ==
> - NULL)
> + next->be_next = ober_get_element(0);
> + if (next->be_next == NULL)
> return -1;
> }
> next = next->be_next;
>
Apart from that OK claudio@
--
:wq Claudio
rpki-client unify http logging a bit
Based on Job's work lets introduce conn_info() which prints the URI /
host plus the IP address. This may be helpful to better understand errors.
With this ip_info() becomes much simpler. I also decided to not check
snprintf returns because the buffer is big enough and afaik encoding
errors can't happen with %s.
In http_connect() when hitting the error case after the for () loop needs
a to set conn->res to the last valid res used else it will print (unknown)
for the IP (since conn->res is NULL at that time).
With this the errors will show up like this:
rpki-client: Error retrieving https://rpki.luys.cloud/rrdp/notification.xml
(54.254.122.29): 504 Gateway Time-out
rpki-client: https://rpki.sailx.co/rrdp/notification.xml (208.82.103.214): TLS
handshake: certificate verification failed: certificate has expired
rpki-client: https://chloe.sobornost.net/rpki/news-public.xml (127.0.0.2):
connect: Network is unreachable
rpki-client: https://rpki1.terratransit.de/rrdp/notification.xml
(2a01:6f0:101:17::1): connect timeout
rpki-client: https://rpki1.terratransit.de/rrdp/notification.xml
(77.237.224.23): connect timeout
--
:wq Claudio
Index: http.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
retrieving revision 1.72
diff -u -p -r1.72 http.c
--- http.c 2 Nov 2022 11:44:19 - 1.72
+++ http.c 2 Nov 2022 14:20:31 -
@@ -215,23 +215,30 @@ http_info(const char *uri)
static const char *
ip_info(const struct http_connection *conn)
{
- static char buf[NI_MAXHOST + 3];
- charipbuf[NI_MAXHOST];
- int ret;
-
- assert(conn->state == STATE_CONNECT);
+ static char ipbuf[NI_MAXHOST];
if (conn->res == NULL)
- return (" (unknown)");
+ return "unknown";
if (getnameinfo(conn->res->ai_addr, conn->res->ai_addrlen, ipbuf,
sizeof(ipbuf), NULL, 0, NI_NUMERICHOST) != 0)
- return (" (unknown)");
+ return "unknown";
- ret = snprintf(buf, sizeof(buf), " (%s)", ipbuf);
- if (ret < 0 || (size_t)ret >= sizeof(buf))
- err(1, NULL);
+ return ipbuf;
+}
+static const char *
+conn_info(const struct http_connection *conn)
+{
+ static char buf[100 + NI_MAXHOST];
+ const char *uri;
+
+ if (conn->req == NULL)
+ uri = conn->host;
+ else
+ uri = conn->req->uri;
+
+ snprintf(buf, sizeof(buf), "%s (%s)", http_info(uri), ip_info(conn));
return buf;
}
@@ -818,7 +825,7 @@ http_do(struct http_connection *conn, en
break;
default:
errx(1, "%s: unexpected function return",
- http_info(conn->host));
+ conn_info(conn));
}
}
@@ -840,6 +847,7 @@ static enum res
http_connect(struct http_connection *conn)
{
const char *cause = NULL;
+ struct addrinfo *res;
assert(conn->fd == -1);
conn->state = STATE_CONNECT;
@@ -850,9 +858,9 @@ http_connect(struct http_connection *con
else
conn->res = conn->res->ai_next;
for (; conn->res != NULL; conn->res = conn->res->ai_next) {
- struct addrinfo *res = conn->res;
int fd, save_errno;
+ res = conn->res;
fd = socket(res->ai_family,
res->ai_socktype | SOCK_NONBLOCK, res->ai_protocol);
if (fd == -1) {
@@ -891,8 +899,10 @@ http_connect(struct http_connection *con
}
if (conn->fd == -1) {
- if (cause != NULL)
- warn("%s: %s", http_info(conn->req->uri), cause);
+ if (cause != NULL) {
+ conn->res = res;
+ warn("%s: %s", conn_info(conn), cause);
+ }
return http_failed(conn);
}
@@ -910,12 +920,12 @@ http_finish_connect(struct http_connecti
len = sizeof(error);
if (getsockopt(conn->fd, SOL_SOCKET, SO_ERROR, , ) == -1) {
- warn("%s: getsockopt SO_ERROR", http_info(conn->req->uri));
+ warn("%s: getsockopt SO_ERROR", conn_info(conn));
return http_connect_failed(conn);
}
if (error != 0) {
errno = error;
- warn("%s: connect", http_info(conn->req->uri));
+ warn("%s: connect", conn_info(conn));
return http_connect_failed(conn);
}
@@ -936,12 +946,12 @@ http_tls_connect(struct http_connection
return http_failed(conn);
}
if (tls_configure(conn->tls, tls_config) == -1) {
- warnx("%s: TLS configuration: %s\n", http_info(conn->req->uri),
+ warnx("%s: TLS configuration: %s\n", conn_info(conn),
tls_error(conn->tls));
return http_failed(conn);
}
if
Fix kernel build without IPSEC option
Hi,
if you build the kernel without IPSEC it will run into several compiler
and linker errors. This diff add some missing #ifdefs to fix this.
ok?
bye,
jan
Index: net/if_pfsync.c
===
RCS file: /mount/openbsd/cvs/src/sys/net/if_pfsync.c,v
retrieving revision 1.305
diff -u -p -r1.305 if_pfsync.c
--- net/if_pfsync.c 21 Apr 2022 15:22:49 - 1.305
+++ net/if_pfsync.c 2 Nov 2022 10:20:38 -
@@ -1576,7 +1576,9 @@ pfsync_grab_snapshot(struct pfsync_snaps
int q;
struct pf_state *st;
struct pfsync_upd_req_item *ur;
+#if defined(IPSEC)
struct tdb *tdb;
+#endif
sn->sn_sc = sc;
@@ -1602,6 +1604,7 @@ pfsync_grab_snapshot(struct pfsync_snaps
}
TAILQ_INIT(>sn_tdb_q);
+#if defined(IPSEC)
while ((tdb = TAILQ_FIRST(>sc_tdb_q)) != NULL) {
TAILQ_REMOVE(>sc_tdb_q, tdb, tdb_sync_entry);
TAILQ_INSERT_TAIL(>sn_tdb_q, tdb, tdb_sync_snap);
@@ -1611,6 +1614,7 @@ pfsync_grab_snapshot(struct pfsync_snaps
SET(tdb->tdb_flags, TDBF_PFSYNC_SNAPPED);
mtx_leave(>tdb_mtx);
}
+#endif
sn->sn_len = sc->sc_len;
sc->sc_len = PFSYNC_MINPKT;
@@ -1630,7 +1634,9 @@ pfsync_drop_snapshot(struct pfsync_snaps
{
struct pf_state *st;
struct pfsync_upd_req_item *ur;
+#if defined(IPSEC)
struct tdb *t;
+#endif
int q;
for (q = 0; q < PFSYNC_S_COUNT; q++) {
@@ -1652,6 +1658,7 @@ pfsync_drop_snapshot(struct pfsync_snaps
pool_put(>sn_sc->sc_pool, ur);
}
+#if defined(IPSEC)
while ((t = TAILQ_FIRST(>sn_tdb_q)) != NULL) {
TAILQ_REMOVE(>sn_tdb_q, t, tdb_sync_snap);
mtx_enter(>tdb_mtx);
@@ -1660,6 +1667,7 @@ pfsync_drop_snapshot(struct pfsync_snaps
CLR(t->tdb_flags, TDBF_PFSYNC);
mtx_leave(>tdb_mtx);
}
+#endif
}
int
@@ -1748,7 +1756,6 @@ pfsync_sendout(void)
struct pfsync_subheader *subh;
struct pf_state *st;
struct pfsync_upd_req_item *ur;
- struct tdb *t;
int offset;
int q, count = 0;
@@ -1842,7 +1849,10 @@ pfsync_sendout(void)
sn.sn_plus = NULL; /* XXX memory leak ? */
}
+#if defined(IPSEC)
if (!TAILQ_EMPTY(_tdb_q)) {
+ struct tdb *t;
+
subh = (struct pfsync_subheader *)(m->m_data + offset);
offset += sizeof(*subh);
@@ -1865,6 +1875,7 @@ pfsync_sendout(void)
subh->len = sizeof(struct pfsync_tdb) >> 2;
subh->count = htons(count);
}
+#endif
/* walk the queues */
for (q = 0; q < PFSYNC_S_COUNT; q++) {
@@ -2486,6 +2497,7 @@ pfsync_q_del(struct pf_state *st)
pf_state_unref(st);
}
+#if defined(IPSEC)
void
pfsync_update_tdb(struct tdb *t, int output)
{
@@ -2540,7 +2552,9 @@ pfsync_update_tdb(struct tdb *t, int out
CLR(t->tdb_flags, TDBF_PFSYNC_RPL);
mtx_leave(>tdb_mtx);
}
+#endif
+#if defined(IPSEC)
void
pfsync_delete_tdb(struct tdb *t)
{
@@ -2576,6 +2590,7 @@ pfsync_delete_tdb(struct tdb *t)
tdb_unref(t);
}
+#endif
void
pfsync_out_tdb(struct tdb *t, void *buf)
Index: netinet/ip_ipsp.c
===
RCS file: /mount/openbsd/cvs/src/sys/netinet/ip_ipsp.c,v
retrieving revision 1.273
diff -u -p -r1.273 ip_ipsp.c
--- netinet/ip_ipsp.c 6 Aug 2022 15:57:59 - 1.273
+++ netinet/ip_ipsp.c 2 Nov 2022 12:09:22 -
@@ -1081,7 +1081,7 @@ tdb_free(struct tdb *tdbp)
tdbp->tdb_xform = NULL;
}
-#if NPFSYNC > 0
+#if NPFSYNC > 0 && defined(IPSEC)
/* Cleanup pfsync references */
pfsync_delete_tdb(tdbp);
#endif
Re: rpki-client keep http connection info around
On Wed, Nov 02, 2022 at 11:55:12AM +0100, Claudio Jeker wrote:
> Job's diff made me realise that clearing the connection info (conn->res)
> makes error reporting worse. It is not like we save lots of memory by
> doing so. So do not call freeaddrinfo() in http_connect_done(), now
> http_free() will free res0 before freeing conn.
Makes sense. We're always in STATE_CONNECT, so it's ok to call
http_connect_failed().
ok tb
>
> --
> :wq Claudio
>
> Index: http.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 http.c
> --- http.c18 Oct 2022 14:03:39 - 1.70
> +++ http.c2 Nov 2022 10:35:28 -
> @@ -802,10 +802,6 @@ http_do(struct http_connection *conn, en
> static enum res
> http_connect_done(struct http_connection *conn)
> {
> - freeaddrinfo(conn->res0);
> - conn->res0 = NULL;
> - conn->res = NULL;
> -
> if (proxy.proxyhost != NULL)
> return proxy_connect(conn);
> return http_tls_connect(conn);
> @@ -889,21 +885,15 @@ http_finish_connect(struct http_connecti
> len = sizeof(error);
> if (getsockopt(conn->fd, SOL_SOCKET, SO_ERROR, , ) == -1) {
> warn("%s: getsockopt SO_ERROR", http_info(conn->req->uri));
> - goto fail;
> + return http_connect_failed(conn);
> }
> if (error != 0) {
> errno = error;
> warn("%s: connect", http_info(conn->req->uri));
> - goto fail;
> + return http_connect_failed(conn);
> }
>
> return http_connect_done(conn);
> -
> -fail:
> - close(conn->fd);
> - conn->fd = -1;
> -
> - return http_connect(conn);
> }
>
> /*
>
rpki-client keep http connection info around
Job's diff made me realise that clearing the connection info (conn->res)
makes error reporting worse. It is not like we save lots of memory by
doing so. So do not call freeaddrinfo() in http_connect_done(), now
http_free() will free res0 before freeing conn.
--
:wq Claudio
Index: http.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
retrieving revision 1.70
diff -u -p -r1.70 http.c
--- http.c 18 Oct 2022 14:03:39 - 1.70
+++ http.c 2 Nov 2022 10:35:28 -
@@ -802,10 +802,6 @@ http_do(struct http_connection *conn, en
static enum res
http_connect_done(struct http_connection *conn)
{
- freeaddrinfo(conn->res0);
- conn->res0 = NULL;
- conn->res = NULL;
-
if (proxy.proxyhost != NULL)
return proxy_connect(conn);
return http_tls_connect(conn);
@@ -889,21 +885,15 @@ http_finish_connect(struct http_connecti
len = sizeof(error);
if (getsockopt(conn->fd, SOL_SOCKET, SO_ERROR, , ) == -1) {
warn("%s: getsockopt SO_ERROR", http_info(conn->req->uri));
- goto fail;
+ return http_connect_failed(conn);
}
if (error != 0) {
errno = error;
warn("%s: connect", http_info(conn->req->uri));
- goto fail;
+ return http_connect_failed(conn);
}
return http_connect_done(conn);
-
-fail:
- close(conn->fd);
- conn->fd = -1;
-
- return http_connect(conn);
}
/*
Re: rpki-client: missing length check in valid_uri()
On Wed, Nov 02, 2022 at 11:45:43AM +0100, Theo Buehler wrote:
> Not all callers of valid_uri() ensure that the uri passed in is actually
> a C string and the API implies at least that uri[usz - 1] != '\0' is
> allowed. For example, x509_location() a priori doesn't pass a C string
> and Job will soon add a second instance. I think we should explicitly
> length check uri to be long enough before calling strncasecmp() on it.
>
> The diff below is mostly cosmetic as it happens to be the case that
> libcrypto NUL-terminates ASN.1 strings for historical reasons, but this
> is an implementation detail and design mistake (general ASN.1 strings
> can have embedded NULs) that we shouldn't rely on.
>
> The diff does change behavior in that a naked "https://; will no longer
> be considered a valid uri (which I'm pretty sure it isn't, but I may
> have missed a corner case in the spec).
>
> Index: validate.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/validate.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 validate.c
> --- validate.c3 Sep 2022 14:41:47 - 1.45
> +++ validate.c2 Nov 2022 10:20:40 -
> @@ -290,6 +290,8 @@ valid_uri(const char *uri, size_t usz, c
>
> if (proto != NULL) {
> s = strlen(proto);
> + if (s >= usz)
> + return 0;
> if (strncasecmp(uri, proto, s) != 0)
> return 0;
> }
>
Makes sense. OK claudio@
Also "https://; should be considered invalid since there is no host
specified.
--
:wq Claudio
rpki-client: missing length check in valid_uri()
Not all callers of valid_uri() ensure that the uri passed in is actually
a C string and the API implies at least that uri[usz - 1] != '\0' is
allowed. For example, x509_location() a priori doesn't pass a C string
and Job will soon add a second instance. I think we should explicitly
length check uri to be long enough before calling strncasecmp() on it.
The diff below is mostly cosmetic as it happens to be the case that
libcrypto NUL-terminates ASN.1 strings for historical reasons, but this
is an implementation detail and design mistake (general ASN.1 strings
can have embedded NULs) that we shouldn't rely on.
The diff does change behavior in that a naked "https://; will no longer
be considered a valid uri (which I'm pretty sure it isn't, but I may
have missed a corner case in the spec).
Index: validate.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/validate.c,v
retrieving revision 1.45
diff -u -p -r1.45 validate.c
--- validate.c 3 Sep 2022 14:41:47 - 1.45
+++ validate.c 2 Nov 2022 10:20:40 -
@@ -290,6 +290,8 @@ valid_uri(const char *uri, size_t usz, c
if (proto != NULL) {
s = strlen(proto);
+ if (s >= usz)
+ return 0;
if (strncasecmp(uri, proto, s) != 0)
return 0;
}
Re: Remove audio(9) speaker_ctl(), let open() handle speakers where needed
On Sun, Oct 30, 2022 at 11:02:39AM +, Klemens Nanni wrote: > Only five legacy half-duplex hardware drivers require this function to > change between playing and recording: > i386: ess(4), gus(4), pas(4), sb(4) > luna88k: nec86(4) > > If defined, it is always called early in audio_open(), so just move the > call from audio(4) to each hardware driver's open() handler. > > SPKR_ON/OFF remain defined to leave driver-specific code unchanged. > > Further cleanup (unchecked speaker_ctl() return values, > FWRITE -> AUMODE_PLAY -> SPKR_ON dances, etc.) can happen later. > > i386/GENERIC.MP builds fine. > Feedback? Objection? OK? ok ratchov
Re: rpki-client: fix x509_get_time() error checks
On Wed, Nov 02, 2022 at 10:38:57AM +0100, Theo Buehler wrote:
> Like most x509_* functions, x509_get_time() returns 0 on error and 1 on
> success, so rather than changing x509_get_time(), I changed the callers.
OK claudio@
> Index: aspa.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/aspa.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 aspa.c
> --- aspa.c13 Oct 2022 04:43:32 - 1.5
> +++ aspa.c2 Nov 2022 09:32:55 -
> @@ -225,7 +225,7 @@ aspa_parse(X509 **x509, const char *fn,
> warnx("%s: X509_get0_notAfter failed", fn);
> goto out;
> }
> - if (x509_get_time(at, >expires) == -1) {
> + if (!x509_get_time(at, >expires)) {
> warnx("%s: ASN1_time_parse failed", fn);
> goto out;
> }
> Index: crl.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 crl.c
> --- crl.c 3 Sep 2022 21:24:02 - 1.16
> +++ crl.c 2 Nov 2022 09:34:39 -
> @@ -57,7 +57,7 @@ crl_parse(const char *fn, const unsigned
> warnx("%s: X509_CRL_get0_lastUpdate failed", fn);
> goto out;
> }
> - if (x509_get_time(at, >issued) == -1) {
> + if (!x509_get_time(at, >issued)) {
> warnx("%s: ASN1_time_parse failed", fn);
> goto out;
> }
> @@ -67,7 +67,7 @@ crl_parse(const char *fn, const unsigned
> warnx("%s: X509_CRL_get0_nextUpdate failed", fn);
> goto out;
> }
> - if (x509_get_time(at, >expires) == -1) {
> + if (!x509_get_time(at, >expires)) {
> warnx("%s: ASN1_time_parse failed", fn);
> goto out;
> }
> Index: roa.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
> retrieving revision 1.53
> diff -u -p -r1.53 roa.c
> --- roa.c 13 Oct 2022 04:43:32 - 1.53
> +++ roa.c 2 Nov 2022 09:33:51 -
> @@ -235,7 +235,7 @@ roa_parse(X509 **x509, const char *fn, c
> warnx("%s: X509_get0_notAfter failed", fn);
> goto out;
> }
> - if (x509_get_time(at, >expires) == -1) {
> + if (!x509_get_time(at, >expires)) {
> warnx("%s: ASN1_time_parse failed", fn);
> goto out;
> }
> Index: rsc.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/rsc.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 rsc.c
> --- rsc.c 13 Oct 2022 04:43:32 - 1.16
> +++ rsc.c 2 Nov 2022 09:34:05 -
> @@ -408,7 +408,7 @@ rsc_parse(X509 **x509, const char *fn, c
> warnx("%s: X509_get0_notAfter failed", fn);
> goto out;
> }
> - if (x509_get_time(at, >expires) == -1) {
> + if (!x509_get_time(at, >expires)) {
> warnx("%s: ASN1_time_parse failed", fn);
> goto out;
> }
> Index: x509.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
> retrieving revision 1.51
> diff -u -p -r1.51 x509.c
> --- x509.c24 Oct 2022 10:26:59 - 1.51
> +++ x509.c2 Nov 2022 09:34:14 -
> @@ -377,7 +377,7 @@ x509_get_expire(X509 *x, const char *fn,
> warnx("%s: X509_get0_notafter failed", fn);
> return 0;
> }
> - if (x509_get_time(at, tt) == -1) {
> + if (!x509_get_time(at, tt)) {
> warnx("%s: ASN1_time_parse failed", fn);
> return 0;
> }
>
--
:wq Claudio
rpki-client: fix x509_get_time() error checks
Like most x509_* functions, x509_get_time() returns 0 on error and 1 on
success, so rather than changing x509_get_time(), I changed the callers.
Index: aspa.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/aspa.c,v
retrieving revision 1.5
diff -u -p -r1.5 aspa.c
--- aspa.c 13 Oct 2022 04:43:32 - 1.5
+++ aspa.c 2 Nov 2022 09:32:55 -
@@ -225,7 +225,7 @@ aspa_parse(X509 **x509, const char *fn,
warnx("%s: X509_get0_notAfter failed", fn);
goto out;
}
- if (x509_get_time(at, >expires) == -1) {
+ if (!x509_get_time(at, >expires)) {
warnx("%s: ASN1_time_parse failed", fn);
goto out;
}
Index: crl.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v
retrieving revision 1.16
diff -u -p -r1.16 crl.c
--- crl.c 3 Sep 2022 21:24:02 - 1.16
+++ crl.c 2 Nov 2022 09:34:39 -
@@ -57,7 +57,7 @@ crl_parse(const char *fn, const unsigned
warnx("%s: X509_CRL_get0_lastUpdate failed", fn);
goto out;
}
- if (x509_get_time(at, >issued) == -1) {
+ if (!x509_get_time(at, >issued)) {
warnx("%s: ASN1_time_parse failed", fn);
goto out;
}
@@ -67,7 +67,7 @@ crl_parse(const char *fn, const unsigned
warnx("%s: X509_CRL_get0_nextUpdate failed", fn);
goto out;
}
- if (x509_get_time(at, >expires) == -1) {
+ if (!x509_get_time(at, >expires)) {
warnx("%s: ASN1_time_parse failed", fn);
goto out;
}
Index: roa.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
retrieving revision 1.53
diff -u -p -r1.53 roa.c
--- roa.c 13 Oct 2022 04:43:32 - 1.53
+++ roa.c 2 Nov 2022 09:33:51 -
@@ -235,7 +235,7 @@ roa_parse(X509 **x509, const char *fn, c
warnx("%s: X509_get0_notAfter failed", fn);
goto out;
}
- if (x509_get_time(at, >expires) == -1) {
+ if (!x509_get_time(at, >expires)) {
warnx("%s: ASN1_time_parse failed", fn);
goto out;
}
Index: rsc.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/rsc.c,v
retrieving revision 1.16
diff -u -p -r1.16 rsc.c
--- rsc.c 13 Oct 2022 04:43:32 - 1.16
+++ rsc.c 2 Nov 2022 09:34:05 -
@@ -408,7 +408,7 @@ rsc_parse(X509 **x509, const char *fn, c
warnx("%s: X509_get0_notAfter failed", fn);
goto out;
}
- if (x509_get_time(at, >expires) == -1) {
+ if (!x509_get_time(at, >expires)) {
warnx("%s: ASN1_time_parse failed", fn);
goto out;
}
Index: x509.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
retrieving revision 1.51
diff -u -p -r1.51 x509.c
--- x509.c 24 Oct 2022 10:26:59 - 1.51
+++ x509.c 2 Nov 2022 09:34:14 -
@@ -377,7 +377,7 @@ x509_get_expire(X509 *x, const char *fn,
warnx("%s: X509_get0_notafter failed", fn);
return 0;
}
- if (x509_get_time(at, tt) == -1) {
+ if (!x509_get_time(at, tt)) {
warnx("%s: ASN1_time_parse failed", fn);
return 0;
}
ber.c: Fix some minor issues in ober_read_element()
I found 2 minor issues in the handling of sequences/sets in
ober_read_element():
1) An empty sequence/set (which is basically always) unconditionally
creates an (uninitialised) sub-element. Add the same length check
used to check the next element
2) For each sub-element r is only checked for -1, but not if it
overflows the length of the sequence itself. This is not a big risk
since each sub-element is length-checked against the buffer
availability and simply returns an ECANCELED, which would be no
worse memory-wise than sending an extremely large packet.
While here, only having the NULL of the comparison on the next line
annoyed me.
OK?
martijn@
Index: ber.c
===
RCS file: /cvs/src/lib/libutil/ber.c,v
retrieving revision 1.23
diff -u -p -r1.23 ber.c
--- ber.c 21 Oct 2021 08:17:33 - 1.23
+++ ber.c 2 Nov 2022 06:28:33 -
@@ -1375,7 +1375,7 @@ ober_read_element(struct ber *ber, struc
break;
case BER_TYPE_SEQUENCE:
case BER_TYPE_SET:
- if (elm->be_sub == NULL) {
+ if (len > 0 && elm->be_sub == NULL) {
if ((elm->be_sub = ober_get_element(0)) == NULL)
return -1;
}
@@ -1390,13 +1390,21 @@ ober_read_element(struct ber *ber, struc
return -1;
}
r = ober_read_element(ber, next);
- if (r == -1)
+ if (r == -1) {
+ /* sub-element overflows sequence/set */
+ if (errno == ECANCELED)
+ errno = EINVAL;
return -1;
+ }
+ if (r > len) {
+ errno = EINVAL;
+ return -1;
+ }
elements++;
len -= r;
if (len > 0 && next->be_next == NULL) {
- if ((next->be_next = ober_get_element(0)) ==
- NULL)
+ next->be_next = ober_get_element(0);
+ if (next->be_next == NULL)
return -1;
}
next = next->be_next;
