Re: __read_mostly
Ingo Schwarze wrote: > Hi, > > >> A downside of this is that it becomes easier to guess the addresses > >> of the tagged variables. > > > No kidding. It partly undoes the effort of KARL. > > I don't feel qualified to comment on the patch, > but i can't resist mentioning that i still love > tedu@'s classical dictum "attack mitigation countermeasures" > which he coined during the aftermath of the heartbleed debacle. true, true. That said, it is a small setset of objects. The small set of objects will still be randomly ordered within that region. As the number of objects increases, it becomes harder for an attacker to guess the relative-fixed offset from one object (assuming they get a pointer) to specific other object. So perhaps this is still acceptable?
Re: fix very small ntpd leak
On Thu, Mar 24, 2022 at 10:34:52AM +1000, Jonathan Matthew wrote:
> On Wed, Mar 23, 2022 at 04:59:06PM +0100, Otto Moerbeek wrote:
> > On Wed, Mar 23, 2022 at 09:09:01PM +1000, Jonathan Matthew wrote:
> >
> > > We noticed that the ntpd engine process was getting a bit big on some
> > > boxes
> > > that we'd accidentally cut off from the ntp servers (routing is hard).
> > > Reading through the code, I noticed the 'query' member of struct ntp_peer
> > > is never freed, which seems to account for the leak.
> > >
> > > If you have a server pool in ntpd.conf and it resolves, but ntpd is unable
> > > to talk to the servers, it will re-resolve periodically, freeing the old
> > > list
> > > of peers and creating new ones.
> > >
> > > To show how slow the leak is, here's the leak report from MALLOC_OPTIONS=D
> > > after running for about two hours with four servers from two pools.
> > >
> > > without diff:
> > >
> > > Leak report
> > > f sum #avg
> > >0x09392128 73
> > > 0x889878b920b 512 1512
> > > 0x889878bc8e14096 4 1024
> > > 0x889878bd065 128 2 64
> > > 0x88bc91f0b4b 18280 1 18280
> > > 0x88bc926a9ed 65536 1 65536
> > >
> > >
> > > with diff:
> > >
> > > Leak report
> > > f sum #avg
> > >0x06064 16379
> > > 0xbee1253320b 512 1512
> > > 0xbf0265f4b4b 18280 1 18280
> > > 0xbf02666e9ed 65536 1 65536
> > >
> > > ok?
> > >
> > > Index: ntp.c
> > > ===
> > > RCS file: /cvs/src/usr.sbin/ntpd/ntp.c,v
> > > retrieving revision 1.168
> > > diff -u -p -r1.168 ntp.c
> > > --- ntp.c 24 Oct 2021 21:24:19 - 1.168
> > > +++ ntp.c 23 Mar 2022 10:43:59 -
> > > @@ -686,6 +686,7 @@ void
> > > peer_remove(struct ntp_peer *p)
> > > {
> > > TAILQ_REMOVE(>ntp_peers, p, entry);
> > > + free(p->query);
> > > free(p);
> > > peer_cnt--;
> > > }
> > >
> >
> > This is a bug that dlg reported last week. Serendepity or not? :-)
>
> We found it together looking at systems we run at work, so not really.
heh, dlg was talking about a collegue but i did not connnect that to you,
anyway, thanks for the analysis,
-Otto
>
> >
> > This is my diff that uses an approach I like a litle bit better.
>
> I agree. I wasn't sure if there was a reason the query was allocated
> separately, so I went with the more straightforward diff to start with.
>
> >
> > -Otto
> >
> > Index: client.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ntpd/client.c,v
> > retrieving revision 1.116
> > diff -u -p -r1.116 client.c
> > --- client.c21 Apr 2021 09:38:11 - 1.116
> > +++ client.c21 Mar 2022 07:31:54 -
> > @@ -51,10 +51,9 @@ set_deadline(struct ntp_peer *p, time_t
> > int
> > client_peer_init(struct ntp_peer *p)
> > {
> > - if ((p->query = calloc(1, sizeof(struct ntp_query))) == NULL)
> > - fatal("client_peer_init calloc");
> > - p->query->fd = -1;
> > - p->query->msg.status = MODE_CLIENT | (NTP_VERSION << 3);
> > + p->query.fd = -1;
> > + p->query.msg.status = MODE_CLIENT | (NTP_VERSION << 3);
> > + p->query.xmttime = 0;
> > p->state = STATE_NONE;
> > p->shift = 0;
> > p->trustlevel = TRUSTLEVEL_PATHETIC;
> > @@ -91,7 +90,7 @@ client_addr_init(struct ntp_peer *p)
> > }
> > }
> >
> > - p->query->fd = -1;
> > + p->query.fd = -1;
> > set_next(p, 0);
> >
> > return (0);
> > @@ -100,9 +99,9 @@ client_addr_init(struct ntp_peer *p)
> > int
> > client_nextaddr(struct ntp_peer *p)
> > {
> > - if (p->query->fd != -1) {
> > - close(p->query->fd);
> > - p->query->fd = -1;
> > + if (p->query.fd != -1) {
> > + close(p->query.fd);
> > + p->query.fd = -1;
> > }
> >
> > if (p->state == STATE_DNS_INPROGRESS)
> > @@ -148,26 +147,26 @@ client_query(struct ntp_peer *p)
> > if (p->state < STATE_DNS_DONE || p->addr == NULL)
> > return (-1);
> >
> > - if (p->query->fd == -1) {
> > + if (p->query.fd == -1) {
> > struct sockaddr *sa = (struct sockaddr *)>addr->ss;
> > struct sockaddr *qa4 = (struct sockaddr *)>query_addr4;
> > struct sockaddr *qa6 = (struct sockaddr *)>query_addr6;
> >
> > - if ((p->query->fd = socket(p->addr->ss.ss_family, SOCK_DGRAM,
> > + if ((p->query.fd = socket(p->addr->ss.ss_family, SOCK_DGRAM,
> > 0)) == -1)
> > fatal("client_query socket");
> >
> > if (p->addr->ss.ss_family == qa4->sa_family) {
> > - if (bind(p->query->fd, qa4, SA_LEN(qa4)) == -1)
> > + if (bind(p->query.fd, qa4, SA_LEN(qa4)) == -1)
> >
Re: sndio: add sio_flush() function
On Thu, Mar 24, 2022 at 07:11:42AM +0100, Alexandre Ratchov wrote:
> Most audio/video players do a stop/start cycle whenever the play
> position is changed, track is changed, etc. Currently, stopping drains
> the play buffer, which by default is very large (to workaround very
> long kernel non-preemptive code-paths). This makes player controls
> sluggish.
>
> This diff adds a new sio_flush() function to workaround the jumbo
> buffer sizes: it stops playback immediately, discarding buffered
> data. Basically it's the same as sio_stop() but doesn't wait. The plan
> is to make players use it.
>
> In the network protocol, sio_flush() is implemented by adding a flag
> to the message corresponding to sio_stop(). Old sndiod servers ignore
> it and just work with new libraries. New sndiod servers see that the
> flag is not set by old libraries and properly drain the play buffer.
>
> Tested with mplayer, mpv and audacious, if we go this way other ports
> will follow.
>
Here's the diff to make mplayer, mpv and audacious use the new
function.
As you see, sio_stop() and sio_flush() are perfectly
interchangable. The only difference is the time the function takes to
complete (and the resulting silence).
Index: x11/mplayer/Makefile
===
RCS file: /cvs/ports/x11/mplayer/Makefile,v
retrieving revision 1.319
diff -u -p -r1.319 Makefile
--- x11/mplayer/Makefile11 Mar 2022 20:16:48 - 1.319
+++ x11/mplayer/Makefile23 Mar 2022 19:49:08 -
@@ -3,7 +3,7 @@ COMMENT=movie player supporting many fo
V= 20211106
FFMPEG_V= 4.4.1
DISTNAME= mplayer-${V}
-REVISION= 0
+REVISION= 1
CATEGORIES=x11 multimedia
MASTER_SITES= https://comstyle.com/source/
EXTRACT_SUFX= .tar.xz
Index: x11/mplayer/patches/patch-libao2_ao_sndio_c
===
RCS file: x11/mplayer/patches/patch-libao2_ao_sndio_c
diff -N x11/mplayer/patches/patch-libao2_ao_sndio_c
--- /dev/null 1 Jan 1970 00:00:00 -
+++ x11/mplayer/patches/patch-libao2_ao_sndio_c 23 Mar 2022 19:49:08 -
@@ -0,0 +1,21 @@
+Index: libao2/ao_sndio.c
+--- libao2/ao_sndio.c.orig
libao2/ao_sndio.c
+@@ -179,7 +179,7 @@ static void uninit(int immed)
+ */
+ static void reset(void)
+ {
+-if (!sio_stop(hdl))
++if (!sio_flush(hdl))
+ mp_msg(MSGT_AO, MSGL_ERR, "ao2: reset: couldn't stop\n");
+ delay = 0;
+ if (!sio_start(hdl))
+@@ -235,7 +235,7 @@ static void audio_pause(void)
+ * sndio can't pause, so just stop
+ */
+ prepause_delay = delay;
+-if (!sio_stop(hdl))
++if (!sio_flush(hdl))
+ mp_msg(MSGT_AO, MSGL_ERR, "ao2: pause: couldn't stop\n");
+ delay = 0;
+ }
Index: multimedia/mpv/Makefile
===
RCS file: /cvs/ports/multimedia/mpv/Makefile,v
retrieving revision 1.81
diff -u -p -r1.81 Makefile
--- multimedia/mpv/Makefile 11 Mar 2022 19:39:24 - 1.81
+++ multimedia/mpv/Makefile 23 Mar 2022 19:49:08 -
@@ -4,7 +4,7 @@ GH_ACCOUNT =mpv-player
GH_PROJECT = mpv
GH_TAGNAME = v0.34.1
-REVISION = 0
+REVISION = 1
SHARED_LIBS += mpv 0.2 # 1.109
Index: multimedia/mpv/patches/patch-audio_out_ao_sndio_c
===
RCS file: /cvs/ports/multimedia/mpv/patches/patch-audio_out_ao_sndio_c,v
retrieving revision 1.6
diff -u -p -r1.6 patch-audio_out_ao_sndio_c
--- multimedia/mpv/patches/patch-audio_out_ao_sndio_c 11 Mar 2022 19:39:24
- 1.6
+++ multimedia/mpv/patches/patch-audio_out_ao_sndio_c 23 Mar 2022 19:49:09
-
@@ -245,8 +245,8 @@ Index: audio/out/ao_sndio.c
+if (p->playing) {
+p->playing = false;
+
-+if (!sio_stop(p->hdl)) {
-+MP_ERR(ao, "reset: couldn't sio_stop()\n");
++if (!sio_flush(p->hdl)) {
++MP_ERR(ao, "reset: couldn't sio_flush()\n");
+}
+p->delay = 0;
+if (!sio_start(p->hdl)) {
Index: audio/audacious/plugins/Makefile
===
RCS file: /cvs/ports/audio/audacious/plugins/Makefile,v
retrieving revision 1.10
diff -u -p -r1.10 Makefile
--- audio/audacious/plugins/Makefile8 Mar 2022 08:53:50 - 1.10
+++ audio/audacious/plugins/Makefile23 Mar 2022 19:49:09 -
@@ -1,7 +1,7 @@
COMMENT = input and output plugins for Audacious
DISTNAME = audacious-plugins-${VERSION}
-REVISION = 2
+REVISION = 3
# BSD / GPL
PERMIT_PACKAGE = Yes
Index: audio/audacious/plugins/patches/patch-src_sndio_sndio_cc
===
RCS file: audio/audacious/plugins/patches/patch-src_sndio_sndio_cc
diff -N audio/audacious/plugins/patches/patch-src_sndio_sndio_cc
--- /dev/null
sndio: add sio_flush() function
Most audio/video players do a stop/start cycle whenever the play
position is changed, track is changed, etc. Currently, stopping drains
the play buffer, which by default is very large (to workaround very
long kernel non-preemptive code-paths). This makes player controls
sluggish.
This diff adds a new sio_flush() function to workaround the jumbo
buffer sizes: it stops playback immediately, discarding buffered
data. Basically it's the same as sio_stop() but doesn't wait. The plan
is to make players use it.
In the network protocol, sio_flush() is implemented by adding a flag
to the message corresponding to sio_stop(). Old sndiod servers ignore
it and just work with new libraries. New sndiod servers see that the
flag is not set by old libraries and properly drain the play buffer.
Tested with mplayer, mpv and audacious, if we go this way other ports
will follow.
ok?
Index: include/sndio.h
===
RCS file: /cvs/src/include/sndio.h,v
retrieving revision 1.13
diff -u -p -r1.13 sndio.h
--- include/sndio.h 28 Jun 2020 05:21:38 - 1.13
+++ include/sndio.h 23 Mar 2022 20:23:29 -
@@ -164,6 +164,7 @@ size_t sio_write(struct sio_hdl *, const
size_t sio_read(struct sio_hdl *, void *, size_t);
int sio_start(struct sio_hdl *);
int sio_stop(struct sio_hdl *);
+int sio_flush(struct sio_hdl *);
int sio_nfds(struct sio_hdl *);
int sio_pollfd(struct sio_hdl *, struct pollfd *, int);
int sio_revents(struct sio_hdl *, struct pollfd *);
Index: lib/libsndio/Symbols.map
===
RCS file: /cvs/src/lib/libsndio/Symbols.map,v
retrieving revision 1.2
diff -u -p -r1.2 Symbols.map
--- lib/libsndio/Symbols.map26 Feb 2020 13:53:58 - 1.2
+++ lib/libsndio/Symbols.map23 Mar 2022 20:23:29 -
@@ -11,6 +11,7 @@
sio_read;
sio_start;
sio_stop;
+ sio_flush;
sio_nfds;
sio_pollfd;
sio_revents;
Index: lib/libsndio/amsg.h
===
RCS file: /cvs/src/lib/libsndio/amsg.h,v
retrieving revision 1.14
diff -u -p -r1.14 amsg.h
--- lib/libsndio/amsg.h 1 Nov 2021 14:43:24 - 1.14
+++ lib/libsndio/amsg.h 23 Mar 2022 20:23:29 -
@@ -96,6 +96,9 @@ struct amsg {
#define AMSG_DATAMAX 0x1000
uint32_t size;
} data;
+ struct amsg_stop {
+ uint8_t drain;
+ } stop;
struct amsg_ts {
int32_t delta;
} ts;
Index: lib/libsndio/shlib_version
===
RCS file: /cvs/src/lib/libsndio/shlib_version,v
retrieving revision 1.12
diff -u -p -r1.12 shlib_version
--- lib/libsndio/shlib_version 26 Feb 2020 13:53:58 - 1.12
+++ lib/libsndio/shlib_version 23 Mar 2022 20:23:29 -
@@ -1,2 +1,2 @@
major=7
-minor=1
+minor=2
Index: lib/libsndio/sio.c
===
RCS file: /cvs/src/lib/libsndio/sio.c,v
retrieving revision 1.26
diff -u -p -r1.26 sio.c
--- lib/libsndio/sio.c 1 Nov 2021 14:43:24 - 1.26
+++ lib/libsndio/sio.c 23 Mar 2022 20:23:29 -
@@ -129,6 +129,8 @@ sio_start(struct sio_hdl *hdl)
int
sio_stop(struct sio_hdl *hdl)
{
+ if (hdl->ops->stop == NULL)
+ return sio_flush(hdl);
if (hdl->eof) {
DPRINTF("sio_stop: eof\n");
return 0;
@@ -139,6 +141,28 @@ sio_stop(struct sio_hdl *hdl)
return 0;
}
if (!hdl->ops->stop(hdl))
+ return 0;
+#ifdef DEBUG
+ DPRINTFN(2, "libsndio: polls: %llu, samples = %llu\n",
+ hdl->pollcnt, hdl->cpos);
+#endif
+ hdl->started = 0;
+ return 1;
+}
+
+int
+sio_flush(struct sio_hdl *hdl)
+{
+ if (hdl->eof) {
+ DPRINTF("sio_flush: eof\n");
+ return 0;
+ }
+ if (!hdl->started) {
+ DPRINTF("sio_flush: not started\n");
+ hdl->eof = 1;
+ return 0;
+ }
+ if (!hdl->ops->flush(hdl))
return 0;
#ifdef DEBUG
DPRINTFN(2, "libsndio: polls: %llu, samples = %llu\n",
Index: lib/libsndio/sio_aucat.c
===
RCS file: /cvs/src/lib/libsndio/sio_aucat.c,v
retrieving revision 1.20
diff -u -p -r1.20 sio_aucat.c
--- lib/libsndio/sio_aucat.c9 Jan 2016 08:27:24 - 1.20
+++ lib/libsndio/sio_aucat.c23 Mar 2022 20:23:29 -
@@ -49,6 +49,7 @@ struct sio_aucat_hdl {
static void sio_aucat_close(struct sio_hdl *);
static int sio_aucat_start(struct sio_hdl *);
static int sio_aucat_stop(struct sio_hdl *);
+static int sio_aucat_flush(struct sio_hdl *);
static int sio_aucat_setpar(struct sio_hdl *, struct sio_par *);
static int
