Re: sync libfido2 with upstream
On Fri, Aug 21, 2020 at 11:42:53AM +1000, Damien Miller wrote: > On Mon, 17 Aug 2020, Damien Miller wrote: > > > On Mon, 10 Aug 2020, Damien Miller wrote: > > > > > Hi, > > > > > > This syncs libfido2 with the current state of upstream. It includes > > > a few new APIs that I want to use in OpenSSH to improve FIDO token > > > support (require-PIN and fixing some corner-case bugs around multiple > > > inserted tokens). > > > > > > ok? > > > > > > (major crank for ABI change) > > > > So I pounced on the new API a bit too soon and before it stabilised. > > There have been a couple more changes upstream that I need. > > > > Sorry for the unneccessary churn. > > > > ok? > > I'd like to commit this. Ok? ok tb
Re: sync libfido2 with upstream
On Mon, 17 Aug 2020, Damien Miller wrote:
> On Mon, 10 Aug 2020, Damien Miller wrote:
>
> > Hi,
> >
> > This syncs libfido2 with the current state of upstream. It includes
> > a few new APIs that I want to use in OpenSSH to improve FIDO token
> > support (require-PIN and fixing some corner-case bugs around multiple
> > inserted tokens).
> >
> > ok?
> >
> > (major crank for ABI change)
>
> So I pounced on the new API a bit too soon and before it stabilised.
> There have been a couple more changes upstream that I need.
>
> Sorry for the unneccessary churn.
>
> ok?
I'd like to commit this. Ok?
(to be clear - nothing in src/ yet uses the APIs affected by this change)
Index: README.openbsd
===
RCS file: /cvs/src/lib/libfido2/README.openbsd,v
retrieving revision 1.3
diff -u -p -r1.3 README.openbsd
--- README.openbsd 11 Aug 2020 08:44:53 - 1.3
+++ README.openbsd 21 Aug 2020 01:42:03 -
@@ -1,4 +1,4 @@
-This is an import of https://github.com/Yubico/libfido2 2fa20b889 (20200810)
+This is an import of https://github.com/Yubico/libfido2 46710ac06 (20200815)
Local changes:
Index: shlib_version
===
RCS file: /cvs/src/lib/libfido2/shlib_version,v
retrieving revision 1.4
diff -u -p -r1.4 shlib_version
--- shlib_version 11 Aug 2020 08:44:53 - 1.4
+++ shlib_version 21 Aug 2020 01:42:03 -
@@ -1,2 +1,2 @@
-major=3
+major=4
minor=0
Index: man/fido_dev_get_touch_begin.3
===
RCS file: /cvs/src/lib/libfido2/man/fido_dev_get_touch_begin.3,v
retrieving revision 1.1
diff -u -p -r1.1 fido_dev_get_touch_begin.3
--- man/fido_dev_get_touch_begin.3 11 Aug 2020 08:44:53 - 1.1
+++ man/fido_dev_get_touch_begin.3 21 Aug 2020 01:42:03 -
@@ -14,7 +14,7 @@
.Ft int
.Fn fido_dev_get_touch_begin "fido_dev_t *dev"
.Ft int
-.Fn fido_dev_get_touch_status "fido_dev_t *dev" "int *touched" "int *pin_set"
"int ms"
+.Fn fido_dev_get_touch_status "fido_dev_t *dev" "int *touched" "int ms"
.Sh DESCRIPTION
The functions described in this page allow an application to
asynchronously wait for touch on a FIDO authenticator.
Index: man/fido_dev_open.3
===
RCS file: /cvs/src/lib/libfido2/man/fido_dev_open.3,v
retrieving revision 1.4
diff -u -p -r1.4 fido_dev_open.3
--- man/fido_dev_open.3 11 Aug 2020 08:44:53 - 1.4
+++ man/fido_dev_open.3 21 Aug 2020 01:42:03 -
@@ -16,6 +16,7 @@
.Nm fido_dev_is_fido2 ,
.Nm fido_dev_supports_cred_prot ,
.Nm fido_dev_supports_pin ,
+.Nm fido_dev_has_pin ,
.Nm fido_dev_protocol ,
.Nm fido_dev_build ,
.Nm fido_dev_flags ,
@@ -44,6 +45,8 @@
.Fn fido_dev_supports_cred_prot "const fido_dev_t *dev"
.Ft bool
.Fn fido_dev_supports_pin "const fido_dev_t *dev"
+.Ft bool
+.Fn fido_dev_has_pin "const fido_dev_t *dev"
.Ft uint8_t
.Fn fido_dev_protocol "const fido_dev_t *dev"
.Ft uint8_t
@@ -137,6 +140,14 @@ function returns
if
.Fa dev
supports FIDO 2.0 Client PINs.
+.Pp
+The
+.Fn fido_dev_has_pin
+function returns
+.Dv true
+if
+.Fa dev
+has a FIDO 2.0 Client PIN set.
.Pp
The
.Fn fido_dev_protocol
Index: src/dev.c
===
RCS file: /cvs/src/lib/libfido2/src/dev.c,v
retrieving revision 1.3
diff -u -p -r1.3 dev.c
--- src/dev.c 11 Aug 2020 08:44:53 - 1.3
+++ src/dev.c 21 Aug 2020 01:42:03 -
@@ -123,30 +123,27 @@ static void
fido_dev_set_flags(fido_dev_t *dev, const fido_cbor_info_t *info)
{
char * const*ptr;
+ const bool *val;
size_t len;
ptr = fido_cbor_info_extensions_ptr(info);
len = fido_cbor_info_extensions_len(info);
- for (size_t i = 0; i < len; i++) {
- if (strcmp(ptr[i], "credProtect") == 0) {
- dev->flags |= FIDO_DEV_SUPPORTS_CRED_PROT;
- }
- }
+ for (size_t i = 0; i < len; i++)
+ if (strcmp(ptr[i], "credProtect") == 0)
+ dev->flags |= FIDO_DEV_CRED_PROT;
ptr = fido_cbor_info_options_name_ptr(info);
+ val = fido_cbor_info_options_value_ptr(info);
len = fido_cbor_info_options_len(info);
- for (size_t i = 0; i < len; i++) {
- /*
-* clientPin: PIN supported and set;
-* noclientPin: PIN supported but not set.
-*/
- if (strcmp(ptr[i], "clientPin") == 0 ||
- strcmp(ptr[i], "noclientPin") == 0) {
- dev->flags |= FIDO_DEV_SUPPORTS_PIN;
+ for (size_t i = 0; i < len; i++)
+ if (strcmp(ptr[i], "clientPin") == 0) {
+ if (val[i] == true)
+ dev->flags |= FIDO_DEV_PIN_SET;
+ else
+
ubcmtp* at uhidev? ?
Is ubcmtp(4) really attaching to uhidev(4)? I only can see it
attaching to uhub(4). Do I miss something?
Index: sys/dev/usb/ubcmtp.c
===
RCS file: /cvs/src/sys/dev/usb/ubcmtp.c,v
retrieving revision 1.21
diff -u -p -u -p -r1.21 ubcmtp.c
--- sys/dev/usb/ubcmtp.c31 Jul 2020 10:49:33 - 1.21
+++ sys/dev/usb/ubcmtp.c20 Aug 2020 21:14:31 -
@@ -47,7 +47,6 @@
#include
#include
#include
-#include
#include
#include
@@ -328,7 +327,6 @@ struct ubcmtp_softc {
struct ubcmtp_dev *dev_type;
- struct uhidev sc_hdev;
struct usbd_device *sc_udev;
struct device *sc_wsmousedev;
Index: share/man/man4/ubcmtp.4
===
RCS file: /cvs/src/share/man/man4/ubcmtp.4,v
retrieving revision 1.2
diff -u -p -u -p -r1.2 ubcmtp.4
--- share/man/man4/ubcmtp.4 25 May 2019 15:47:46 - 1.2
+++ share/man/man4/ubcmtp.4 20 Aug 2020 21:14:31 -
@@ -34,7 +34,7 @@
.Nm ubcmtp
.Nd Broadcom trackpad mouse
.Sh SYNOPSIS
-.Cd "ubcmtp* at uhidev?"
+.Cd "ubcmtp* at uhub?"
.Cd "wsmouse* at ubcmtp? mux 0"
.Sh DESCRIPTION
The
@@ -42,7 +42,7 @@ The
driver provides support for Broadcom multi-touch trackpads found on
newer Apple MacBook, MacBook Pro, and MacBook Air laptops.
.Sh SEE ALSO
-.Xr uhidev 4 ,
+.Xr uhub 4 ,
.Xr usb 4 ,
.Xr wsmouse 4
.Sh HISTORY
Index: share/man/man4/uhidev.4
===
RCS file: /cvs/src/share/man/man4/uhidev.4,v
retrieving revision 1.10
diff -u -p -u -p -r1.10 uhidev.4
--- share/man/man4/uhidev.4 25 Aug 2018 20:31:31 - 1.10
+++ share/man/man4/uhidev.4 20 Aug 2020 21:14:31 -
@@ -36,7 +36,6 @@
.Nd USB Human Interface Device support
.Sh SYNOPSIS
.Cd "uhidev* at uhub?"
-.Cd "ubcmtp* at uhidev?"
.Cd "ucycom* at uhidev?"
.Cd "ugold* at uhidev?"
.Cd "uhid*at uhidev?"
@@ -68,7 +67,6 @@ kinds of devices and
only dispatches data to them based on the report id.
.Sh SEE ALSO
.Xr intro 4 ,
-.Xr ubcmtp 4 ,
.Xr ucycom 4 ,
.Xr ugold 4 ,
.Xr uhid 4 ,
make(1): anchors in :S/old_string/new_string/
gnezdo noticed that :S/old_string/new_string/ modifiers such as
:S/^sth/&/ and :S/sth$/&/ with an anchor in the old_string and an &
in the new_string don't work as documented (and expected) since they
replace & with old_string including the anchors.
This is because get_spatternarg() deals with skipping the anchors in
pattern->lhs only after having replaced any '&' in new_string with
pattern->lhs. This happens in VarGetPattern if the last argument is
non-NULL when pattern->rhs is determined, i.e., before
common_get_patternarg() is done.
I think the fix should be as simple as the diff below. I added a small
extension of a regress test for this. Unpatched make fails, patched
make passes.
Simple test case that illustrates the problem:
$ cat > makefile
A= foo bar barr
B= ${A:S/^b/s&/}
C= ${A:S/r$/&/}
D= ${A:S/^bar$/&/}
all:
@echo "B= $B"
@echo "C= $C"
@echo "D= $D"
EOF
$ make
B= foo s^bar s^barr
C= foo bar$ barr$
D= foo ^bar$bar barr
I'm not entirely clear on why I get that result for D, but with the
patch below, I get the expected " foo barbarian barr"
Index: usr.bin/make/varmodifiers.c
===
RCS file: /var/cvs/src/usr.bin/make/varmodifiers.c,v
retrieving revision 1.47
diff -u -p -r1.47 varmodifiers.c
--- usr.bin/make/varmodifiers.c 10 Jul 2017 07:10:29 - 1.47
+++ usr.bin/make/varmodifiers.c 20 Aug 2020 16:03:54 -
@@ -1215,21 +1215,7 @@ get_patternarg(const char **p, SymTable
static void *
get_spatternarg(const char **p, SymTable *ctxt, bool err, int endc)
{
- VarPattern *pattern;
-
- pattern = common_get_patternarg(p, ctxt, err, endc, true);
- if (pattern != NULL && pattern->leftLen > 0) {
- if (pattern->lhs[pattern->leftLen-1] == '$') {
- pattern->leftLen--;
- pattern->flags |= VAR_MATCH_END;
- }
- if (pattern->lhs[0] == '^') {
- pattern->lhs++;
- pattern->leftLen--;
- pattern->flags |= VAR_MATCH_START;
- }
- }
- return pattern;
+ return common_get_patternarg(p, ctxt, err, endc, true);
}
static void
@@ -1304,6 +1290,17 @@ common_get_patternarg(const char **p, Sy
>leftLen, NULL);
pattern->lbuffer = pattern->lhs;
if (pattern->lhs != NULL) {
+ if (dosubst && pattern->leftLen > 0) {
+ if (pattern->lhs[pattern->leftLen-1] == '$') {
+ pattern->leftLen--;
+ pattern->flags |= VAR_MATCH_END;
+ }
+ if (pattern->lhs[0] == '^') {
+ pattern->lhs++;
+ pattern->leftLen--;
+ pattern->flags |= VAR_MATCH_START;
+ }
+ }
pattern->rhs = VarGetPattern(ctxt, err, , delim, delim,
>rightLen, dosubst ? pattern: NULL);
if (pattern->rhs != NULL) {
Index: regress/usr.bin/make/mk21
===
RCS file: /var/cvs/src/regress/usr.bin/make/mk21,v
retrieving revision 1.2
diff -u -p -r1.2 mk21
--- regress/usr.bin/make/mk21 7 Jul 2017 16:31:37 - 1.2
+++ regress/usr.bin/make/mk21 20 Aug 2020 16:20:31 -
@@ -12,7 +12,13 @@ X?=${TRUC:C@([^:/])/.+$@\1/@}
Y?=${TRUC:S/^${X}//:S/^://}
Z?=${TRUC:S/^${TRUC:C@([^:/])/.+$@\1/@}//:S/^://}
+A?=machin truc
+B?=${A:S/^/mot: &/}
+C?=${A:S/$/&: mot/}
+
all:
+ @echo "B= $B"
+ @echo "C= $C"
@echo "S= $S"
@echo "T= $T"
@echo "X= $X"
Index: regress/usr.bin/make/t21.out
===
RCS file: /var/cvs/src/regress/usr.bin/make/t21.out,v
retrieving revision 1.2
diff -u -p -r1.2 t21.out
--- regress/usr.bin/make/t21.out7 Jul 2017 16:31:37 - 1.2
+++ regress/usr.bin/make/t21.out20 Aug 2020 16:20:31 -
@@ -1,3 +1,5 @@
+B= mot: machin mot: truc
+C= machin: mot truc: mot
S= sourceforge/%SUBDIR%/
T= sourceforge/%SUBDIR%/
X= http://heanet.dl.sourceforge.net/
Re: pppoe: add sizes to free() calls
ok mvs@
> On 20 Aug 2020, at 17:12, Klemens Nanni wrote:
>
> On Thu, Aug 20, 2020 at 03:33:17PM +0200, Klemens Nanni wrote:
>> These are straight forward as we either maintain a size variable all the
>> way or can reuse strlen() for free() just like it's done during malloc().
>>
>> One exception is freeing the softc structure, which is fixed in size;
>> `ifconfig pppoe1 create; ifconfig pppoe1 destroy' exercises this code
>> path and does not blow up - as expected.
>>
>> Running fine on a octeon vdsl2 router.
>>
>> Feedback? OK?
> Sorry for the noise, Peter J. Philipp pointed out how I missed the +1
> byte to account for the NUL character which strlen(9) does not count.
> All corresponding malloc calls do strlen() + 1, so without it there's an
> off-by-one in the size.
>
> In my setup I do not exercise the service and concentrator name paths,
> but playing with `ifconfig pppoe0 [[-]pppoeac foo] [[-]pppoesvc bar]'
> doesn't blow up on any of those diff versions, either.
>
> Fixed diff.
>
> Index: if_pppoe.c
> ===
> RCS file: /cvs/src/sys/net/if_pppoe.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 if_pppoe.c
> --- if_pppoe.c28 Jul 2020 09:52:32 - 1.70
> +++ if_pppoe.c20 Aug 2020 13:50:07 -
> @@ -257,15 +267,17 @@ pppoe_clone_destroy(struct ifnet *ifp)
> if_detach(ifp);
>
> if (sc->sc_concentrator_name)
> - free(sc->sc_concentrator_name, M_DEVBUF, 0);
> + free(sc->sc_concentrator_name, M_DEVBUF,
> + strlen(sc->sc_concentrator_name) + 1);
> if (sc->sc_service_name)
> - free(sc->sc_service_name, M_DEVBUF, 0);
> + free(sc->sc_service_name, M_DEVBUF,
> + strlen(sc->sc_service_name) + 1);
> if (sc->sc_ac_cookie)
> - free(sc->sc_ac_cookie, M_DEVBUF, 0);
> + free(sc->sc_ac_cookie, M_DEVBUF, sc->sc_ac_cookie_len);
> if (sc->sc_relay_sid)
> - free(sc->sc_relay_sid, M_DEVBUF, 0);
> + free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len);
>
> - free(sc, M_DEVBUF, 0);
> + free(sc, M_DEVBUF, sizeof(*sc));
>
> return (0);
> }
> @@ -547,7 +559,8 @@ breakbreak:
> }
> if (ac_cookie) {
> if (sc->sc_ac_cookie)
> - free(sc->sc_ac_cookie, M_DEVBUF, 0);
> + free(sc->sc_ac_cookie, M_DEVBUF,
> + sc->sc_ac_cookie_len);
> sc->sc_ac_cookie = malloc(ac_cookie_len, M_DEVBUF,
> M_DONTWAIT);
> if (sc->sc_ac_cookie == NULL)
> @@ -557,7 +570,8 @@ breakbreak:
> }
> if (relay_sid) {
> if (sc->sc_relay_sid)
> - free(sc->sc_relay_sid, M_DEVBUF, 0);
> + free(sc->sc_relay_sid, M_DEVBUF,
> + sc->sc_relay_sid_len);
> sc->sc_relay_sid = malloc(relay_sid_len, M_DEVBUF,
> M_DONTWAIT);
> if (sc->sc_relay_sid == NULL)
> @@ -610,11 +624,12 @@ breakbreak:
> sc->sc_state = PPPOE_STATE_INITIAL;
> memcpy(>sc_dest, etherbroadcastaddr, sizeof(sc->sc_dest));
> if (sc->sc_ac_cookie) {
> - free(sc->sc_ac_cookie, M_DEVBUF, 0);
> + free(sc->sc_ac_cookie, M_DEVBUF,
> + sc->sc_ac_cookie_len);
> sc->sc_ac_cookie = NULL;
> }
> if (sc->sc_relay_sid) {
> - free(sc->sc_relay_sid, M_DEVBUF, 0);
> + free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len);
> sc->sc_relay_sid = NULL;
> }
> sc->sc_ac_cookie_len = 0;
> @@ -817,7 +847,8 @@ pppoe_ioctl(struct ifnet *ifp, unsigned
> }
>
> if (sc->sc_concentrator_name)
> - free(sc->sc_concentrator_name, M_DEVBUF, 0);
> + free(sc->sc_concentrator_name, M_DEVBUF,
> + strlen(sc->sc_concentrator_name) + 1);
> sc->sc_concentrator_name = NULL;
>
> len = strlen(parms->ac_name);
> @@ -830,7 +861,8 @@ pppoe_ioctl(struct ifnet *ifp, unsigned
> }
>
> if (sc->sc_service_name)
> - free(sc->sc_service_name, M_DEVBUF, 0);
> + free(sc->sc_service_name, M_DEVBUF,
> + strlen(sc->sc_service_name) + 1);
> sc->sc_service_name = NULL;
>
> len = strlen(parms->service_name);
> @@ -1175,12 +1207,12 @@ pppoe_disconnect(struct pppoe_softc *sc)
> sc->sc_state = PPPOE_STATE_INITIAL;
> memcpy(>sc_dest, etherbroadcastaddr, sizeof(sc->sc_dest));
> if (sc->sc_ac_cookie) {
> -
Re: Log mutex for msgbuf concurrency control
On Tue, Aug 18, 2020 at 03:06:58PM +, Visa Hankala wrote:
> This diff introduces a mutex that serializes access to the kernel
> message buffers. At the moment, the buffers do not have clear
> concurrency control.
>
> The mutex controls access to all the modifiable fields of struct msgbuf.
> It also protects logsoftc.sc_state for managing thread wakeups.
>
> A tricky thing with the diff is that the code is indirectly used in many
> different contexts and in varied machine-specific functions. Luckily,
> the code already utilizes splhigh() in critical parts, so the transition
> to using a mutex possibly is not such a big step any longer.
>
> To avoid creating problems with lock order, the code does not take
> new locks when the log mutex is held. This is visible in logwakeup()
> and logread(). It looks that selrecord() does not acquire new locks,
> so no extra steps are needed in logpoll().
>
> In logread(), the code assumes that there is only one reader. This keeps
> the data extraction logic simple.
>
> An additional benefit (?) of the diff is that now uiomove(9) is called
> without splhigh().
>
> I have tested the diff on amd64 and octeon. However, additional tests
> on these and other platforms would be very helpful.
Here is an updated diff that has sleep_setup()'s priority parameter
fixed. The mistake was noticed by mpi@.
Index: kern/subr_log.c
===
RCS file: src/sys/kern/subr_log.c,v
retrieving revision 1.68
diff -u -p -r1.68 subr_log.c
--- kern/subr_log.c 18 Aug 2020 13:41:49 - 1.68
+++ kern/subr_log.c 20 Aug 2020 13:52:05 -
@@ -52,6 +52,7 @@
#include
#include
#include
+#include
#include
#ifdef KTRACE
@@ -69,8 +70,12 @@
#define LOG_ASYNC 0x04
#define LOG_RDWAIT 0x08
+/*
+ * Locking:
+ * L log_mtx
+ */
struct logsoftc {
- int sc_state; /* see above for possibilities */
+ int sc_state; /* [L] see above for possibilities */
struct selinfo sc_selp;/* process waiting on select call */
struct sigio_ref sc_sigio; /* async I/O registration */
int sc_need_wakeup; /* if set, wake up waiters */
@@ -83,6 +88,14 @@ struct msgbuf *msgbufp;/* the mapped b
struct msgbuf *consbufp; /* console message buffer. */
struct file *syslogf;
+/*
+ * Lock that serializes access to log message buffers.
+ * This should be kept as a leaf lock in order not to constrain where
+ * printf(9) can be used.
+ */
+struct mutex log_mtx =
+MUTEX_INITIALIZER_FLAGS(IPL_HIGH, "logmtx", MTX_NOWITNESS);
+
void filt_logrdetach(struct knote *kn);
int filt_logread(struct knote *kn, long hint);
@@ -144,13 +157,11 @@ initconsbuf(void)
void
msgbuf_putchar(struct msgbuf *mbp, const char c)
{
- int s;
-
if (mbp->msg_magic != MSG_MAGIC)
/* Nothing we can do */
return;
- s = splhigh();
+ mtx_enter(_mtx);
mbp->msg_bufc[mbp->msg_bufx++] = c;
if (mbp->msg_bufx < 0 || mbp->msg_bufx >= mbp->msg_bufs)
mbp->msg_bufx = 0;
@@ -160,20 +171,19 @@ msgbuf_putchar(struct msgbuf *mbp, const
mbp->msg_bufr = 0;
mbp->msg_bufd++;
}
- splx(s);
+ mtx_leave(_mtx);
}
size_t
msgbuf_getlen(struct msgbuf *mbp)
{
long len;
- int s;
- s = splhigh();
+ mtx_enter(_mtx);
len = mbp->msg_bufx - mbp->msg_bufr;
if (len < 0)
len += mbp->msg_bufs;
- splx(s);
+ mtx_leave(_mtx);
return (len);
}
@@ -208,34 +218,47 @@ logclose(dev_t dev, int flag, int mode,
int
logread(dev_t dev, struct uio *uio, int flag)
{
+ struct sleep_state sls;
struct msgbuf *mbp = msgbufp;
- size_t l;
- int s, error = 0;
+ size_t l, rpos;
+ int error = 0;
- s = splhigh();
+ mtx_enter(_mtx);
while (mbp->msg_bufr == mbp->msg_bufx) {
if (flag & IO_NDELAY) {
error = EWOULDBLOCK;
goto out;
}
logsoftc.sc_state |= LOG_RDWAIT;
- error = tsleep_nsec(mbp, LOG_RDPRI | PCATCH, "klog", INFSLP);
+ mtx_leave(_mtx);
+ /*
+* Set up and enter sleep manually instead of using msleep()
+* to keep log_mtx as a leaf lock.
+*/
+ sleep_setup(, mbp, LOG_RDPRI | PCATCH, "klog");
+ sleep_setup_signal();
+ sleep_finish(, logsoftc.sc_state & LOG_RDWAIT);
+ error = sleep_finish_signal();
+ mtx_enter(_mtx);
if (error)
goto out;
}
- logsoftc.sc_state &= ~LOG_RDWAIT;
if (mbp->msg_bufd > 0) {
char buf[64];
+ long ndropped;
+
Re: pppoe: add sizes to free() calls
> On 20 Aug 2020, at 16:33, Klemens Nanni wrote:
>
> These are straight forward as we either maintain a size variable all the
> way or can reuse strlen() for free() just like it's done during malloc().
>
> One exception is freeing the softc structure, which is fixed in size;
> `ifconfig pppoe1 create; ifconfig pppoe1 destroy' exercises this code
> path and does not blow up - as expected.
>
> Running fine on a octeon vdsl2 router.
>
> Feedback? OK?
Hi.
You forgot about ‘\0’ for `sc_concentrator_name’ and `sc_service_name'.
>
>
> Index: if_pppoe.c
> ===
> RCS file: /cvs/src/sys/net/if_pppoe.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 if_pppoe.c
> --- if_pppoe.c28 Jul 2020 09:52:32 - 1.70
> +++ if_pppoe.c19 Aug 2020 23:33:23 -
> @@ -257,15 +264,17 @@ pppoe_clone_destroy(struct ifnet *ifp)
> if_detach(ifp);
>
> if (sc->sc_concentrator_name)
> - free(sc->sc_concentrator_name, M_DEVBUF, 0);
> + free(sc->sc_concentrator_name, M_DEVBUF,
> + strlen(sc->sc_concentrator_name));
> if (sc->sc_service_name)
> - free(sc->sc_service_name, M_DEVBUF, 0);
> + free(sc->sc_service_name, M_DEVBUF,
> + strlen(sc->sc_service_name));
> if (sc->sc_ac_cookie)
> - free(sc->sc_ac_cookie, M_DEVBUF, 0);
> + free(sc->sc_ac_cookie, M_DEVBUF, sc->sc_ac_cookie_len);
> if (sc->sc_relay_sid)
> - free(sc->sc_relay_sid, M_DEVBUF, 0);
> + free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len);
>
> - free(sc, M_DEVBUF, 0);
> + free(sc, M_DEVBUF, sizeof(*sc));
>
> return (0);
> }
> @@ -547,7 +556,8 @@ breakbreak:
> }
> if (ac_cookie) {
> if (sc->sc_ac_cookie)
> - free(sc->sc_ac_cookie, M_DEVBUF, 0);
> + free(sc->sc_ac_cookie, M_DEVBUF,
> + sc->sc_ac_cookie_len);
> sc->sc_ac_cookie = malloc(ac_cookie_len, M_DEVBUF,
> M_DONTWAIT);
> if (sc->sc_ac_cookie == NULL)
> @@ -557,7 +567,8 @@ breakbreak:
> }
> if (relay_sid) {
> if (sc->sc_relay_sid)
> - free(sc->sc_relay_sid, M_DEVBUF, 0);
> + free(sc->sc_relay_sid, M_DEVBUF,
> + sc->sc_relay_sid_len);
> sc->sc_relay_sid = malloc(relay_sid_len, M_DEVBUF,
> M_DONTWAIT);
> if (sc->sc_relay_sid == NULL)
> @@ -610,11 +621,12 @@ breakbreak:
> sc->sc_state = PPPOE_STATE_INITIAL;
> memcpy(>sc_dest, etherbroadcastaddr, sizeof(sc->sc_dest));
> if (sc->sc_ac_cookie) {
> - free(sc->sc_ac_cookie, M_DEVBUF, 0);
> + free(sc->sc_ac_cookie, M_DEVBUF,
> + sc->sc_ac_cookie_len);
> sc->sc_ac_cookie = NULL;
> }
> if (sc->sc_relay_sid) {
> - free(sc->sc_relay_sid, M_DEVBUF, 0);
> + free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len);
> sc->sc_relay_sid = NULL;
> }
> sc->sc_ac_cookie_len = 0;
> @@ -817,7 +835,8 @@ pppoe_ioctl(struct ifnet *ifp, unsigned
> }
>
> if (sc->sc_concentrator_name)
> - free(sc->sc_concentrator_name, M_DEVBUF, 0);
> + free(sc->sc_concentrator_name, M_DEVBUF,
> + strlen(sc->sc_concentrator_name));
> sc->sc_concentrator_name = NULL;
>
> len = strlen(parms->ac_name);
> @@ -830,7 +849,8 @@ pppoe_ioctl(struct ifnet *ifp, unsigned
> }
>
> if (sc->sc_service_name)
> - free(sc->sc_service_name, M_DEVBUF, 0);
> + free(sc->sc_service_name, M_DEVBUF,
> + strlen(sc->sc_service_name));
> sc->sc_service_name = NULL;
>
> len = strlen(parms->service_name);
> @@ -1175,12 +1195,12 @@ pppoe_disconnect(struct pppoe_softc *sc)
> sc->sc_state = PPPOE_STATE_INITIAL;
> memcpy(>sc_dest, etherbroadcastaddr, sizeof(sc->sc_dest));
> if (sc->sc_ac_cookie) {
> - free(sc->sc_ac_cookie, M_DEVBUF, 0);
> + free(sc->sc_ac_cookie, M_DEVBUF, sc->sc_ac_cookie_len);
> sc->sc_ac_cookie = NULL;
> }
> sc->sc_ac_cookie_len = 0;
> if (sc->sc_relay_sid) {
> - free(sc->sc_relay_sid, M_DEVBUF, 0);
> + free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len);
> sc->sc_relay_sid = NULL;
> }
> sc->sc_relay_sid_len = 0;
>
Re: pppoe: add sizes to free() calls
On Thu, Aug 20, 2020 at 03:33:17PM +0200, Klemens Nanni wrote:
> These are straight forward as we either maintain a size variable all the
> way or can reuse strlen() for free() just like it's done during malloc().
>
> One exception is freeing the softc structure, which is fixed in size;
> `ifconfig pppoe1 create; ifconfig pppoe1 destroy' exercises this code
> path and does not blow up - as expected.
>
> Running fine on a octeon vdsl2 router.
>
> Feedback? OK?
Sorry for the noise, Peter J. Philipp pointed out how I missed the +1
byte to account for the NUL character which strlen(9) does not count.
All corresponding malloc calls do strlen() + 1, so without it there's an
off-by-one in the size.
In my setup I do not exercise the service and concentrator name paths,
but playing with `ifconfig pppoe0 [[-]pppoeac foo] [[-]pppoesvc bar]'
doesn't blow up on any of those diff versions, either.
Fixed diff.
Index: if_pppoe.c
===
RCS file: /cvs/src/sys/net/if_pppoe.c,v
retrieving revision 1.70
diff -u -p -r1.70 if_pppoe.c
--- if_pppoe.c 28 Jul 2020 09:52:32 - 1.70
+++ if_pppoe.c 20 Aug 2020 13:50:07 -
@@ -257,15 +267,17 @@ pppoe_clone_destroy(struct ifnet *ifp)
if_detach(ifp);
if (sc->sc_concentrator_name)
- free(sc->sc_concentrator_name, M_DEVBUF, 0);
+ free(sc->sc_concentrator_name, M_DEVBUF,
+ strlen(sc->sc_concentrator_name) + 1);
if (sc->sc_service_name)
- free(sc->sc_service_name, M_DEVBUF, 0);
+ free(sc->sc_service_name, M_DEVBUF,
+ strlen(sc->sc_service_name) + 1);
if (sc->sc_ac_cookie)
- free(sc->sc_ac_cookie, M_DEVBUF, 0);
+ free(sc->sc_ac_cookie, M_DEVBUF, sc->sc_ac_cookie_len);
if (sc->sc_relay_sid)
- free(sc->sc_relay_sid, M_DEVBUF, 0);
+ free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len);
- free(sc, M_DEVBUF, 0);
+ free(sc, M_DEVBUF, sizeof(*sc));
return (0);
}
@@ -547,7 +559,8 @@ breakbreak:
}
if (ac_cookie) {
if (sc->sc_ac_cookie)
- free(sc->sc_ac_cookie, M_DEVBUF, 0);
+ free(sc->sc_ac_cookie, M_DEVBUF,
+ sc->sc_ac_cookie_len);
sc->sc_ac_cookie = malloc(ac_cookie_len, M_DEVBUF,
M_DONTWAIT);
if (sc->sc_ac_cookie == NULL)
@@ -557,7 +570,8 @@ breakbreak:
}
if (relay_sid) {
if (sc->sc_relay_sid)
- free(sc->sc_relay_sid, M_DEVBUF, 0);
+ free(sc->sc_relay_sid, M_DEVBUF,
+ sc->sc_relay_sid_len);
sc->sc_relay_sid = malloc(relay_sid_len, M_DEVBUF,
M_DONTWAIT);
if (sc->sc_relay_sid == NULL)
@@ -610,11 +624,12 @@ breakbreak:
sc->sc_state = PPPOE_STATE_INITIAL;
memcpy(>sc_dest, etherbroadcastaddr, sizeof(sc->sc_dest));
if (sc->sc_ac_cookie) {
- free(sc->sc_ac_cookie, M_DEVBUF, 0);
+ free(sc->sc_ac_cookie, M_DEVBUF,
+ sc->sc_ac_cookie_len);
sc->sc_ac_cookie = NULL;
}
if (sc->sc_relay_sid) {
- free(sc->sc_relay_sid, M_DEVBUF, 0);
+ free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len);
sc->sc_relay_sid = NULL;
}
sc->sc_ac_cookie_len = 0;
@@ -817,7 +847,8 @@ pppoe_ioctl(struct ifnet *ifp, unsigned
}
if (sc->sc_concentrator_name)
- free(sc->sc_concentrator_name, M_DEVBUF, 0);
+ free(sc->sc_concentrator_name, M_DEVBUF,
+ strlen(sc->sc_concentrator_name) + 1);
sc->sc_concentrator_name = NULL;
len = strlen(parms->ac_name);
@@ -830,7 +861,8 @@ pppoe_ioctl(struct ifnet *ifp, unsigned
}
if (sc->sc_service_name)
- free(sc->sc_service_name, M_DEVBUF, 0);
+ free(sc->sc_service_name, M_DEVBUF,
+ strlen(sc->sc_service_name) + 1);
sc->sc_service_name = NULL;
len = strlen(parms->service_name);
@@ -1175,12 +1207,12 @@ pppoe_disconnect(struct pppoe_softc *sc)
sc->sc_state = PPPOE_STATE_INITIAL;
memcpy(>sc_dest, etherbroadcastaddr, sizeof(sc->sc_dest));
if (sc->sc_ac_cookie) {
- free(sc->sc_ac_cookie, M_DEVBUF, 0);
+ free(sc->sc_ac_cookie, M_DEVBUF, sc->sc_ac_cookie_len);
pppoe: add sizes to free() calls
These are straight forward as we either maintain a size variable all the
way or can reuse strlen() for free() just like it's done during malloc().
One exception is freeing the softc structure, which is fixed in size;
`ifconfig pppoe1 create; ifconfig pppoe1 destroy' exercises this code
path and does not blow up - as expected.
Running fine on a octeon vdsl2 router.
Feedback? OK?
Index: if_pppoe.c
===
RCS file: /cvs/src/sys/net/if_pppoe.c,v
retrieving revision 1.70
diff -u -p -r1.70 if_pppoe.c
--- if_pppoe.c 28 Jul 2020 09:52:32 - 1.70
+++ if_pppoe.c 19 Aug 2020 23:33:23 -
@@ -257,15 +264,17 @@ pppoe_clone_destroy(struct ifnet *ifp)
if_detach(ifp);
if (sc->sc_concentrator_name)
- free(sc->sc_concentrator_name, M_DEVBUF, 0);
+ free(sc->sc_concentrator_name, M_DEVBUF,
+ strlen(sc->sc_concentrator_name));
if (sc->sc_service_name)
- free(sc->sc_service_name, M_DEVBUF, 0);
+ free(sc->sc_service_name, M_DEVBUF,
+ strlen(sc->sc_service_name));
if (sc->sc_ac_cookie)
- free(sc->sc_ac_cookie, M_DEVBUF, 0);
+ free(sc->sc_ac_cookie, M_DEVBUF, sc->sc_ac_cookie_len);
if (sc->sc_relay_sid)
- free(sc->sc_relay_sid, M_DEVBUF, 0);
+ free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len);
- free(sc, M_DEVBUF, 0);
+ free(sc, M_DEVBUF, sizeof(*sc));
return (0);
}
@@ -547,7 +556,8 @@ breakbreak:
}
if (ac_cookie) {
if (sc->sc_ac_cookie)
- free(sc->sc_ac_cookie, M_DEVBUF, 0);
+ free(sc->sc_ac_cookie, M_DEVBUF,
+ sc->sc_ac_cookie_len);
sc->sc_ac_cookie = malloc(ac_cookie_len, M_DEVBUF,
M_DONTWAIT);
if (sc->sc_ac_cookie == NULL)
@@ -557,7 +567,8 @@ breakbreak:
}
if (relay_sid) {
if (sc->sc_relay_sid)
- free(sc->sc_relay_sid, M_DEVBUF, 0);
+ free(sc->sc_relay_sid, M_DEVBUF,
+ sc->sc_relay_sid_len);
sc->sc_relay_sid = malloc(relay_sid_len, M_DEVBUF,
M_DONTWAIT);
if (sc->sc_relay_sid == NULL)
@@ -610,11 +621,12 @@ breakbreak:
sc->sc_state = PPPOE_STATE_INITIAL;
memcpy(>sc_dest, etherbroadcastaddr, sizeof(sc->sc_dest));
if (sc->sc_ac_cookie) {
- free(sc->sc_ac_cookie, M_DEVBUF, 0);
+ free(sc->sc_ac_cookie, M_DEVBUF,
+ sc->sc_ac_cookie_len);
sc->sc_ac_cookie = NULL;
}
if (sc->sc_relay_sid) {
- free(sc->sc_relay_sid, M_DEVBUF, 0);
+ free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len);
sc->sc_relay_sid = NULL;
}
sc->sc_ac_cookie_len = 0;
@@ -817,7 +835,8 @@ pppoe_ioctl(struct ifnet *ifp, unsigned
}
if (sc->sc_concentrator_name)
- free(sc->sc_concentrator_name, M_DEVBUF, 0);
+ free(sc->sc_concentrator_name, M_DEVBUF,
+ strlen(sc->sc_concentrator_name));
sc->sc_concentrator_name = NULL;
len = strlen(parms->ac_name);
@@ -830,7 +849,8 @@ pppoe_ioctl(struct ifnet *ifp, unsigned
}
if (sc->sc_service_name)
- free(sc->sc_service_name, M_DEVBUF, 0);
+ free(sc->sc_service_name, M_DEVBUF,
+ strlen(sc->sc_service_name));
sc->sc_service_name = NULL;
len = strlen(parms->service_name);
@@ -1175,12 +1195,12 @@ pppoe_disconnect(struct pppoe_softc *sc)
sc->sc_state = PPPOE_STATE_INITIAL;
memcpy(>sc_dest, etherbroadcastaddr, sizeof(sc->sc_dest));
if (sc->sc_ac_cookie) {
- free(sc->sc_ac_cookie, M_DEVBUF, 0);
+ free(sc->sc_ac_cookie, M_DEVBUF, sc->sc_ac_cookie_len);
sc->sc_ac_cookie = NULL;
}
sc->sc_ac_cookie_len = 0;
if (sc->sc_relay_sid) {
- free(sc->sc_relay_sid, M_DEVBUF, 0);
+ free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len);
sc->sc_relay_sid = NULL;
}
sc->sc_relay_sid_len = 0;
Re: release.8 - .fs -> .img
On Wed, Aug 19, 2020 at 11:42:39PM -0700, na...@airpost.net wrote:
> Update the name of the install image.
Fixed, thanks.
>
> Index: release.8
> ===
> RCS file: /cvs/src/share/man/man8/release.8,v
> retrieving revision 1.95
> diff -u -p -r1.95 release.8
> --- release.8 29 Apr 2020 15:02:51 - 1.95
> +++ release.8 20 Aug 2020 06:38:16 -
> @@ -258,7 +258,7 @@ This is described in
> .Xr ports 7 .
> .Ss 8. Create boot and installation disk images
> The disk images
> -.No install${ Ns Va VERSION Ns }.fs
> +.No install${ Ns Va VERSION Ns }.img
> and
> .No install${ Ns Va VERSION Ns }.iso
> are suitable for installs without network connectivity.
>
release.8 - .fs -> .img
Update the name of the install image.
Index: release.8
===
RCS file: /cvs/src/share/man/man8/release.8,v
retrieving revision 1.95
diff -u -p -r1.95 release.8
--- release.8 29 Apr 2020 15:02:51 - 1.95
+++ release.8 20 Aug 2020 06:38:16 -
@@ -258,7 +258,7 @@ This is described in
.Xr ports 7 .
.Ss 8. Create boot and installation disk images
The disk images
-.No install${ Ns Va VERSION Ns }.fs
+.No install${ Ns Va VERSION Ns }.img
and
.No install${ Ns Va VERSION Ns }.iso
are suitable for installs without network connectivity.
