athn auto media type (Was: Re: Explicit NULL assignment after free)
Stefan Sperling wrote: > On Sun, Jan 01, 2023 at 09:00:30PM +0000, Ali Farzanrad wrote: > > Hi Stefan, > > > > Stefan Sperling wrote: > > > On Sun, Jan 01, 2023 at 05:00:35PM +, Ali Farzanrad wrote: > > > > Hi tech@, > > > > > > > > Happy new year! > > > > I have some weird problems with my athn interface which drives me crazy. > > > > > > > > 1. Whenever I configure my athn as `media auto' for the first time it > > > > correctly detects correct media subclass, but as soon as I select exact > > > > same media subclass manually, it diverts to DS1 media subclass and after > > > > that auto will not work again (I need to reboot system or my WiFi > > > > provider aka my Android phone). > > > > > > You are not supposed to force a specific Tx rate, unless you are > > > debugging a Tx-specific problem in the code. The driver will > > > adjust the Tx rate on demand, based on packet loss statistics. > > > If you force a specific Tx rate then associated devices will either see > > > a lot of packet loss (if the rate is too high and the device is too far > > > away) or very low speed (if the Tx rate is forced to a low rate). > > > > OK, but the point is DS1 is not a correst config for my Android WiFi. > > Normally it should be HT-MCS0 or HT-MCS7 in 11n mode. > > Given your android phone is the AP which athn(4) connects to, indeed, > the driver should use HT-MCS0 up to HT-MCS7 while connected in 11n mode > (retried frames will use legacy rates if HT-MCS0 doesn't work, but this > won't be displayed by ifconfig; you would have to capture frames from > the air with a separate wifi device in monitor mode to see this). > > Because there is no support for Tx aggregation and 40Mhz channels in > our athn driver yet, running athn in 11n mode does not provide a > significant performance advantage over 11a/11g modes. The driver does > support Rx aggregation though, so at least traffic sent from the AP to > the athn client may be a bit faster than in 11a/11g modes. > > > > On 2Ghz channels the driver will start out in DS1 and adjust upwards, > > > and adjust the rate upwards if packet loss remains low while doing so. > > > > So it might be my Android phone problem which caused my system think DS1 > > is the correct config, right? > > I don't know why ifconfig would show DS1 if 11n mode is active. > I believe it is possible for DS1 to show up somewhere in ifconfig output > If you forced it to use DS1 earlier and produced a non-default media > config as a result. In such cases the media info shown by ifconfig might > be incorrect and confusing. No polish has gone into that part of the > user-facing display because it is not really intended for users. AFAIK the > commands which force a particular Tx rate aren't even documented anymore. > We removed the docs at some point because regular users should not need > to fiddle with wifi Tx rate selection. > > You might simply not be switching back into auto-selection mode with > the commands you are running. Try rebooting with a hostnname.athn0 > that does not contain any 'media' and 'mode' commands and it should > be reset to defaults (that is the foolproof way to reset things; there > are other ways but I'd rather avoid talking about those without a solid > understanding of what you were doing). OK, I started in a working state where I was connected to my Android phone: # cat /etc/hostname.athn0 nwid [..name..] wpakey [..pass..] inet autoconf # ifconfig athn0 athn0: flags=808843 mtu 1500 lladdr [..lladdr..] index 1 priority 4 llprio 3 groups: wlan egress media: IEEE802.11 autoselect (HT-MCS7 mode 11n) status: active ieee80211: nwid [..name..] chan 9 bssid [..bssid..] -45dBm wpakey wpaprotos wpa2 wpaakms psk wpaciphers ccmp wpagroupcipher ccmp inet 192.168.131.83 netmask 0xff00 broadcast 192.168.131.255 then I simply re-run netstart: # sh /etc/netstart athn0 # ifconfig athn0 athn0: flags=808843 mtu 1500 lladdr [..lladdr..] index 1 priority 4 llprio 3 groups: wlan egress media: IEEE802.11 autoselect (DS1 mode 11b) status: no network ieee80211: nwid [..name..] wpakey wpaprotos wpa2 wpaakms psk wpaciphers ccmp wpagroupcipher ccmp inet 192.168.131.83 netmask 0xff00 broadcast 192.168.131.255 [..after some time..] # ifconfig athn0 athn0: flags=808843 mtu 1500 lladdr [..lladdr..] index 1 priority 4 llprio 3 groups: wlan egress media: IEEE802.11 autoselect (DS1 mode 11g) status: no network ieee80211: nwid [..name..] chan 9 bssid [..bssid..] -100dBm wpakey
Re: Explicit NULL assignment after free
Hi Stefan,
Stefan Sperling wrote:
> On Sun, Jan 01, 2023 at 05:00:35PM +0000, Ali Farzanrad wrote:
> > Hi tech@,
> >
> > Happy new year!
> > I have some weird problems with my athn interface which drives me crazy.
> >
> > 1. Whenever I configure my athn as `media auto' for the first time it
> > correctly detects correct media subclass, but as soon as I select exact
> > same media subclass manually, it diverts to DS1 media subclass and after
> > that auto will not work again (I need to reboot system or my WiFi
> > provider aka my Android phone).
>
> You are not supposed to force a specific Tx rate, unless you are
> debugging a Tx-specific problem in the code. The driver will
> adjust the Tx rate on demand, based on packet loss statistics.
> If you force a specific Tx rate then associated devices will either see
> a lot of packet loss (if the rate is too high and the device is too far
> away) or very low speed (if the Tx rate is forced to a low rate).
OK, but the point is DS1 is not a correst config for my Android WiFi.
Normally it should be HT-MCS0 or HT-MCS7 in 11n mode.
> > 2. Sometimes my athn interface will reset all of a sudden and after
> > reset it diverts to DS1 media subclass again.
>
> On 2Ghz channels the driver will start out in DS1 and adjust upwards,
> and adjust the rate upwards if packet loss remains low while doing so.
So it might be my Android phone problem which caused my system think DS1
is the correct config, right?
> > Anyway I decided to read athn related files, but I rarely understand the
> > code. My primary suspect was bad dynamic allocation usage, and I found
> > this:
> >
> > /usr/src/sys/dev/ic/ar5008.c:676: free(rxq->bf, M_DEVBUF, 0);
> > /usr/src/sys/dev/ic/ar5008.c:677:
> > /usr/src/sys/dev/ic/ar5008.c:678: /* Free Rx descriptors. */
> >
> > In this file almost after every free there is an explicit NULL
> > assignment but why there is no NULL assignment after this line?
> >
> > ===
> > RCS file: /home/cvs/src/sys/dev/ic/ar5008.c,v
> > retrieving revision 1.71
> > diff -u -p -r1.71 ar5008.c
> > --- ar5008.c27 Dec 2022 20:13:03 - 1.71
> > +++ ar5008.c1 Jan 2023 16:57:54 -
> > @@ -674,6 +674,7 @@ ar5008_rx_free(struct athn_softc *sc)
> > m_freem(bf->bf_m);
> > }
> > free(rxq->bf, M_DEVBUF, 0);
> > + rxq->bf = NULL;
> >
> > /* Free Rx descriptors. */
> > if (rxq->map != NULL) {
>
> There is no reason to NULL out this pointer.
> ar5008_rx_free() is called from either athn_detach() (via sc->ops.dma_free)
> or if device attachment fails in athn_attach() (via sc->osp.dma_alloc).
> In either case we're not going to be using this allocation ever again.
Oh, I see, thank you and sorry for the noise.
Explicit NULL assignment after free
Hi tech@,
Happy new year!
I have some weird problems with my athn interface which drives me crazy.
1. Whenever I configure my athn as `media auto' for the first time it
correctly detects correct media subclass, but as soon as I select exact
same media subclass manually, it diverts to DS1 media subclass and after
that auto will not work again (I need to reboot system or my WiFi
provider aka my Android phone).
2. Sometimes my athn interface will reset all of a sudden and after
reset it diverts to DS1 media subclass again.
Anyway I decided to read athn related files, but I rarely understand the
code. My primary suspect was bad dynamic allocation usage, and I found
this:
/usr/src/sys/dev/ic/ar5008.c:676: free(rxq->bf, M_DEVBUF, 0);
/usr/src/sys/dev/ic/ar5008.c:677:
/usr/src/sys/dev/ic/ar5008.c:678: /* Free Rx descriptors. */
In this file almost after every free there is an explicit NULL
assignment but why there is no NULL assignment after this line?
===
RCS file: /home/cvs/src/sys/dev/ic/ar5008.c,v
retrieving revision 1.71
diff -u -p -r1.71 ar5008.c
--- ar5008.c27 Dec 2022 20:13:03 - 1.71
+++ ar5008.c1 Jan 2023 16:57:54 -
@@ -674,6 +674,7 @@ ar5008_rx_free(struct athn_softc *sc)
m_freem(bf->bf_m);
}
free(rxq->bf, M_DEVBUF, 0);
+ rxq->bf = NULL;
/* Free Rx descriptors. */
if (rxq->map != NULL) {
Re: acme-client: check token names
Florian Obser wrote:
> On 2022-05-03 17:41 +0430, Ali Farzanrad wrote:
> >
> > Hi Florian,
> >
> > Yes, I read the RFC, it should work, but I couldn't test it yet, because
> > my domain manager is a little lazy (I've registeret 2 subdomains for my
> > domain, but it is not listed in name servers yet). I'll probably test
> > it tomorrow.
> >
> > I read the isalnum man page, it said that isalnum might have different
> > behavior based on locale, so I decide to not use that. Is it OK?
>
> No, please do a
> setlocale(LC_CTYPE, "");
> at the beginning of chngproc, before the unveil and then use isalnum
> instead of handrolling it. It will fit into chngproc, so no need for
> another function
>
> --
> I'm not entirely sure you are real.
>
OK, I've tested following diff on my own domain and it works.
I did 2 modifications:
1. I explicitly call setlocate with "C" to ensure C locale,
2. I did a string length check. According to RFC it must have 128 bit
of random entropy, so it should have at least 22 base64 characters,
but I was unsure. So I only check for empty strings.
Index: chngproc.c
===
RCS file: /cvs/src/usr.sbin/acme-client/chngproc.c,v
retrieving revision 1.16
diff -u -p -r1.16 chngproc.c
--- chngproc.c 12 Jul 2021 15:09:20 - 1.16
+++ chngproc.c 4 May 2022 08:37:32 -
@@ -16,9 +16,11 @@
*/
#include
+#include
#include
#include
#include
+#include
#include
#include
#include
@@ -42,6 +44,11 @@ chngproc(int netsock, const char *root)
goto out;
}
+ if (setlocale(LC_CTYPE, "C") == NULL) {
+ warnx("setlocale");
+ goto out;
+ }
+
if (pledge("stdio cpath wpath", NULL) == -1) {
warn("pledge");
goto out;
@@ -77,6 +84,18 @@ chngproc(int netsock, const char *root)
goto out;
else if ((tok = readstr(netsock, COMM_TOK)) == NULL)
goto out;
+ else if (strlen(tok) < 1) {
+ warnx("token is too short");
+ goto out;
+ }
+
+ for (i = 0; tok[i]; ++i) {
+ int ch = (unsigned char)tok[i];
+ if (!isalnum(ch) && ch != '-' && ch != '_') {
+ warnx("token is not a valid base64url");
+ goto out;
+ }
+ }
if (asprintf(, "%s.%s", tok, th) == -1) {
warn("asprintf");
Re: acme-client: check token names
Florian Obser wrote:
> On 2022-05-02 03:04 +0430, Ali Farzanrad wrote:
> > Hi tech@,
> >
> > I know that acme-client is unveiled properly, but isn't it better to
> > check token names?
>
> Nice catch, the token is untrusted input.
> We should validate this differently though.
>
> RFC 8555, 8.5 HTTP Challenge:
>
>token (required, string): A random value that uniquely identifies
> the challenge. This value MUST have at least 128 bits of entropy.
> It MUST NOT contain any characters outside the base64url alphabet
> and MUST NOT include base64 padding characters ("=").
>
> base64url is defined in
> RFC 4648, 5. Base 64 Encoding with URL and Filename Safe Alphabet
>
> It's basically isalpha || '-' || '_'.
>
> Are you up to implementing that check instead?
>
Hi Florian,
Yes, I read the RFC, it should work, but I couldn't test it yet, because
my domain manager is a little lazy (I've registeret 2 subdomains for my
domain, but it is not listed in name servers yet). I'll probably test
it tomorrow.
I read the isalnum man page, it said that isalnum might have different
behavior based on locale, so I decide to not use that. Is it OK?
Index: chngproc.c
===
RCS file: /cvs/src/usr.sbin/acme-client/chngproc.c,v
retrieving revision 1.16
diff -u -p -r1.16 chngproc.c
--- chngproc.c 12 Jul 2021 15:09:20 - 1.16
+++ chngproc.c 3 May 2022 13:03:45 -
@@ -26,6 +26,8 @@
#include "extern.h"
+static int is_base64url(const char *);
+
int
chngproc(int netsock, const char *root)
{
@@ -77,6 +79,10 @@ chngproc(int netsock, const char *root)
goto out;
else if ((tok = readstr(netsock, COMM_TOK)) == NULL)
goto out;
+ else if (!is_base64url(tok)) {
+ warnx("token is not base64url");
+ goto out;
+ }
if (asprintf(, "%s.%s", tok, th) == -1) {
warn("asprintf");
@@ -152,4 +158,24 @@ out:
free(th);
free(tok);
return rc;
+}
+
+static int
+is_base64url(const char *s)
+{
+ for (; *s; ++s) {
+ int ch = (unsigned char)*s;
+ if ((ch >= '0' && ch <= '9') ||
+ (ch >= 'A' && ch <= 'Z') ||
+ (ch >= 'a' && ch <= 'z'))
+ continue;
+ switch (ch) {
+ case '-':
+ case '_':
+ break;
+ default:
+ return 0;
+ }
+ }
+ return 1;
}
acme-client: check token names
Hi tech@,
I know that acme-client is unveiled properly, but isn't it better to
check token names?
===
RCS file: /cvs/src/usr.sbin/acme-client/chngproc.c,v
retrieving revision 1.16
diff -u -p -r1.16 chngproc.c
--- chngproc.c 12 Jul 2021 15:09:20 - 1.16
+++ chngproc.c 1 May 2022 22:28:43 -
@@ -77,6 +77,11 @@ chngproc(int netsock, const char *root)
goto out;
else if ((tok = readstr(netsock, COMM_TOK)) == NULL)
goto out;
+ else if (strstr(tok, "../") == tok ||
+ strstr(tok, "/../") != NULL) {
+ warnx("bad file path");
+ goto out;
+ }
if (asprintf(, "%s.%s", tok, th) == -1) {
warn("asprintf");
Re: stpecpy(): A better string copy and concatenation function
"Alejandro Colomar (man-pages)" wrote: >char dest[SIZE]; >char *end; > >end = [SIZE - 1]; >stpecpy(dest, "Hello world", end); Perfect way to introduce new hidden backdoors! Just use realloc `dest' to a new value, but forget to update `end' properly: size_t size = 7; char *dest = malloc(size); char *end = [size - 1]; stpecpy(dest, "Hello ", end); dest = realloc(dest, size + 6) end += 6; stpecpy(dest, "World!", end);
Re: Bad comparison between double and uint64_t
Hi Masato,
Masato Asou wrote:
> Hi Ali,
>
> In this case, ULLONG_MAX is implicitly cast to double, isn't it?
Yes it is implicitly cast to double, but due to floating point number
precision, ULLONG_MAX will convert to (ULLONG_MAX + 1).
So in case val is really ULLONG_MAX + 1, this condition is false:
if (val > ULLONG_MAX)
and when we want to convert val to u_int64_t we will have
(ULLONG_MAX + 1) which is 0.
*n = val; /* stores ULLONG_MAX + 1 which is 0 */
I'm not sure if it cause any real problem, but it is wrong and should be
fixed.
>
> Do you have any problems if you don't cast to double?
> --
> ASOU Masato
>
> From: Ali Farzanrad
> Date: Wed, 23 Jun 2021 20:24:27 +
>
> > Oh, my bad, sorry.
> > I assumed that val is always integer.
> > I guess it is better to ignore val == ULLONG_MAX:
> >
> > ===
> > RCS file: /cvs/src/sbin/disklabel/editor.c,v
> > retrieving revision 1.368
> > diff -u -p -r1.368 editor.c
> > --- editor.c2021/05/30 19:02:30 1.368
> > +++ editor.c2021/06/23 20:23:03
> > @@ -2418,7 +2418,7 @@ apply_unit(double val, u_char unit, u_int64_t *n)
> > }
> >
> > val *= factor / DEV_BSIZE;
> > - if (val > ULLONG_MAX)
> > + if (val >= (double)ULLONG_MAX)
> > return (-1);
> > *n = val;
> > return (0);
> > @@ -2429,7 +2429,7 @@ parse_sizespec(const char *buf, double *val, char **un
> > {
> > errno = 0;
> > *val = strtod(buf, unit);
> > - if (errno == ERANGE || *val < 0 || *val > ULLONG_MAX)
> > + if (errno == ERANGE || *val < 0 || *val >= (double)ULLONG_MAX)
> > return (-1);/* too big/small */
> > if (*val == 0 && *unit == buf)
> > return (-1);/* No conversion performed. */
> >
> > Ali Farzanrad wrote:
> >> Hi tech@,
> >>
> >> disklabel shows a warning at build time which might be important.
> >> Following diff will surpass the warning.
> >>
> >> ===
> >> RCS file: /cvs/src/sbin/disklabel/editor.c,v
> >> retrieving revision 1.368
> >> diff -u -p -r1.368 editor.c
> >> --- editor.c 2021/05/30 19:02:30 1.368
> >> +++ editor.c 2021/06/23 19:25:45
> >> @@ -2418,7 +2418,7 @@ apply_unit(double val, u_char unit, u_int64_t *n)
> >>}
> >>
> >>val *= factor / DEV_BSIZE;
> >> - if (val > ULLONG_MAX)
> >> + if (val != (double)(u_int64_t)val)
> >>return (-1);
> >>*n = val;
> >>return (0);
> >> @@ -2429,7 +2429,7 @@ parse_sizespec(const char *buf, double *val, char
> >> **un
> >> {
> >>errno = 0;
> >>*val = strtod(buf, unit);
> >> - if (errno == ERANGE || *val < 0 || *val > ULLONG_MAX)
> >> + if (errno == ERANGE || *val < 0 || *val != (double)(u_int64_t)*val)
> >>return (-1);/* too big/small */
> >>if (*val == 0 && *unit == buf)
> >>return (-1);/* No conversion performed. */
> >>
> >>
> >
>
>
Re: Bad comparison between double and uint64_t
Oh, my bad, sorry.
I assumed that val is always integer.
I guess it is better to ignore val == ULLONG_MAX:
===
RCS file: /cvs/src/sbin/disklabel/editor.c,v
retrieving revision 1.368
diff -u -p -r1.368 editor.c
--- editor.c2021/05/30 19:02:30 1.368
+++ editor.c2021/06/23 20:23:03
@@ -2418,7 +2418,7 @@ apply_unit(double val, u_char unit, u_int64_t *n)
}
val *= factor / DEV_BSIZE;
- if (val > ULLONG_MAX)
+ if (val >= (double)ULLONG_MAX)
return (-1);
*n = val;
return (0);
@@ -2429,7 +2429,7 @@ parse_sizespec(const char *buf, double *val, char **un
{
errno = 0;
*val = strtod(buf, unit);
- if (errno == ERANGE || *val < 0 || *val > ULLONG_MAX)
+ if (errno == ERANGE || *val < 0 || *val >= (double)ULLONG_MAX)
return (-1);/* too big/small */
if (*val == 0 && *unit == buf)
return (-1);/* No conversion performed. */
Ali Farzanrad wrote:
> Hi tech@,
>
> disklabel shows a warning at build time which might be important.
> Following diff will surpass the warning.
>
> ===
> RCS file: /cvs/src/sbin/disklabel/editor.c,v
> retrieving revision 1.368
> diff -u -p -r1.368 editor.c
> --- editor.c 2021/05/30 19:02:30 1.368
> +++ editor.c 2021/06/23 19:25:45
> @@ -2418,7 +2418,7 @@ apply_unit(double val, u_char unit, u_int64_t *n)
> }
>
> val *= factor / DEV_BSIZE;
> - if (val > ULLONG_MAX)
> + if (val != (double)(u_int64_t)val)
> return (-1);
> *n = val;
> return (0);
> @@ -2429,7 +2429,7 @@ parse_sizespec(const char *buf, double *val, char **un
> {
> errno = 0;
> *val = strtod(buf, unit);
> - if (errno == ERANGE || *val < 0 || *val > ULLONG_MAX)
> + if (errno == ERANGE || *val < 0 || *val != (double)(u_int64_t)*val)
> return (-1);/* too big/small */
> if (*val == 0 && *unit == buf)
> return (-1);/* No conversion performed. */
>
>
Bad comparison between double and uint64_t
Hi tech@,
disklabel shows a warning at build time which might be important.
Following diff will surpass the warning.
===
RCS file: /cvs/src/sbin/disklabel/editor.c,v
retrieving revision 1.368
diff -u -p -r1.368 editor.c
--- editor.c2021/05/30 19:02:30 1.368
+++ editor.c2021/06/23 19:25:45
@@ -2418,7 +2418,7 @@ apply_unit(double val, u_char unit, u_int64_t *n)
}
val *= factor / DEV_BSIZE;
- if (val > ULLONG_MAX)
+ if (val != (double)(u_int64_t)val)
return (-1);
*n = val;
return (0);
@@ -2429,7 +2429,7 @@ parse_sizespec(const char *buf, double *val, char **un
{
errno = 0;
*val = strtod(buf, unit);
- if (errno == ERANGE || *val < 0 || *val > ULLONG_MAX)
+ if (errno == ERANGE || *val < 0 || *val != (double)(u_int64_t)*val)
return (-1);/* too big/small */
if (*val == 0 && *unit == buf)
return (-1);/* No conversion performed. */
Re: Undefined Behavior at jsmn.c
Florian Obser wrote:
> On Sun, Jul 12, 2020 at 09:10:57AM +0200, Otto Moerbeek wrote:
> > On Sun, Jul 12, 2020 at 09:57:02AM +0430, Ali Farzanrad wrote:
> >
> > > Hi @tech,
> > >
> > > I was comparing jsmn.c in acme-client with jsmn.c in FreeBSD [1].
> > > I found a switch without a default case which is an undefined behavior:
> > >
> > > @@ -69,6 +69,8 @@
> > > case '\t' : case '\r' : case '\n' : case ' ' :
> > > case ',' : case ']' : case '}' :
> > > goto found;
> > > + default:
> > > + break;
> > > }
> > > if (js[parser->pos] < 32 || js[parser->pos] >= 127) {
> > > parser->pos = start;
> > >
> > > I have patched that undefined behavior + some style fix.
> >
> > It is bad practise to intermix style changes with bug fixes.
> > Please post the fix seperately.
>
> It's also not undefined behaviour.
> ISO/IEC 9899:1999 6.8.4.2:
>
> If no converted case constant expression matches and there is no
> default label, no part of the switch body is executed.
My bad, sorry.
> Not having a default case might be considered bad style in itself
> though.
I also compared it to latest jsmn code, and found out that they have
fixed few other things, like preventing array / object as a key in
another object.
Is it OK to sync in-base jsmn with upstream? It seems harmless (only
stricter + few style).
(I've attached a single diff, because it is simple sync)
cvs diff: Diffing usr.sbin/acme-client
Index: usr.sbin/acme-client/jsmn.c
===
RCS file: /cvs/src/usr.sbin/acme-client/jsmn.c,v
retrieving revision 1.1
diff -u -p -r1.1 jsmn.c
--- usr.sbin/acme-client/jsmn.c 31 Aug 2016 22:01:42 - 1.1
+++ usr.sbin/acme-client/jsmn.c 12 Jul 2020 14:20:06 -
@@ -1,31 +1,34 @@
/*
- Copyright (c) 2010 Serge A. Zaitsev
-
- Permission is hereby granted, free of charge, to any person obtaining a copy
- of this software and associated documentation files (the "Software"), to deal
- in the Software without restriction, including without limitation the rights
- to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- copies of the Software, and to permit persons to whom the Software is
- furnished to do so, subject to the following conditions:
-
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
-
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- THE SOFTWARE.*
+ * MIT License
+ *
+ * Copyright (c) 2010 Serge Zaitsev
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
THE
+ * SOFTWARE.
*/
+
#include "jsmn.h"
/**
- * Allocates a fresh unused token from the token pull.
+ * Allocates a fresh unused token from the token pool.
*/
static jsmntok_t *jsmn_alloc_token(jsmn_parser *parser,
- jsmntok_t *tokens, size_t num_tokens) {
+ jsmntok_t *tokens, const size_t num_tokens) {
jsmntok_t *tok;
if (parser->toknext >= num_tokens) {
return NULL;
@@ -42,8 +45,8 @@ static jsmntok_t *jsmn_alloc_token(jsmn_
/**
* Fills token type and boundaries.
*/
-static void jsmn_f
Undefined Behavior at jsmn.c
Hi @tech,
I was comparing jsmn.c in acme-client with jsmn.c in FreeBSD [1].
I found a switch without a default case which is an undefined behavior:
@@ -69,6 +69,8 @@
case '\t' : case '\r' : case '\n' : case ' ' :
case ',' : case ']' : case '}' :
goto found;
+ default:
+ break;
}
if (js[parser->pos] < 32 || js[parser->pos] >= 127) {
parser->pos = start;
I have patched that undefined behavior + some style fix.
[1] https://svnweb.freebsd.org/base/head/lib/libpmc/pmu-events/jsmn.c
Index: jsmn.c
===
RCS file: /cvs/src/usr.sbin/acme-client/jsmn.c,v
retrieving revision 1.1
diff -u -p -r1.1 jsmn.c
--- jsmn.c 31 Aug 2016 22:01:42 - 1.1
+++ jsmn.c 12 Jul 2020 05:10:34 -
@@ -1,31 +1,33 @@
/*
- Copyright (c) 2010 Serge A. Zaitsev
-
- Permission is hereby granted, free of charge, to any person obtaining a copy
- of this software and associated documentation files (the "Software"), to deal
- in the Software without restriction, including without limitation the rights
- to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- copies of the Software, and to permit persons to whom the Software is
- furnished to do so, subject to the following conditions:
-
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
-
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- THE SOFTWARE.*
+ * Copyright (c) 2010 Serge A. Zaitsev
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
*/
+
#include "jsmn.h"
-/**
- * Allocates a fresh unused token from the token pull.
+/*
+ * Allocates a fresh unused token from the token pool.
*/
-static jsmntok_t *jsmn_alloc_token(jsmn_parser *parser,
- jsmntok_t *tokens, size_t num_tokens) {
+static jsmntok_t *
+jsmn_alloc_token(jsmn_parser *parser, jsmntok_t *tokens, size_t num_tokens)
+{
jsmntok_t *tok;
if (parser->toknext >= num_tokens) {
return NULL;
@@ -39,22 +41,25 @@ static jsmntok_t *jsmn_alloc_token(jsmn_
return tok;
}
-/**
+/*
* Fills token type and boundaries.
*/
-static void jsmn_fill_token(jsmntok_t *token, jsmntype_t type,
-int start, int end) {
+static void
+jsmn_fill_token(jsmntok_t *token, jsmntype_t type, int start, int end)
+{
token->type = type;
token->start = start;
token->end = end;
token->size = 0;
}
-/**
+/*
* Fills next available token with JSON primitive.
*/
-static int jsmn_parse_primitive(jsmn_parser *parser, const char *js,
- size_t len, jsmntok_t *tokens, size_t num_tokens) {
+static int
+jsmn_parse_primitive(jsmn_parser *parser, const char *js,
+size_t len, jsmntok_t *tokens, size_t num_tokens)
+{
jsmntok_t *token;
int start;
@@ -63,12 +68,19 @@ static int jsmn_parse_primitive(jsmn_par
for (; parser->pos < len && js[parser->pos] != '\0'; parser->pos++) {
switch (js[parser->pos]) {
#ifndef JSMN_STRICT
- /* In strict mode primitive must be followed by "," or
"}" or "]" */
- case ':':
+ /* In strict mode primitive must be followed by "," or "}" or
"]" */
+ case ':':
#endif
- case '\t' : case
pkg_info: recommend -aQ over -Q
Hi,
Today I was searching for postgresql-server package using pkg_info,
but I didn't found that (OpenBSD-6.7-STABLE)!
$ pkg_info -Q postgres
dovecot-postgresql-2.3.10.1v0
After a bit of research I found the problem in this file:
/usr/libdata/perl5/OpenBSD/PackageRepositoryList.pm:
...
sub match_locations
{
my ($self, @search) = @_;
my $result = [];
for my $repo (@{$self->{l}}) {
my $l = $repo->match_locations(@search);
if ($search[0]->{keep_all}) {
push(@$result, @$l);
} elsif (@$l > 0) {
return $l;
}
}
return $result;
}
which searches for matched packages in packages-stable before packages
url; so I figured out that I should use -aQ options:
$ pkg_info -aQ postgres
check_postgres-2.25.0
debug-qt5-postgresql-5.13.2
dovecot-postgresql-2.3.10.1v0
dovecot-postgresql-2.3.10v0
kamailio-postgresql-5.0.6p0
orthanc-plugin-postgresql-2.0p2
postgresql-client-12.2
postgresql-contrib-12.2
postgresql-docs-12.2
postgresql-odbc-10.02.p0
postgresql-pg_upgrade-12.2
postgresql-pllua-2.0.4
postgresql-plpython-12.2
postgresql-plr-8.4
postgresql-previous-11.6
postgresql-server-12.2
postgresql_autodoc-1.40p1
qt3-postgresql-3.8p12
qt4-postgresql-4.8.7p7
qt5-postgresql-5.13.2
sope-postgres-4.3.0
tdbc-postgres-1.0.6
Anyway, I recommend following patch for www/faq/faq15.html:
cvs diff: Diffing .
Index: faq15.html
===
RCS file: /cvs/www/faq/faq15.html,v
retrieving revision 1.184
diff -u -p -r1.184 faq15.html
--- faq15.html 14 Aug 2019 13:07:46 - 1.184
+++ faq15.html 16 Jun 2020 19:29:53 -
@@ -148,10 +148,10 @@ To search for any given package name, us
https://man.openbsd.org/pkg_info;>pkg_info(1).
-$ pkg_info -Q unzip
-lunzip-1.8
-unzip-6.0p9
-unzip-6.0p9-iconv
+$ pkg_info -aQ unzip
+lunzip-1.11
+unzip-6.0p13
+unzip-6.0p13-iconv
Another way to find what you're looking for is with the pkglocate
Re: delete ligature support for Arabic "la" from the less(1) command line
Hi Ingo,
Thanks for your effort in unicode support. I hope my feedback as a
native Persian would be helpful.
Ingo Schwarze wrote:
> If i understand correctly, xterm(1) does indeed have that problem.
> I prepared a test file that contains, in this order,
>
> - some Latin characters
> - the Arabic word "la" ("no"), i.e. first LAM, then ALEF
> - some more Latin characters
> - the Arabic word "al" ("the"), i.e. first ALEF, then LAM
> - some final Latin characters
>
> And indeed, xterm(1) does not respect the writing direction of the
> individual words. When cat(1)'ing the file to stdout, both xterm(1)
> and konsole(1) show all the words from left to right, but *inside*
> each word, konsole(1) uses the correct writing direction: right to
> left for Arabic and left to right for Latin. For example, in the
> Arabic word "al", konsole(1) correctly shows the ALEF right of the
> LAM, whereas xterm(1) wrongly shows the ALEF left of the LAM.
>
There are many rules. Each letter / character has a direction by
itself. For example English letters are LTR (left-to-right), Arabic /
Persian letters are RTL, but some characters, say symbols, have no
direction. For example, when you write:
'A' '+' 'B'
It should be displayed as is ('+' is LTR), but when you write:
'A' ALEF '+' LAM 'B'
The '+' should be displayed in the left side of ALEF ('+' is RTL):
'A' LAM '+' ALEF 'B'
I think you need to detect all maximal non-LTR substrings (which don't
start or end with a symbol) inside LTR strings to render them correctly.
There are also RTL / LTR control characters in Unicode which manipulate
this behaviour.
> I'm not entirely sure this has much to do with ligatures, though.
> What matters for building ligatures is only the logical ordering,
> the ordering in *time* so to speak, i.e. what comes before and what
> comes after. LAM before ALEF has to become the ligature glyph "al",
> whereas ALEF before LAM remains two glyphs. Technically, the
> question of ordering in space, whether glyphs are painted onto the
> screen right to left or left to right, only comes into play after
> characters have already been combined into glyphs.
>
> Actually, now that you bring up the topic, i see another situation
> where less(1) causes an issue. Let's use konsole(1) and not xterm(1)
> such that we get the correct writing direction, and let's put the
> word "al" onto the screen. No ligature here, so that part of the
> topic is suspended for a moment. Now let's slowly scroll right in
> one-column steps. All is fine as long as the word "al" is completely
> visible on screen. But when the final letter LAM of "al" is in the
> last (leftmost) column of the screen and you scroll right one more
> column, something weird happens, even in konsole(1). You would
> expect the final letter LAM to scroll off screen first and the initial
> letter ALEF to remain on the screen for a little longer. Instead,
> less(1) incorrectly thinks the *initial* letter of the word scrolls
> off screen first, and it tells xterm(1) to display the ALEF in the
> leftmost column of the screen while the LAM just went off-screen.
> That looks weird because there is no word in that text beginning
> with ALEF.
>
It's a difficult problem. You need to consider all maximal non-LTR
substrings, and all LTR / RTL modifiers. Also consider a file with long
RTL lines; user prefer to see the beginig of lines (in all languages,
readers read from start), so less(1) should display right-most part of
each line, and when user scrolls the text to right, less(1) should
display left-side of each line.
I think that if xterm had a complete RTL mode with swapped right and
left keys, it might solve many problems. In your example in RTL xterm,
there will be no right scroll (because of swapped keys) and when you
scroll less(1) to the left, less(1) will correctly scrolls off the
initial letter. Of course it will not work on complex mixed RTL / LTR
texts, but it solves the problem in most common situations.
> This means that being able to properly view Arabic or Farsi text
> with the default OpenBSD terminal emulator and parser would require
>
> 1. bidi support in xterm(1)
> to render Farsi words with the correct writing direction
> 2. ligature support in xterm(1)
> to correctly connect letters
> 3. bidi support in less(1)
> to correctly scroll parts of words on and off screen, horizontally
According to previous example (a file with long RTL lines), I don't
agree with bidi support in less(1).
> 4. ligature support in less(1)
> for correct columnation
>
> As far as i understand, you are saying that the extremely fragmentary
> support for item 4 which we happen to have right now is not really
> useful without items 1-3, and even when using konsole(1), which does
> have items 1 and 2, implementing item 3 before item 4 would make
> sense because item 3 is more importrant.
>
> So my understanding is that you are not objecting to the patch because
Re: src/usr.sbin/slowcgi: possible bug
Florian Obser wrotes:
>On Mon, Jan 02, 2017 at 04:29:21PM +0330, temp+...@frad.ir wrote:
>> Hi tech@,
>>
>> I recently checked the slowcgi(8) and found that it might have an issue
>> when buf_pos is at the end of buffer and buf_len is zero.
>>
>> Am I right?
>
>we can simplify this even more. There is no need to remember the
>buffer position outside of this function. It will be 0 on every call,
>either because we made progress in parsing data and then copied the
>rest to the beginning of the buffer or we did not make progress at
>all, and then we need to start parsing from the beginning again.
>
>OK?
It seems perfect.
>
>diff --git slowcgi.c slowcgi.c
>index dec4df8d1a1..83d12d99160 100644
>--- slowcgi.c
>+++ slowcgi.c
>@@ -118,7 +118,6 @@ struct request {
> struct eventtmo;
> int fd;
> uint8_t buf[FCGI_RECORD_SIZE];
>- size_t buf_pos;
> size_t buf_len;
> struct fcgi_response_head response_head;
> struct fcgi_stdin_head stdin_head;
>@@ -495,7 +494,6 @@ slowcgi_accept(int fd, short events, void *arg)
> return;
> }
> c->fd = s;
>- c->buf_pos = 0;
> c->buf_len = 0;
> c->request_started = 0;
> c->stdin_fd_closed = c->stdout_fd_closed = c->stderr_fd_closed = 0;
>@@ -632,12 +630,12 @@ slowcgi_request(int fd, short events, void *arg)
> {
> struct request *c;
> ssize_t n;
>- size_t parsed;
>+ size_t parsed, pos = 0;
>
> c = arg;
>
>- n = read(fd, c->buf + c->buf_pos + c->buf_len,
>- FCGI_RECORD_SIZE - c->buf_pos-c->buf_len);
>+ n = read(fd, c->buf + c->buf_len,
>+ FCGI_RECORD_SIZE - c->buf_len);
>
> switch (n) {
> case -1:
>@@ -666,16 +664,15 @@ slowcgi_request(int fd, short events, void *arg)
>* at that point, which is what happens here.
>*/
> do {
>- parsed = parse_record(c->buf + c->buf_pos, c->buf_len, c);
>- c->buf_pos += parsed;
>+ parsed = parse_record(c->buf + pos, c->buf_len, c);
>+ pos += parsed;
> c->buf_len -= parsed;
> } while (parsed > 0 && c->buf_len > 0);
>
> /* Make space for further reads */
>- if (c->buf_len > 0) {
>- bcopy(c->buf + c->buf_pos, c->buf, c->buf_len);
>- c->buf_pos = 0;
>- }
>+ if (c->buf_len > 0 && pos > 0)
>+ bcopy(c->buf + pos, c->buf, c->buf_len);
>+
> return;
> fail:
> cleanup_request(c);
>
>
Re: src/usr.sbin/slowcgi: possible bug
>I recently checked the slowcgi(8) and found that it might have an issue
>when buf_pos is at the end of buffer and buf_len is zero.
>
>Am I right?
It seems that all fastcgi blocks are aligned in 8-bytes and buffer size
is 8+65535+255 = 65798 bytes which is not aligned in 8-bytes. It seems
that slowcgi has no problem with aligned data, but I think in general
slowcgi should not assume that all blocks are aligned in 8-bytes.
>Index: slowcgi.c
>===
>RCS file: /cvs/src/usr.sbin/slowcgi/slowcgi.c,v
>retrieving revision 1.50
>diff -u -p -r1.50 slowcgi.c
>--- slowcgi.c 4 Sep 2016 14:40:34 - 1.50
>+++ slowcgi.c 2 Jan 2017 12:52:01 -
>@@ -674,8 +674,8 @@ slowcgi_request(int fd, short events, vo
> /* Make space for further reads */
> if (c->buf_len > 0) {
> bcopy(c->buf + c->buf_pos, c->buf, c->buf_len);
>- c->buf_pos = 0;
> }
>+ c->buf_pos = 0;
> return;
> fail:
> cleanup_request(c);
>
>
