malloc: micro optimizations
Hi,
a few micro-optimization, including getting rid of some statistics
that are not actualy very interesting.
Speedup amounts to a few tenths of percents to a few percents,
depending on how biased the benchmark is.
-Otto
Index: stdlib/malloc.c
===
RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.291
diff -u -p -r1.291 malloc.c
--- stdlib/malloc.c 22 Oct 2023 12:19:26 - 1.291
+++ stdlib/malloc.c 24 Oct 2023 14:05:37 -
@@ -169,16 +169,12 @@ struct dir_info {
void *caller;
size_t inserts;
size_t insert_collisions;
- size_t finds;
- size_t find_collisions;
size_t deletes;
size_t delete_moves;
size_t cheap_realloc_tries;
size_t cheap_reallocs;
size_t malloc_used; /* bytes allocated */
size_t malloc_guarded; /* bytes used for guards */
- size_t pool_searches; /* searches for pool */
- size_t other_pool; /* searches in other pool */
#define STATS_ADD(x,y) ((x) += (y))
#define STATS_SUB(x,y) ((x) -= (y))
#define STATS_INC(x) ((x)++)
@@ -209,12 +205,14 @@ static void unmap(struct dir_info *d, vo
struct chunk_info {
LIST_ENTRY(chunk_info) entries;
void *page; /* pointer to the page */
+ /* number of shorts should add up to 8, check alloc_chunk_info() */
u_short canary;
u_short bucket;
u_short free; /* how many free chunks */
u_short total; /* how many chunks */
u_short offset; /* requested size table offset */
- u_short bits[1];/* which chunks are free */
+#define CHUNK_INFO_TAIL3
+ u_short bits[CHUNK_INFO_TAIL]; /* which chunks are free */
};
#define CHUNK_FREE(i, n) ((i)->bits[(n) / MALLOC_BITS] & (1U << ((n) %
MALLOC_BITS)))
@@ -656,12 +654,10 @@ find(struct dir_info *d, void *p)
index = hash(p) & mask;
r = d->r[index].p;
q = MASK_POINTER(r);
- STATS_INC(d->finds);
while (q != p && r != NULL) {
index = (index - 1) & mask;
r = d->r[index].p;
q = MASK_POINTER(r);
- STATS_INC(d->find_collisions);
}
return (q == p && r != NULL) ? >r[index] : NULL;
}
@@ -949,7 +945,7 @@ init_chunk_info(struct dir_info *d, stru
p->bucket = bucket;
p->total = p->free = MALLOC_PAGESIZE / B2ALLOC(bucket);
- p->offset = bucket == 0 ? 0xdead : howmany(p->total, MALLOC_BITS);
+ p->offset = howmany(p->total, MALLOC_BITS);
p->canary = (u_short)d->canary1;
/* set all valid bits in the bitmap */
@@ -971,8 +967,13 @@ alloc_chunk_info(struct dir_info *d, u_i
count = MALLOC_PAGESIZE / B2ALLOC(bucket);
size = howmany(count, MALLOC_BITS);
- size = sizeof(struct chunk_info) + (size - 1) * sizeof(u_short);
- if (mopts.chunk_canaries)
+ /* see declaration of struct chunk_info */
+ if (size <= CHUNK_INFO_TAIL)
+ size = 0;
+ else
+ size -= CHUNK_INFO_TAIL;
+ size = sizeof(struct chunk_info) + size * sizeof(u_short);
+ if (mopts.chunk_canaries && bucket > 0)
size += count * sizeof(u_short);
size = _ALIGN(size);
count = MALLOC_PAGESIZE / size;
@@ -1129,8 +1130,7 @@ fill_canary(char *ptr, size_t sz, size_t
static void *
malloc_bytes(struct dir_info *d, size_t size)
{
- u_int i, r, bucket, listnum;
- size_t k;
+ u_int i, k, r, bucket, listnum;
u_short *lp;
struct chunk_info *bp;
void *p;
@@ -1170,7 +1170,7 @@ malloc_bytes(struct dir_info *d, size_t
/* no bit halfway, go to next full short */
i /= MALLOC_BITS;
for (;;) {
- if (++i >= howmany(bp->total, MALLOC_BITS))
+ if (++i >= bp->offset)
i = 0;
lp = >bits[i];
if (*lp) {
@@ -1228,7 +1228,7 @@ validate_canary(struct dir_info *d, u_ch
}
}
-static uint32_t
+static inline uint32_t
find_chunknum(struct dir_info *d, struct chunk_info *info, void *ptr, int
check)
{
uint32_t chunknum;
@@ -1532,12 +1532,10 @@ findpool(void *p, struct dir_info *argpo
struct dir_info *pool = argpool;
struct region_info *r = find(pool, p);
- STATS_INC(pool->pool_searches);
if (r == NULL) {
u_int i, nmutexes;
nmutexes = mopts.malloc_pool[1]->malloc_mt ?
mopts.malloc_mutexes : 2;
- STATS_INC(pool->other_pool);
for (i = 1; i < nmutexes; i++) {
u_int j = (argpool->mutex + i) & (nmutexes - 1);
@@
Re: relayd does not delete control socket on shutdown
On Sat, Oct 21, 2023 at 10:40:45PM +0300, Kapetanakis Giannis wrote: > On 21/10/2023 20:39, Florian Obser wrote: > > Which was 8 years ago. I don't understand why you see a change in 7.4. > > > > Anyway, we decided to not clean up control sockets in any of our > > privsep daemons because leaving them behind does not cause any issues. > > I just noticed it today when I tried to use the socket in a script and > noticed that it stayed there even after shutdown and though it was after 7.4 > but I was wrong about that. > > Your commit made it that clear. > > Agree it's not a big case if it stays there. > > Would the unlink succeed if the socket was owned by _relayd? > > G Unlinking somthing requires write permissions to the directory it is in. -Otto
Re: Why store pointers for some functions in malloc.c?
On Wed, Oct 18, 2023 at 09:23:49AM +0900, Masato Asou wrote:
> Hello tech@ and otto,
>
> Why do only some calling functions store the pinttes in region_info as
> below:
>
> static void *
> malloc_bytes(struct dir_info *d, size_t size, void *f)
> {
>
> found:
> if (i == 0 && k == 0 && DO_STATS) {
> struct region_info *r = find(d, bp->page);
> STATS_SETF(r, f);
> }
>
> I found following mail from otto:
> https://marc.info/?l=openbsd-tech=168171382927798=2
> > The null "f" values (call sites) are due to the sampling nature of
> > small allocations. Recording all call sites of all potential leaks
> > introduces too much overhead.
>
> Is this the answer to my question?
> --
> ASOU Masato
Yes.
The reason is that (in the existing code) there's only one pointer per
region_info available to store callers. So for a chunk page (which has
many small alocations) ony slot 0 gets recorded.
But there's a diff I posted last week on tech@ that will change this
so that all call sites are recorded (in a different location and only
if D is used). It will also report more details when a write of a free
chunk is detected. That diff could use some review/testing.
-Otto
malloc: more info in error message for write-after-free with option D
Hi,
This diff adds better error reporting for write-after-free or the more
general write of free memory if malloc option D is active. Knowing the
place where allocations were done often helps to find out where the
overwrite happened.
If option D is active malloc now saves caller info in a separate page
instead of only doing that for chunk index 0. It also reports about
the preceding chunk if applicable. A report looks like this:
X(12489) in calloc(): write to free chunk 0x851ec6066d0[0..7]@16
allocated at /usr/X11R6/lib/modules/dri/radeonsi_dri.so 0x88c949
(preceding chunk 0x851ec6066c0 allocated at /usr/X11R6/bin/X 0x177374 (now
free))
You can use addr2line -e to get the file and linenumber the allocation
was done (if the object was compiled with debug info).
If D is not used only the first part is printed, as no caller info is
saved. So no extra overhead if D is not useed.
Also: the leak report now do not contain unknown locations anymore, as
each allocation gets its caller recorded if D is active.
-Otto
Index: stdlib/malloc.3
===
RCS file: /home/cvs/src/lib/libc/stdlib/malloc.3,v
retrieving revision 1.137
diff -u -p -r1.137 malloc.3
--- stdlib/malloc.3 1 Jul 2023 18:35:14 - 1.137
+++ stdlib/malloc.3 10 Oct 2023 10:23:19 -
@@ -307,7 +307,7 @@ These malloc options imply
.Cm D .
.It Cm F
.Dq Freecheck .
-Enable more extensive double free and use after free detection.
+Enable more extensive double free and write after free detection.
All chunks in the delayed free list will be checked for double frees and
write after frees.
Unused pages on the freelist are read and write protected to
@@ -641,18 +641,34 @@ or
reallocate an unallocated pointer was made.
.It Dq double free
There was an attempt to free an allocation that had already been freed.
-.It Dq write after free
-An allocation has been modified after it was freed.
+.It Dq write of free mem Va address Ns [ Va start Ns .. Ns Va end Ns ]@ Ns Va
size
+An allocation has been modified after it was freed,
+or a chunk that was never allocated was written to.
+The
+.Va range
+at which corruption was detected is printed between [ and ].
+.Pp
+Enabling option
+.Cm D
+allows malloc to print information about where the allocation
+was done.
.It Dq modified chunk-pointer
The pointer passed to
.Fn free
or a reallocation function has been modified.
-.It Dq canary corrupted address offset@length
-A byte after the requested size has been overwritten,
+.It Dq canary corrupted Va address Ns [ Va offset Ns ]@ Ns Va length Ns / Ns
Va size
+A byte after the requested
+.Va length has been overwritten,
indicating a heap overflow.
-The offset at which corruption was detected is printed before the @,
-and the requested length of the allocation after the @.
-.It Dq recorded size oldsize inconsistent with size
+The
+.Va offset
+at which corruption was detected is printed between [ and ],
+the requested
+.Va length
+of the allocation is printed before the / and the
+.Va size
+of the allocation after the /.
+.It Dq recorded size Va oldsize No inconsistent with Va size
.Fn recallocarray
or
.Fn freezero
@@ -676,7 +692,7 @@ functions nor utilize any other function
(e.g.,
.Xr stdio 3
routines).
-.It Dq unknown char in MALLOC_OPTIONS
+.It Dq unknown char in Ev MALLOC_OPTIONS
We found something we didn't understand.
.It any other error
.Fn malloc
Index: stdlib/malloc.c
===
RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.290
diff -u -p -r1.290 malloc.c
--- stdlib/malloc.c 9 Sep 2023 06:52:40 - 1.290
+++ stdlib/malloc.c 10 Oct 2023 10:23:19 -
@@ -112,7 +112,7 @@ struct region_info {
void *p;/* page; low bits used to mark chunks */
uintptr_t size; /* size for pages, or chunk_info pointer */
#ifdef MALLOC_STATS
- void *f;/* where allocated from */
+ void **f; /* where allocated from */
#endif
};
@@ -146,7 +146,7 @@ struct dir_info {
size_t regions_total; /* number of region slots */
size_t regions_free;/* number of free slots */
size_t rbytesused; /* random bytes used */
- char *func; /* current function */
+ const char *func; /* current function */
int malloc_junk;/* junk fill? */
int mmap_flag; /* extra flag for mmap */
int mutex;
@@ -166,6 +166,7 @@ struct dir_info {
void *chunk_pages;
size_t chunk_pages_used;
#ifdef MALLOC_STATS
+ void *caller;
size_t inserts;
size_t insert_collisions;
size_t finds;
@@ -183,12 +184,16 @@ struct dir_info {
#define STATS_INC(x) ((x)++)
#define STATS_ZERO(x) ((x) = 0)
#define STATS_SETF(x,y)((x)->f = (y))
+#define
Re: Significance of MALLOC_OPTIONS=G
On Fri, Sep 29, 2023 at 12:11:51PM +0900, Masato Asou wrote:
> I am investigating what problems can bt detected with MALLOC_OPTIONS.
> SEGV occurs even if MALLOC_OPTIONS=G is not specified. Normally, the
> areas allocated by malloc() are not contiguous. However, after many
> malloc() operations and not free() these areas, contiguous areas may
> be allocated. I guessed that MALLOC_OPTIONS=G would be effective in
> this case, is this correct?
>
>
> The above estimates are based on the following research:
>
> I investigated MALLOC_OPTIONS=G using the following program.
>
> $ cat main.c
> #include
> #include
> #include
>
> int
> main(int argc, char *argv[])
> {
> size_t size;
> char*buf;
>
> size = atoi(argv[1]);
> if ((buf = malloc(size)) == NULL)
> err(1, "malloc(%zu) failed", size);
> buf[size] = 1; /* Writes outside the range allocated by malloc */
> free(buf);
>
> return (0);
> }
> $ cc main.c
> $ MALLOC_OPTIONS=G ./a.out 4096
> zsh: segmentation fault (core dumped) MALLOC_OPTIONS=G ./a.out 4096
>
> The program occurred SEGV. Because, malloc() allocates the requested
> size + MALLOC_PAGESIZE area using mmap() as below, and the makes the
> extra MALLOC_PAGESIZE allocated area to be unreadble and unwritable
> using mprotect().
>
> p = mmap(NULL, size + MALLOC_PAGESIZE, ...);
> mprotect(p + size, MALLOC_PAGESIZE, PROT_NONE);
>
> However, SEGV occurs even if not specify MALLOC_OPTIONS=G.
>
> $ ./a.out 4096
> zsh: segmentation fault (core dumped) ./a.out 4096
>
> Because, malloc() allocates the requested size as below:
>
> p = mmap(NULL, size, ...);
>
> Of course, can not read and write to area that exceeded size.
> --
> ASOU Masato
>
In this caseof a single malloc call you'll get a page that is followed
by unmapped memory as the kernel does that. In general, that may not
happen though, depending on what happended previously (i.e. contents
of the caches used by malloc, or just bad luck). G ensures that the
page is followed by a guard page.
-Otto
Re: malloc write after free error checking
On Wed, Sep 20, 2023 at 08:08:23AM +0200, Otto Moerbeek wrote: > On Sun, Sep 03, 2023 at 09:21:18AM +0200, Otto Moerbeek wrote: > > > Hello, > > > > I'm seeing some reports of "write after free" reported by malloc by > > peolpe running current. Malloc has become more strict since begining > > of June. Let me explain: > > > > Small allocations share a page. e.g. a 4k page will hold 8 512 byte > > allocations. > > > > When one such allocation is freed, it will be filled with a "free > > junk" pattern, stay for a short while in the delayed free list (and be > > subhject to write after free checks) and after that it will be marked > > free and it might be allocated again. On a new allocation the write > > after free check will also be done. That is the new part of my commit > > in June. Another change is that on the allocation of the page > > containing chunks, all chunks wil be filled with a "free junk" (0xdf) > > patttern. > > > > The change has a consequence: the time span of the "write after free" > > checks is much longer, as it can take a long time for a chunk to get > > used again. > > > > I do believe the changes itself are correct and can help findiung > > bugs in other code. > > > > You can also be set upon a wrong foot: if an out of bounds write on a > > adjacent chunk happens and lands in (another) free chunk, upon > > allocation of that free chunk it will be reported as a "write after > > free" case. It might even be that an allocation that was never > > allocated and never freed is still "write after free" because of > > "close" out of bounds writes. > > > > I'm thinking how to change the error message to reflect that. > > > > If these extra checks hinder progress, it is possible to swicth them > > off using malloc option f (small F). > > That should be j (small J). > > > For general debugging I recommend using malloc option S, is has all > > the extra strictness that can be enabled while still conforming to the > > malloc API. > > > > Please be aware of these things while hunting for bugs. > > I'm happy to say two of the more complex ones are (being) fixed: one > turned out to be a reference counting bug in firefox. See > http://undeadly.org/cgi?action=article;sid=20230912094727 > > The other, a write after free that crashed the X server when running > picard was diagnosed by me. This one was a bit nasty, as it required > instrumenting malloc to print some extra info to find the root cause. > > The bug is that the call in > https://github.com/openbsd/xenocara/blob/master/xserver/Xext/xvdisp.c#L1002 > overwrites the first 4 bytes of the chunk next to the one allocated on > line 995. > > A workaround is to allocate 4 bytes extra, matthieu@ will be looking > for a proper fix, as it requires knowledge of the X internals. > > I am pondering if the instrumentation hack I used could be modified to > be always available and print the function that did the original > allocation when detecting a write after free. > > The conclusion is that in these two cases, malloc helped in > detecting/finding the bugs (and it was not a bug in malloc itself :-). > Coincidentally this was the topioc of my talk during EuroBSD2023 last > weekend, see http://www.openbsd.org/events.html > > I hope to look at the reports I got for the Wayland port (which is > WIP) as well, see > https://github.com/openbsd/ports/blob/master/wayland/TODO-Wayland.md The wayland issue was found as well, using the same method. I'm working on programming the heuristic that is quite effective into malloc itself. It currently looks like this for the X case above: X(67417) in malloc(): write to free mem 0xdd0277d4270 [0..7]@16 allocated at /usr/lib/libc.so.97.1 0x92f22 (preceding chunk 0xdd0277d4260 allocated at /usr/X11R6/bin/X 0x177374 (now free)) $ addr2line -e /usr/lib/libc.so.97.1 0x92f22 /usr/src/lib/libc/string/strdup.c:45 $ addr2line -e /usr/X11R6/bin/X 0x177374 /usr/xenocara/xserver/Xext/xvdisp.c:995 The idea is: if a buffer overflow is detected, it is very wel possible that the overwrite happened as an out-of-bound write of the preceding chunk. It is also possible that it is a genuine write-after-free, in that case the developer should inspect the code that allocated and manipulated the first chunk. Malloc has no way to the the difference, that requires debugging by a human. This is all experimental and the final form may change but it certainly look promising, especially as regular malloc code did not change at all (the extra info needed is only collected if malloc flag D is set). -Otto
Re: malloc write after free error checking
On Wed, Sep 20, 2023 at 03:02:27PM +0200, Matthieu Herrb wrote: > On Wed, Sep 20, 2023 at 08:08:23AM +0200, Otto Moerbeek wrote: > > > > The other, a write after free that crashed the X server when running > > picard was diagnosed by me. This one was a bit nasty, as it required > > instrumenting malloc to print some extra info to find the root cause. > > > > The bug is that the call in > > https://github.com/openbsd/xenocara/blob/master/xserver/Xext/xvdisp.c#L1002 > > overwrites the first 4 bytes of the chunk next to the one allocated on > > line 995. > > > > A workaround is to allocate 4 bytes extra, matthieu@ will be looking > > for a proper fix, as it requires knowledge of the X internals. > > > > Hi, > > Thanks again for finding it. Can you try this patch ? > > === > Fix overflow in glamor_xv_query_image_attributes for NV12 image format > > This is a format with num_planes == 2, we have only 2 elements in > offsets[] and pitches[] > > Bug found by Otto Moerbeek on OpenBSD using his strict malloc checking. > --- > glamor/glamor_xv.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/glamor/glamor_xv.c b/glamor/glamor_xv.c > index a3d6b3bc3..e0e8e0ba9 100644 > --- a/glamor/glamor_xv.c > +++ b/glamor/glamor_xv.c > @@ -291,10 +291,10 @@ glamor_xv_query_image_attributes(int id, > pitches[0] = size; > size *= *h; > if (offsets) > -offsets[1] = offsets[2] = size; > +offsets[1] = size; > tmp = ALIGN(*w, 4); > if (pitches) > -pitches[1] = pitches[2] = tmp; > +pitches[1] = tmp; > tmp *= (*h >> 1); > size += tmp; > break; > --- > 2.42.0 Yes, that works for me, -Otto
Re: malloc write after free error checking
On Sun, Sep 03, 2023 at 09:21:18AM +0200, Otto Moerbeek wrote: > Hello, > > I'm seeing some reports of "write after free" reported by malloc by > peolpe running current. Malloc has become more strict since begining > of June. Let me explain: > > Small allocations share a page. e.g. a 4k page will hold 8 512 byte > allocations. > > When one such allocation is freed, it will be filled with a "free > junk" pattern, stay for a short while in the delayed free list (and be > subhject to write after free checks) and after that it will be marked > free and it might be allocated again. On a new allocation the write > after free check will also be done. That is the new part of my commit > in June. Another change is that on the allocation of the page > containing chunks, all chunks wil be filled with a "free junk" (0xdf) > patttern. > > The change has a consequence: the time span of the "write after free" > checks is much longer, as it can take a long time for a chunk to get > used again. > > I do believe the changes itself are correct and can help findiung > bugs in other code. > > You can also be set upon a wrong foot: if an out of bounds write on a > adjacent chunk happens and lands in (another) free chunk, upon > allocation of that free chunk it will be reported as a "write after > free" case. It might even be that an allocation that was never > allocated and never freed is still "write after free" because of > "close" out of bounds writes. > > I'm thinking how to change the error message to reflect that. > > If these extra checks hinder progress, it is possible to swicth them > off using malloc option f (small F). That should be j (small J). > For general debugging I recommend using malloc option S, is has all > the extra strictness that can be enabled while still conforming to the > malloc API. > > Please be aware of these things while hunting for bugs. I'm happy to say two of the more complex ones are (being) fixed: one turned out to be a reference counting bug in firefox. See http://undeadly.org/cgi?action=article;sid=20230912094727 The other, a write after free that crashed the X server when running picard was diagnosed by me. This one was a bit nasty, as it required instrumenting malloc to print some extra info to find the root cause. The bug is that the call in https://github.com/openbsd/xenocara/blob/master/xserver/Xext/xvdisp.c#L1002 overwrites the first 4 bytes of the chunk next to the one allocated on line 995. A workaround is to allocate 4 bytes extra, matthieu@ will be looking for a proper fix, as it requires knowledge of the X internals. I am pondering if the instrumentation hack I used could be modified to be always available and print the function that did the original allocation when detecting a write after free. The conclusion is that in these two cases, malloc helped in detecting/finding the bugs (and it was not a bug in malloc itself :-). Coincidentally this was the topioc of my talk during EuroBSD2023 last weekend, see http://www.openbsd.org/events.html I hope to look at the reports I got for the Wayland port (which is WIP) as well, see https://github.com/openbsd/ports/blob/master/wayland/TODO-Wayland.md -Otto
Re: malloc: add error message in putleakinfo
On Sat, Sep 09, 2023 at 11:27:37AM +0900, Masato Asou wrote:
> From: Otto Moerbeek
> Date: Fri, 8 Sep 2023 13:39:53 +0200
>
> > On Fri, Sep 08, 2023 at 10:08:28AM +0900, Masato Asou wrote:
> >
> >> From: Masato Asou
> >> Date: Fri, 08 Sep 2023 05:45:55 +0900 (JST)
> >>
> >> > There was a mistake in the diff.
> >> >
> >> > From: Masato Asou
> >> > Date: Fri, 08 Sep 2023 05:33:23 +0900 (JST)
> >> >
> >> >> Hi,
> >> >>
> >> >> I have modified diff. comments, ok?
> >
> > As wrtwarning() is only used #ifdef MALLOC_STATS, please put it inside
> > those guards (e.g directly above putleakinfo()).
> >
> > -Otto
>
> I have fixed diff. ok?
OK,
-Otto
> --
> ASOU Masato
>
> Index: stdlib/malloc.c
> ===
> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.289
> diff -u -p -r1.289 malloc.c
> --- stdlib/malloc.c 30 Jun 2023 06:24:58 - 1.289
> +++ stdlib/malloc.c 9 Sep 2023 02:16:22 -
> @@ -2338,6 +2338,22 @@ RBT_PROTOTYPE(leaktree, leaknode, entry,
> RBT_GENERATE(leaktree, leaknode, entry, leakcmp);
>
> static void
> +wrtwarning(const char *func, char *msg, ...)
> +{
> + int saved_errno = errno;
> + va_list ap;
> +
> + dprintf(STDERR_FILENO, "%s(%d) in %s(): ", __progname,
> + getpid(), func != NULL ? func : "unknown");
> + va_start(ap, msg);
> + vdprintf(STDERR_FILENO, msg, ap);
> + va_end(ap);
> + dprintf(STDERR_FILENO, "\n");
> +
> + errno = saved_errno;
> +}
> +
> +static void
> putleakinfo(struct leaktree *leaks, void *f, size_t sz, int cnt)
> {
> struct leaknode key, *p;
> @@ -2353,8 +2369,10 @@ putleakinfo(struct leaktree *leaks, void
> if (page == NULL ||
> used >= MALLOC_PAGESIZE / sizeof(struct leaknode)) {
> page = MMAP(MALLOC_PAGESIZE, 0);
> - if (page == MAP_FAILED)
> + if (page == MAP_FAILED) {
> + wrtwarning(__func__, strerror(errno));
> return;
> + }
> used = 0;
> }
> p = [used++];
>
> >> >>
> >> >> $ MALLOC_OPTIONS=D ktrace -tu ./a.out 107349
> >> >> a.out(99781) in unknown(): putleakinfo(): Cannot allocate memory
> >> >> --
> >> >> ASOU Masato
> >> >>
> >> >> Index: stdlib/malloc.c
> >> >> ===
> >> >> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> >> >> retrieving revision 1.289
> >> >> diff -u -p -r1.289 malloc.c
> >> >> --- stdlib/malloc.c 30 Jun 2023 06:24:58 - 1.289
> >> >> +++ stdlib/malloc.c 7 Sep 2023 20:30:01 -
> >> >> @@ -344,6 +344,22 @@ wrterror(struct dir_info *d, char *msg,
> >> >> }
> >> >>
> >> >> static void
> >> >> +wrtwarning(char *func, char *msg, ...)
> >> >> +{
> >> >> + int saved_errno = errno;
> >> >> + va_list ap;
> >> >> +
> >> >> + dprintf(STDERR_FILENO, "%s(%d) in %s(): ", __progname,
> >> >> + getpid(), func == NULL ? func : "unknown");
> >> >
> >> > func != NULL ? func : "unknown"
> >> >
> >> > I will take a break and re-create the diff.
> >>
> >> I have fixed the diff.
> >>
> >> $ MALLOC_OPTIONS=D ktrace -tu ./a.out 107350
> >> a.out(34886) in putleakinfo(): Cannot allocate memory
> >> $ kdump -u malloc
> >> Start dump a.out ***
> >> M=8 I=1 F=0 U=0 J=1 R=0 X=0 C=0 cache=64 G=0
> >> Leak report:
> >> f sum #avg
> >>
> >> End dump a.out ***
> >> $ ls -l ktrace.out
> >> -rw--- 1 asou asou 734 Sep 8 09:59 ktrace.out
> >>
> >> comments, ok?
> >> --
> >> ASOU Masato
> >>
> >> Index: stdlib/malloc.c
> >> ===
> >> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> >>
Re: malloc: add error message in putleakinfo
On Fri, Sep 08, 2023 at 10:08:28AM +0900, Masato Asou wrote:
> From: Masato Asou
> Date: Fri, 08 Sep 2023 05:45:55 +0900 (JST)
>
> > There was a mistake in the diff.
> >
> > From: Masato Asou
> > Date: Fri, 08 Sep 2023 05:33:23 +0900 (JST)
> >
> >> Hi,
> >>
> >> I have modified diff. comments, ok?
As wrtwarning() is only used #ifdef MALLOC_STATS, please put it inside
those guards (e.g directly above putleakinfo()).
-Otto
> >>
> >> $ MALLOC_OPTIONS=D ktrace -tu ./a.out 107349
> >> a.out(99781) in unknown(): putleakinfo(): Cannot allocate memory
> >> --
> >> ASOU Masato
> >>
> >> Index: stdlib/malloc.c
> >> ===
> >> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> >> retrieving revision 1.289
> >> diff -u -p -r1.289 malloc.c
> >> --- stdlib/malloc.c30 Jun 2023 06:24:58 - 1.289
> >> +++ stdlib/malloc.c7 Sep 2023 20:30:01 -
> >> @@ -344,6 +344,22 @@ wrterror(struct dir_info *d, char *msg,
> >> }
> >>
> >> static void
> >> +wrtwarning(char *func, char *msg, ...)
> >> +{
> >> + int saved_errno = errno;
> >> + va_list ap;
> >> +
> >> + dprintf(STDERR_FILENO, "%s(%d) in %s(): ", __progname,
> >> + getpid(), func == NULL ? func : "unknown");
> >
> > func != NULL ? func : "unknown"
> >
> > I will take a break and re-create the diff.
>
> I have fixed the diff.
>
> $ MALLOC_OPTIONS=D ktrace -tu ./a.out 107350
> a.out(34886) in putleakinfo(): Cannot allocate memory
> $ kdump -u malloc
> Start dump a.out ***
> M=8 I=1 F=0 U=0 J=1 R=0 X=0 C=0 cache=64 G=0
> Leak report:
> f sum #avg
>
> End dump a.out ***
> $ ls -l ktrace.out
> -rw--- 1 asou asou 734 Sep 8 09:59 ktrace.out
>
> comments, ok?
> --
> ASOU Masato
>
> Index: stdlib/malloc.c
> ===
> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.289
> diff -u -p -r1.289 malloc.c
> --- stdlib/malloc.c 30 Jun 2023 06:24:58 - 1.289
> +++ stdlib/malloc.c 8 Sep 2023 00:55:13 -
> @@ -344,6 +344,22 @@ wrterror(struct dir_info *d, char *msg,
> }
>
> static void
> +wrtwarning(const char *func, char *msg, ...)
> +{
> + int saved_errno = errno;
> + va_list ap;
> +
> + dprintf(STDERR_FILENO, "%s(%d) in %s(): ", __progname,
> + getpid(), func != NULL ? func : "unknown");
> + va_start(ap, msg);
> + vdprintf(STDERR_FILENO, msg, ap);
> + va_end(ap);
> + dprintf(STDERR_FILENO, "\n");
> +
> + errno = saved_errno;
> +}
> +
> +static void
> rbytes_init(struct dir_info *d)
> {
> arc4random_buf(d->rbytes, sizeof(d->rbytes));
> @@ -2353,8 +2369,10 @@ putleakinfo(struct leaktree *leaks, void
> if (page == NULL ||
> used >= MALLOC_PAGESIZE / sizeof(struct leaknode)) {
> page = MMAP(MALLOC_PAGESIZE, 0);
> - if (page == MAP_FAILED)
> + if (page == MAP_FAILED) {
> + wrtwarning(__func__, strerror(errno));
> return;
> + }
> used = 0;
> }
> p = [used++];
Re: malloc: add error message in putleakinfo
On Thu, Sep 07, 2023 at 06:45:55PM +0900, Masato Asou wrote:
> Hi,
>
> I am using MALLOC_OPTIONS=D and kdump report no information when
> malloc() exceeds 107350 bytes on my OpenBSD box. I have
> investigated and found that mmap() faild in putleakinfo().
I'm ok with the general idea, but see inline
>
>
> I used the following program:
>
> #include
> #include
> #include
> #include
> #include
>
> int
> main(int argc, char *argv[])
> {
> size_t size;
> void*buf;
> char*end;
>
> if (argc != 2) {
> fprintf(stderr, "usage: ./a.out size\n");
> exit(1);
> }
>
> size = strtol(argv[1], , 0);
> if (argv[1] == end) {
> fprintf(stderr, "Invalid argument: %s\n", argv[1]);
> exit(1);
> }
> if (*end == 'k')
> size *= 1024;
> else if (*end == 'm')
> size *= 1024 * 1024;
> else if (*end == 'g')
> size *= 1024 * 1024 * 1024;
>
> if ((buf = malloc(size)) == NULL)
> err(1, "malloc");
>
> return (0);
> }
>
> I used this program to perform the following:
>
> $ MALLOC_OPTIONS=D ktrace -tu ./a.out 107349
> $ kdump -u malloc
> Start dump a.out ***
> M=8 I=1 F=0 U=0 J=1 R=0 X=0 C=0 cache=64 G=0
> Leak report:
> f sum #avg
> 0xeb017e21c6f 107349 1 107349 addr2line -e ./a.out
> 0x1c6f
>
> End dump a.out ***
> $ MALLOC_OPTIONS=D ktrace -tu ./a.out 107350
> $ kdump -u malloc
> $ ls -l ktrace.out
> -rw--- 1 asou asou 64 Sep 7 18:38 ktrace.out
> $ export LD_LIBRARY_PATH=/usr/obj/lib/libc
> $ MALLOC_OPTIONS=D ktrace -tu ./a.out 107350
> /usr/src/lib/libc/stdlib/malloc.c: putleakinfo: Cannot allocate memory
> $
>
>
> In such a case, shouldn't an error message be displayed? I made a
> patch for print error message if mmap() was failed.
>
> comments, ok?
> --
> ASOU Masato
>
> Index: lib/libc/stdlib/malloc.c
> ===
> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.289
> diff -u -p -r1.289 malloc.c
> --- lib/libc/stdlib/malloc.c 30 Jun 2023 06:24:58 - 1.289
> +++ lib/libc/stdlib/malloc.c 7 Sep 2023 09:44:59 -
> @@ -2354,7 +2354,14 @@ putleakinfo(struct leaktree *leaks, void
> used >= MALLOC_PAGESIZE / sizeof(struct leaknode)) {
> page = MMAP(MALLOC_PAGESIZE, 0);
> if (page == MAP_FAILED)
> + {
Brace should be after if (... )
> + char buf[256];
> +
> + snprintf(buf, sizeof (buf), "%s: %s: %s\n",
> + __FILE__, __func__, strerror(errno));
> + write(STDERR_FILENO, buf, strlen(buf));
> return;
Please format the message to be like what wrterror() does (without the
abort of ccurse), using dprintf().
-Otto
> + }
> used = 0;
> }
> p = [used++];
>
Re: malloc write after free error checking
On Sun, Sep 03, 2023 at 09:30:49AM +0200, Otto Moerbeek wrote: > > You can also be set upon a wrong foot: if an out of bounds write on a > > adjacent chunk happens and lands in (another) free chunk, upon > > allocation of that free chunk it will be reported as a "write after > > free" case. It might even be that an allocation that was never > > allocated and never freed is still "write after free" because of > > "close" out of bounds writes. > > > > I'm thinking how to change the error message to reflect that. I was thinking to change the message to "write to unallocated chunk", trying to get the message trough that it also applies to chunks never allocated but written to, not only to chunks that were freed and then written to. -Otto
Re: malloc write after free error checking
On Sun, Sep 03, 2023 at 09:21:18AM +0200, Otto Moerbeek wrote: > Hello, > > I'm seeing some reports of "write after free" reported by malloc by > peolpe running current. Malloc has become more strict since begining > of June. Let me explain: > > Small allocations share a page. e.g. a 4k page will hold 8 512 byte > allocations. > > When one such allocation is freed, it will be filled with a "free > junk" pattern, stay for a short while in the delayed free list (and be > subhject to write after free checks) and after that it will be marked > free and it might be allocated again. On a new allocation the write > after free check will also be done. That is the new part of my commit > in June. Another change is that on the allocation of the page > containing chunks, all chunks wil be filled with a "free junk" (0xdf) > patttern. > > The change has a consequence: the time span of the "write after free" > checks is much longer, as it can take a long time for a chunk to get > used again. > > I do believe the changes itself are correct and can help findiung > bugs in other code. > > You can also be set upon a wrong foot: if an out of bounds write on a > adjacent chunk happens and lands in (another) free chunk, upon > allocation of that free chunk it will be reported as a "write after > free" case. It might even be that an allocation that was never > allocated and never freed is still "write after free" because of > "close" out of bounds writes. > > I'm thinking how to change the error message to reflect that. > > If these extra checks hinder progress, it is possible to swicth them > off using malloc option f (small F). Oops, that should be j (small J). > > For general debugging I recommend using malloc option S, is has all > the extra strictness that can be enabled while still conforming to the > malloc API. > > Please be aware of these things while hunting for bugs. > > -Otto >
malloc write after free error checking
Hello, I'm seeing some reports of "write after free" reported by malloc by peolpe running current. Malloc has become more strict since begining of June. Let me explain: Small allocations share a page. e.g. a 4k page will hold 8 512 byte allocations. When one such allocation is freed, it will be filled with a "free junk" pattern, stay for a short while in the delayed free list (and be subhject to write after free checks) and after that it will be marked free and it might be allocated again. On a new allocation the write after free check will also be done. That is the new part of my commit in June. Another change is that on the allocation of the page containing chunks, all chunks wil be filled with a "free junk" (0xdf) patttern. The change has a consequence: the time span of the "write after free" checks is much longer, as it can take a long time for a chunk to get used again. I do believe the changes itself are correct and can help findiung bugs in other code. You can also be set upon a wrong foot: if an out of bounds write on a adjacent chunk happens and lands in (another) free chunk, upon allocation of that free chunk it will be reported as a "write after free" case. It might even be that an allocation that was never allocated and never freed is still "write after free" because of "close" out of bounds writes. I'm thinking how to change the error message to reflect that. If these extra checks hinder progress, it is possible to swicth them off using malloc option f (small F). For general debugging I recommend using malloc option S, is has all the extra strictness that can be enabled while still conforming to the malloc API. Please be aware of these things while hunting for bugs. -Otto
Re: pax: truncate times to MAX_TIME_T, not INT_MAX
On Mon, Jun 26, 2023 at 11:09:10AM -0600, Todd C. Miller wrote:
> If the mtime in the file header is larger than MAX_TIME_T, trucate
> it to MAX_TIME_T, not INT_MAX. The existing assignment dates from
> before we had a MAX_TIME_T definition in pax.
How strange the checks use MAX_TIME_T as a bound value, but the
assignments not. For FFS1 it likely does not matter, since its
timestamps are 32-bit on disk. But for ffs2 it does.
OK,
-Otto
>
> OK?
>
> - todd
>
> Index: cpio.c
> ===
> RCS file: /cvs/src/bin/pax/cpio.c,v
> retrieving revision 1.33
> diff -u -p -u -r1.33 cpio.c
> --- cpio.c16 Sep 2017 07:42:34 - 1.33
> +++ cpio.c26 Jun 2023 17:05:35 -
> @@ -294,7 +294,7 @@ cpio_rd(ARCHD *arcn, char *buf)
> arcn->sb.st_rdev = (dev_t)asc_ul(hd->c_rdev, sizeof(hd->c_rdev), OCT);
> val = asc_ull(hd->c_mtime, sizeof(hd->c_mtime), OCT);
> if (val > MAX_TIME_T)
> - arcn->sb.st_mtime = INT_MAX;/* XXX 2038 */
> + arcn->sb.st_mtime = MAX_TIME_T;
> else
> arcn->sb.st_mtime = val;
> arcn->sb.st_mtim.tv_nsec = 0;
> Index: tar.c
> ===
> RCS file: /cvs/src/bin/pax/tar.c,v
> retrieving revision 1.70
> diff -u -p -u -r1.70 tar.c
> --- tar.c 1 Mar 2022 21:19:11 - 1.70
> +++ tar.c 26 Jun 2023 17:05:35 -
> @@ -411,7 +411,7 @@ tar_rd(ARCHD *arcn, char *buf)
> arcn->sb.st_size = (off_t)asc_ull(hd->size, sizeof(hd->size), OCT);
> val = asc_ull(hd->mtime, sizeof(hd->mtime), OCT);
> if (val > MAX_TIME_T)
> - arcn->sb.st_mtime = INT_MAX;/* XXX 2038 */
> + arcn->sb.st_mtime = MAX_TIME_T;
> else
> arcn->sb.st_mtime = val;
> arcn->sb.st_ctime = arcn->sb.st_atime = arcn->sb.st_mtime;
> @@ -788,7 +788,7 @@ reset:
> if (arcn->sb.st_mtime == 0) {
> val = asc_ull(hd->mtime, sizeof(hd->mtime), OCT);
> if (val > MAX_TIME_T)
> - arcn->sb.st_mtime = INT_MAX;/* XXX 2038 */
> + arcn->sb.st_mtime = MAX_TIME_T;
> else
> arcn->sb.st_mtime = val;
> }
>
Re: recent malloc changes
On Sun, Jun 18, 2023 at 03:51:20PM +0200, Theo Buehler wrote: > > So what's in your malloc options? > > mdnsd -d must have thrown this warning for nearly 7 years. With d, it sets > 'malloc_options = "AFGJPX";'. You removed A and P end of october 2016. Sigh should be "S". I take care to make sure that S always has the meaning: all flags that enable extra strict checking or otherwise enhance securiy but cannot be enabled always for performance reasons. All other flag are subject to change. -Otto
Re: recent malloc changes
On Sun, Jun 18, 2023 at 03:24:17PM +0200, Why 42? The lists account. wrote: > > On Sun, Jun 04, 2023 at 01:03:14PM +0200, Otto Moerbeek wrote: > > Hello, > > > > In the last few weeks a series of mallocs diff have been committed. > > The last one today. That one allows malloc to detect more cases of > > write-after-free. While that is a good thing, it might uncover latent > > bugs in appliations. > > > > So if you are running current or snapshots, please keep an eye out for > > issues reported by malloc. If we get too many reports of issues I > > might change things so the extra write-after-free detetecion is ony > > enabled when malloc option S is active. > > Well, I don't know if this related, but I just noticed this: > mjoelnir:robb 18.06 15:17:30 # rcctl check mdnsd > mdnsd(failed) > > mjoelnir:robb 18.06 15:17:32 [$?==1]# rcctl restart mdnsd > mdnsd(ok) > > mjoelnir:robb 18.06 15:17:37 # rcctl check mdnsd > mdnsd(failed) > > mjoelnir:robb 18.06 15:17:39 [$?==1]# pgrep -l mdnsd > mjoelnir:robb 18.06 15:18:06 [$?==1]# > > mjoelnir:robb 18.06 15:18:07 [$?==1]# mdnsd -h > mdnsd: unknown option -- h > usage: mdnsd [-dw] ifname [ifnames...] > usage: mdnsd -v > > mjoelnir:robb 18.06 15:18:22 [$?==1]# mdnsd -dv > malloc() warning: unknown char in MALLOC_OPTIONS > malloc() warning: unknown char in MALLOC_OPTIONS So what's in your malloc options? -Otto > OpenMdns Daemon 0.7 (2017-03-10) > Copyright (C) 2010-2014 Christiano F. Haesbaert > mjoelnir:robb 18.06 15:18:30 # > > mjoelnir:robb 18.06 15:18:33 # pgrep -l mdnsd > mjoelnir:robb 18.06 15:18:36 [$?==1]# mdnsd -d > malloc() warning: unknown char in MALLOC_OPTIONS > malloc() warning: unknown char in MALLOC_OPTIONS > usage: mdnsd [-dw] ifname [ifnames...] > usage: mdnsd -v > > This is with a 7.3 snapshot from about a week ago: > mjoelnir:robb 18.06 15:22:22 # ls -ltr /bsd* > -rwx-- 1 root wheel 25245701 Jun 11 13:42 /bsd.sp > -rw--- 1 root wheel 4674809 Jun 11 13:42 /bsd.rd > -rwx-- 1 root wheel 25364480 Jun 11 13:44 /bsd.booted > -rwx-- 1 root wheel 25375272 Jun 11 13:56 /bsd > > mjoelnir:robb 18.06 15:21:53 # uname -a > OpenBSD mjoelnir.fritz.box 7.3 GENERIC.MP#1230 amd64 >
recent malloc changes
Hello, In the last few weeks a series of mallocs diff have been committed. The last one today. That one allows malloc to detect more cases of write-after-free. While that is a good thing, it might uncover latent bugs in appliations. So if you are running current or snapshots, please keep an eye out for issues reported by malloc. If we get too many reports of issues I might change things so the extra write-after-free detetecion is ony enabled when malloc option S is active. -Otto
Re: malloc: better write-after-free detection for chunks
On Sun, May 14, 2023 at 10:42:34AM +0200, Otto Moerbeek wrote:
> Hi,
>
> On free, chunks (the pieces of a pages used for smaller allocations)
> are junked and then validated after they leave the delayed free list.
> So after free, a chunk always contains junk bytes. This means that if
> we start with the right contents for a new page of chunks, we can
> *validate* instead of *write* junk bytes when (re)-using a chunk.
>
> Wiht this, we can detect write-after-free when a chunk is recycled,
> not justy when a chunk is in the delayed free list. We do a little
> bit more work on initial allocation of a page of chunks and when
> re-using (as I validate now even on junk level 1), so some performance
> validation is needed. In my tests I did not see negative effects, even
> some slight improvemt (likely because validating junk bytes is cheaper
> than writing). But this needs tests to see if that is true in more
> cases than my tests.
>
> Also: some extra consistency checks for recallocaray(3) and fixes in
> error messages to make them more consistent, with man page bits. Plus
> regress additions.
>
> Please test and review!
>
> Thanks,
>
> -Otto
>
Now with diff attached!
Index: lib/libc/stdlib/malloc.3
===
RCS file: /home/cvs/src/lib/libc/stdlib/malloc.3,v
retrieving revision 1.132
diff -u -p -r1.132 malloc.3
--- lib/libc/stdlib/malloc.317 Apr 2023 05:45:06 - 1.132
+++ lib/libc/stdlib/malloc.314 May 2023 08:27:16 -
@@ -314,7 +314,7 @@ Increase the junk level by one if it is
Decrease the junk level by one if it is larger than 0.
Junking writes some junk bytes into the area allocated.
Junk is bytes of 0xdb when allocating;
-freed chunks are filled with 0xdf.
+freed allocations are filled with 0xdf.
By default the junk level is 1: after free,
small chunks are completely junked;
for pages the first part is junked.
@@ -628,22 +628,24 @@ An attempt to
.Fn free
or
reallocate an unallocated pointer was made.
-.It Dq chunk is already free
-There was an attempt to free a chunk that had already been freed.
+.It Dq double free
+There was an attempt to free an allocation that had already been freed.
.It Dq write after free
-A chunk has been modified after it was freed.
+An allocation has been modified after it was freed.
.It Dq modified chunk-pointer
The pointer passed to
.Fn free
or a reallocation function has been modified.
-.It Dq chunk canary corrupted address offset@length
+.It Dq canary corrupted address offset@length
A byte after the requested size has been overwritten,
indicating a heap overflow.
The offset at which corruption was detected is printed before the @,
and the requested length of the allocation after the @.
-.It Dq recorded old size oldsize != size
+.It Dq recorded size oldsize inconsistent with size
.Fn recallocarray
-has detected that the given old size does not equal the recorded size in its
+or
+.Fn freezero
+has detected that the given old size does not match the recorded size in its
meta data.
Enabling option
.Cm C
Index: lib/libc/stdlib/malloc.c
===
RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.283
diff -u -p -r1.283 malloc.c
--- lib/libc/stdlib/malloc.c10 May 2023 07:58:06 - 1.283
+++ lib/libc/stdlib/malloc.c14 May 2023 08:27:16 -
@@ -977,6 +977,10 @@ omalloc_make_chunks(struct dir_info *d,
NULL))
goto err;
LIST_INSERT_HEAD(>chunk_dir[bucket][listnum], bp, entries);
+
+ if (bucket > 0 && d->malloc_junk != 0)
+ memset(pp, SOME_FREEJUNK, MALLOC_PAGESIZE);
+
return bp;
err:
@@ -1113,9 +1117,8 @@ found:
p = (char *)bp->page + k;
if (bp->bucket > 0) {
- if (d->malloc_junk == 2)
- memset(p, SOME_JUNK, B2SIZE(bp->bucket));
- else if (mopts.chunk_canaries)
+ validate_junk(d, p, B2SIZE(bp->bucket));
+ if (mopts.chunk_canaries)
fill_canary(p, size, B2SIZE(bp->bucket));
}
return p;
@@ -1134,7 +1137,7 @@ validate_canary(struct dir_info *d, u_ch
while (p < q) {
if (*p != (u_char)mopts.chunk_canaries && *p != SOME_JUNK) {
- wrterror(d, "chunk canary corrupted %p %#tx@%#zx%s",
+ wrterror(d, "canary corrupted %p %#tx@%#zx%s",
ptr, p - ptr, sz,
*p == SOME_FREEJUNK ? " (double free?)" : "");
}
@@ -1157,7 +1160,7 @@ find_chunknum(struct dir_info *d, struct
wrterror(d, "modified chunk-pointer %p", ptr);
if (info->bits[chunknum / MALLOC_BI
malloc: better write-after-free detection for chunks
Hi, On free, chunks (the pieces of a pages used for smaller allocations) are junked and then validated after they leave the delayed free list. So after free, a chunk always contains junk bytes. This means that if we start with the right contents for a new page of chunks, we can *validate* instead of *write* junk bytes when (re)-using a chunk. Wiht this, we can detect write-after-free when a chunk is recycled, not justy when a chunk is in the delayed free list. We do a little bit more work on initial allocation of a page of chunks and when re-using (as I validate now even on junk level 1), so some performance validation is needed. In my tests I did not see negative effects, even some slight improvemt (likely because validating junk bytes is cheaper than writing). But this needs tests to see if that is true in more cases than my tests. Also: some extra consistency checks for recallocaray(3) and fixes in error messages to make them more consistent, with man page bits. Plus regress additions. Please test and review! Thanks, -Otto
Re: malloc: less unlock/lock dancing
On Wed, May 10, 2023 at 10:08:09AM +0200, Theo Buehler wrote: > > Thanks! It has been committed. I doubt though if the Go runtime uses > > libc malloc. > > I don't know if the pure Go runtime uses libc malloc. However, some > of the test code involves cgo and calls into various C libraries > including libcrypto. So it definitely exercised malloc in a threaded > environment. Good to know, thanks again, -Otto
Re: malloc: less unlock/lock dancing
On Tue, May 09, 2023 at 09:55:32PM +0200, Theo Buehler wrote: > On Thu, May 04, 2023 at 03:40:35PM +0200, Otto Moerbeek wrote: > > On Thu, Apr 27, 2023 at 02:17:10PM +0200, Otto Moerbeek wrote: > > > > > This was introduced to not stall other threads while mmap is called by > > > a thread. But now that mmap is unlocked, I believe it is no longer > > > useful. > > > > > > A full build is slighlty faster with this. But this also needs testing > > > with you favorite multithreaded program. > > > > I'd really would like some feedback/performance tests on this. > > I see the same: slight reduction of 'make build' time and it also works > fine with heavily threaded things like some Go that I play with. I have > put this through a full bulk build with no fallout. It seems to be > slightly faster for this as well. > > I have been running this on several machines including my main laptop > and could not spot a single downside. > > The diff makes sense, so > > ok tb Thanks! It has been committed. I doubt though if the Go runtime uses libc malloc. -Otto
Re: malloc: less unlock/lock dancing
On Thu, Apr 27, 2023 at 02:17:10PM +0200, Otto Moerbeek wrote:
> This was introduced to not stall other threads while mmap is called by
> a thread. But now that mmap is unlocked, I believe it is no longer
> useful.
>
> A full build is slighlty faster with this. But this also needs testing
> with you favorite multithreaded program.
I'd really would like some feedback/performance tests on this.
-Otto
>
> Index: stdlib/malloc.c
> ===
> RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.282
> diff -u -p -r1.282 malloc.c
> --- stdlib/malloc.c 21 Apr 2023 06:19:40 - 1.282
> +++ stdlib/malloc.c 27 Apr 2023 05:40:49 -
> @@ -264,24 +264,6 @@ static void malloc_exit(void);
> (sz) = (uintptr_t)(r)->p & MALLOC_PAGEMASK, \
> (sz) = ((sz) == 0 ? (r)->size : B2SIZE((sz) - 1))
>
> -static inline void
> -_MALLOC_LEAVE(struct dir_info *d)
> -{
> - if (d->malloc_mt) {
> - d->active--;
> - _MALLOC_UNLOCK(d->mutex);
> - }
> -}
> -
> -static inline void
> -_MALLOC_ENTER(struct dir_info *d)
> -{
> - if (d->malloc_mt) {
> - _MALLOC_LOCK(d->mutex);
> - d->active++;
> - }
> -}
> -
> static inline size_t
> hash(void *p)
> {
> @@ -879,9 +861,7 @@ map(struct dir_info *d, size_t sz, int z
> return p;
> }
> if (psz <= 1) {
> - _MALLOC_LEAVE(d);
> p = MMAP(cache->max * sz, d->mmap_flag);
> - _MALLOC_ENTER(d);
> if (p != MAP_FAILED) {
> STATS_ADD(d->malloc_used, cache->max * sz);
> cache->length = cache->max - 1;
> @@ -901,9 +881,7 @@ map(struct dir_info *d, size_t sz, int z
> }
>
> }
> - _MALLOC_LEAVE(d);
> p = MMAP(sz, d->mmap_flag);
> - _MALLOC_ENTER(d);
> if (p != MAP_FAILED)
> STATS_ADD(d->malloc_used, sz);
> /* zero fill not needed */
>
malloc: less unlock/lock dancing
This was introduced to not stall other threads while mmap is called by
a thread. But now that mmap is unlocked, I believe it is no longer
useful.
A full build is slighlty faster with this. But this also needs testing
with you favorite multithreaded program.
-Otto
Index: stdlib/malloc.c
===
RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.282
diff -u -p -r1.282 malloc.c
--- stdlib/malloc.c 21 Apr 2023 06:19:40 - 1.282
+++ stdlib/malloc.c 27 Apr 2023 05:40:49 -
@@ -264,24 +264,6 @@ static void malloc_exit(void);
(sz) = (uintptr_t)(r)->p & MALLOC_PAGEMASK, \
(sz) = ((sz) == 0 ? (r)->size : B2SIZE((sz) - 1))
-static inline void
-_MALLOC_LEAVE(struct dir_info *d)
-{
- if (d->malloc_mt) {
- d->active--;
- _MALLOC_UNLOCK(d->mutex);
- }
-}
-
-static inline void
-_MALLOC_ENTER(struct dir_info *d)
-{
- if (d->malloc_mt) {
- _MALLOC_LOCK(d->mutex);
- d->active++;
- }
-}
-
static inline size_t
hash(void *p)
{
@@ -879,9 +861,7 @@ map(struct dir_info *d, size_t sz, int z
return p;
}
if (psz <= 1) {
- _MALLOC_LEAVE(d);
p = MMAP(cache->max * sz, d->mmap_flag);
- _MALLOC_ENTER(d);
if (p != MAP_FAILED) {
STATS_ADD(d->malloc_used, cache->max * sz);
cache->length = cache->max - 1;
@@ -901,9 +881,7 @@ map(struct dir_info *d, size_t sz, int z
}
}
- _MALLOC_LEAVE(d);
p = MMAP(sz, d->mmap_flag);
- _MALLOC_ENTER(d);
if (p != MAP_FAILED)
STATS_ADD(d->malloc_used, sz);
/* zero fill not needed */
malloc leak detection available in -current
Hi, OpenBSD current now has built-in malloc leak detection. Make sure you run current and have debug symbols (OpenBSD base libraries have debug symbols, compile your own program with -g). To record the leak report: $ MALLOC_OPTIONS=D ktrace -tu a.out To view the leak report: $ kdump -u malloc Example output: Start dump a.out *** M=8 I=1 F=0 U=0 J=1 R=0 X=0 C=0 cache=64 G=0 Leak report: f sum #avg 0x0 1088864 9722112 addr2line -e '?' 0x0 0xf4b73093c 31136278112 addr2line -e a.out 0x1093c End dump a.out *** $ addr2line -e a.out 0x1093c /home/otto/x.c:6 Some additional info: The null "f" values (call sites) are due to the sampling nature of small allocations. Recording all call sites of all potential leaks introduces too much overhead. Note that aggresssive optimizations might confuse the line numbers reported. For -static programs, compile with -nopie to make addr2line work. In some cases will want to use the packaged version of addr2line (gaddr2line, in the binutils package) as the base addr2line does not grok all debug info formats. -Otto
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Thu, Apr 13, 2023 at 08:22:45PM +0200, Otto Moerbeek wrote: > On Tue, Apr 11, 2023 at 05:50:43PM +0200, Otto Moerbeek wrote: > > > On Sun, Apr 09, 2023 at 12:17:35PM +0200, Otto Moerbeek wrote: > > > > > On Sun, Apr 09, 2023 at 10:08:25AM +0200, Claudio Jeker wrote: > > > > > > > On Sun, Apr 09, 2023 at 09:15:12AM +0200, Otto Moerbeek wrote: > > > > > On Sun, Apr 09, 2023 at 08:20:43AM +0200, Otto Moerbeek wrote: > > > > > > > > > > > On Sun, Apr 09, 2023 at 07:53:31AM +0200, Sebastien Marie wrote: > > > > > > > > > > > > > On Fri, Apr 07, 2023 at 09:52:52AM +0200, Otto Moerbeek wrote: > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > This is work in progress. I have to think if the flags to > > > > > > > > > kdump I'm > > > > > > > > > introducing should be two or a single one. > > > > > > > > > > > > > > > > > > Currently, malloc.c can be compiled with MALLOC_STATS > > > > > > > > > defined. If run > > > > > > > > > with option D it dumps its state to a malloc.out file at > > > > > > > > > exit. This > > > > > > > > > state can be used to find leaks amongst other things. > > > > > > > > > > > > > > > > > > This is not ideal for pledged processes, as they often have > > > > > > > > > no way to > > > > > > > > > write files. > > > > > > > > > > > > > > > > > > This changes malloc to use utrace(2) for that. > > > > > > > > > > > > > > > > > > As kdump has no nice way to show those lines without all > > > > > > > > > extras it > > > > > > > > > normally shows, so add two options to it to just show the > > > > > > > > > lines. > > > > > > > > > > > > > > > > > > To use, compile and install libc with MALLOC_STATS defined. > > > > > > > > > > > > > > > > > > Run : > > > > > > > > > > > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > > > > > > ... > > > > > > > > > $ kdump -hu > > > > > > > > > > > > > > > > > > Feedback appreciated. > > > > > > > > > > > > > > I can't really comment on malloc(3) stuff, but I agree that > > > > > > > utrace(2) is a good > > > > > > > way to get information outside a pledged process. > > > > > > > > > > > > > > I tend to think it is safe to use it, as the pledged process need > > > > > > > cooperation > > > > > > > from outside to exfiltrate informations (a process with > > > > > > > permission to call > > > > > > > ktrace(2) on this pid). > > > > > > > > > > > > > > Please note it is a somehow generic problem: at least profiled > > > > > > > processes would > > > > > > > also get advantage of using it. > > > > > > > > > > > > > > > > > > > > > Regarding kdump options, I think that -u option should implies -h > > > > > > > (no header). > > > > > > > > > > > > > > Does it would make sens to considere a process using utrace(2) > > > > > > > with several > > > > > > > interleaved records for different sources ? A process with > > > > > > > MALLOC_OPTIONS=D and > > > > > > > profiling enabled for example ? An (another) option on kdump to > > > > > > > filter on utrace > > > > > > > label would be useful in such case, or have -u mandate a label to > > > > > > > filter on. > > > > > > > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > > > > $ kdump -u mallocdumpline > > > > > > > > > > > > > > and for profiling: > > > > > > >
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Tue, Apr 11, 2023 at 05:50:43PM +0200, Otto Moerbeek wrote: > On Sun, Apr 09, 2023 at 12:17:35PM +0200, Otto Moerbeek wrote: > > > On Sun, Apr 09, 2023 at 10:08:25AM +0200, Claudio Jeker wrote: > > > > > On Sun, Apr 09, 2023 at 09:15:12AM +0200, Otto Moerbeek wrote: > > > > On Sun, Apr 09, 2023 at 08:20:43AM +0200, Otto Moerbeek wrote: > > > > > > > > > On Sun, Apr 09, 2023 at 07:53:31AM +0200, Sebastien Marie wrote: > > > > > > > > > > > On Fri, Apr 07, 2023 at 09:52:52AM +0200, Otto Moerbeek wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > This is work in progress. I have to think if the flags to kdump > > > > > > > > I'm > > > > > > > > introducing should be two or a single one. > > > > > > > > > > > > > > > > Currently, malloc.c can be compiled with MALLOC_STATS defined. > > > > > > > > If run > > > > > > > > with option D it dumps its state to a malloc.out file at exit. > > > > > > > > This > > > > > > > > state can be used to find leaks amongst other things. > > > > > > > > > > > > > > > > This is not ideal for pledged processes, as they often have no > > > > > > > > way to > > > > > > > > write files. > > > > > > > > > > > > > > > > This changes malloc to use utrace(2) for that. > > > > > > > > > > > > > > > > As kdump has no nice way to show those lines without all extras > > > > > > > > it > > > > > > > > normally shows, so add two options to it to just show the lines. > > > > > > > > > > > > > > > > To use, compile and install libc with MALLOC_STATS defined. > > > > > > > > > > > > > > > > Run : > > > > > > > > > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > > > > > ... > > > > > > > > $ kdump -hu > > > > > > > > > > > > > > > > Feedback appreciated. > > > > > > > > > > > > I can't really comment on malloc(3) stuff, but I agree that > > > > > > utrace(2) is a good > > > > > > way to get information outside a pledged process. > > > > > > > > > > > > I tend to think it is safe to use it, as the pledged process need > > > > > > cooperation > > > > > > from outside to exfiltrate informations (a process with permission > > > > > > to call > > > > > > ktrace(2) on this pid). > > > > > > > > > > > > Please note it is a somehow generic problem: at least profiled > > > > > > processes would > > > > > > also get advantage of using it. > > > > > > > > > > > > > > > > > > Regarding kdump options, I think that -u option should implies -h > > > > > > (no header). > > > > > > > > > > > > Does it would make sens to considere a process using utrace(2) with > > > > > > several > > > > > > interleaved records for different sources ? A process with > > > > > > MALLOC_OPTIONS=D and > > > > > > profiling enabled for example ? An (another) option on kdump to > > > > > > filter on utrace > > > > > > label would be useful in such case, or have -u mandate a label to > > > > > > filter on. > > > > > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > > > $ kdump -u mallocdumpline > > > > > > > > > > > > and for profiling: > > > > > > > > > > > > $ kdump -u profil > gmon.out > > > > > > $ gprof your_program gmon.out > > > > > > > > > > > > Thanks. > > > > > > > > > > Thanks! Your suggestions make a lot of sense. I'll rework the kdump > > > > > part to make it more flexable for different purposes. > > > > > > > > Anothew aspect of safety is the information availble in the heap >
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Sun, Apr 09, 2023 at 12:17:35PM +0200, Otto Moerbeek wrote: > On Sun, Apr 09, 2023 at 10:08:25AM +0200, Claudio Jeker wrote: > > > On Sun, Apr 09, 2023 at 09:15:12AM +0200, Otto Moerbeek wrote: > > > On Sun, Apr 09, 2023 at 08:20:43AM +0200, Otto Moerbeek wrote: > > > > > > > On Sun, Apr 09, 2023 at 07:53:31AM +0200, Sebastien Marie wrote: > > > > > > > > > On Fri, Apr 07, 2023 at 09:52:52AM +0200, Otto Moerbeek wrote: > > > > > > > Hi, > > > > > > > > > > > > > > This is work in progress. I have to think if the flags to kdump > > > > > > > I'm > > > > > > > introducing should be two or a single one. > > > > > > > > > > > > > > Currently, malloc.c can be compiled with MALLOC_STATS defined. If > > > > > > > run > > > > > > > with option D it dumps its state to a malloc.out file at exit. > > > > > > > This > > > > > > > state can be used to find leaks amongst other things. > > > > > > > > > > > > > > This is not ideal for pledged processes, as they often have no > > > > > > > way to > > > > > > > write files. > > > > > > > > > > > > > > This changes malloc to use utrace(2) for that. > > > > > > > > > > > > > > As kdump has no nice way to show those lines without all extras it > > > > > > > normally shows, so add two options to it to just show the lines. > > > > > > > > > > > > > > To use, compile and install libc with MALLOC_STATS defined. > > > > > > > > > > > > > > Run : > > > > > > > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > > > > ... > > > > > > > $ kdump -hu > > > > > > > > > > > > > > Feedback appreciated. > > > > > > > > > > I can't really comment on malloc(3) stuff, but I agree that utrace(2) > > > > > is a good > > > > > way to get information outside a pledged process. > > > > > > > > > > I tend to think it is safe to use it, as the pledged process need > > > > > cooperation > > > > > from outside to exfiltrate informations (a process with permission to > > > > > call > > > > > ktrace(2) on this pid). > > > > > > > > > > Please note it is a somehow generic problem: at least profiled > > > > > processes would > > > > > also get advantage of using it. > > > > > > > > > > > > > > > Regarding kdump options, I think that -u option should implies -h (no > > > > > header). > > > > > > > > > > Does it would make sens to considere a process using utrace(2) with > > > > > several > > > > > interleaved records for different sources ? A process with > > > > > MALLOC_OPTIONS=D and > > > > > profiling enabled for example ? An (another) option on kdump to > > > > > filter on utrace > > > > > label would be useful in such case, or have -u mandate a label to > > > > > filter on. > > > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > > $ kdump -u mallocdumpline > > > > > > > > > > and for profiling: > > > > > > > > > > $ kdump -u profil > gmon.out > > > > > $ gprof your_program gmon.out > > > > > > > > > > Thanks. > > > > > > > > Thanks! Your suggestions make a lot of sense. I'll rework the kdump > > > > part to make it more flexable for different purposes. > > > > > > Anothew aspect of safety is the information availble in the heap > > > itself. I'm pretty sure the addresses of call sites into malloc are > > > interesting to attackers. To prevent a program having access to those > > > (even if they are stored inside the malloc meta data that is not > > > directly accesible to a program), I'm making sure the recording only > > > takes place if malloc option D is used. > > > > > > This is included in a diff with the kdump changes and a few other > > > things below. I'm also
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Sun, Apr 09, 2023 at 10:08:25AM +0200, Claudio Jeker wrote: > On Sun, Apr 09, 2023 at 09:15:12AM +0200, Otto Moerbeek wrote: > > On Sun, Apr 09, 2023 at 08:20:43AM +0200, Otto Moerbeek wrote: > > > > > On Sun, Apr 09, 2023 at 07:53:31AM +0200, Sebastien Marie wrote: > > > > > > > On Fri, Apr 07, 2023 at 09:52:52AM +0200, Otto Moerbeek wrote: > > > > > > Hi, > > > > > > > > > > > > This is work in progress. I have to think if the flags to kdump I'm > > > > > > introducing should be two or a single one. > > > > > > > > > > > > Currently, malloc.c can be compiled with MALLOC_STATS defined. If > > > > > > run > > > > > > with option D it dumps its state to a malloc.out file at exit. This > > > > > > state can be used to find leaks amongst other things. > > > > > > > > > > > > This is not ideal for pledged processes, as they often have no way > > > > > > to > > > > > > write files. > > > > > > > > > > > > This changes malloc to use utrace(2) for that. > > > > > > > > > > > > As kdump has no nice way to show those lines without all extras it > > > > > > normally shows, so add two options to it to just show the lines. > > > > > > > > > > > > To use, compile and install libc with MALLOC_STATS defined. > > > > > > > > > > > > Run : > > > > > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > > > ... > > > > > > $ kdump -hu > > > > > > > > > > > > Feedback appreciated. > > > > > > > > I can't really comment on malloc(3) stuff, but I agree that utrace(2) > > > > is a good > > > > way to get information outside a pledged process. > > > > > > > > I tend to think it is safe to use it, as the pledged process need > > > > cooperation > > > > from outside to exfiltrate informations (a process with permission to > > > > call > > > > ktrace(2) on this pid). > > > > > > > > Please note it is a somehow generic problem: at least profiled > > > > processes would > > > > also get advantage of using it. > > > > > > > > > > > > Regarding kdump options, I think that -u option should implies -h (no > > > > header). > > > > > > > > Does it would make sens to considere a process using utrace(2) with > > > > several > > > > interleaved records for different sources ? A process with > > > > MALLOC_OPTIONS=D and > > > > profiling enabled for example ? An (another) option on kdump to filter > > > > on utrace > > > > label would be useful in such case, or have -u mandate a label to > > > > filter on. > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > $ kdump -u mallocdumpline > > > > > > > > and for profiling: > > > > > > > > $ kdump -u profil > gmon.out > > > > $ gprof your_program gmon.out > > > > > > > > Thanks. > > > > > > Thanks! Your suggestions make a lot of sense. I'll rework the kdump > > > part to make it more flexable for different purposes. > > > > Anothew aspect of safety is the information availble in the heap > > itself. I'm pretty sure the addresses of call sites into malloc are > > interesting to attackers. To prevent a program having access to those > > (even if they are stored inside the malloc meta data that is not > > directly accesible to a program), I'm making sure the recording only > > takes place if malloc option D is used. > > > > This is included in a diff with the kdump changes and a few other > > things below. I'm also compiling with MALLOC_STATS if !SMALL. > > > > usage is now: > > > > $ MALLOC_OPTIONS=D ktrace -tu a.out > > $ kdump -u malloc > > > > I would prefer if every utrace() call is a full line (in other words ulog > should be line buffered). It makes the regular kdump output more useable. > Right now you depend on kdump -u to put the lines back together. Right. Done line buffering in the new diff below. > Whenever I used utrace() I normally passed binary objects to the call so I > could enrich the ktra
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Sun, Apr 09, 2023 at 08:20:43AM +0200, Otto Moerbeek wrote: > On Sun, Apr 09, 2023 at 07:53:31AM +0200, Sebastien Marie wrote: > > > On Fri, Apr 07, 2023 at 09:52:52AM +0200, Otto Moerbeek wrote: > > > > Hi, > > > > > > > > This is work in progress. I have to think if the flags to kdump I'm > > > > introducing should be two or a single one. > > > > > > > > Currently, malloc.c can be compiled with MALLOC_STATS defined. If run > > > > with option D it dumps its state to a malloc.out file at exit. This > > > > state can be used to find leaks amongst other things. > > > > > > > > This is not ideal for pledged processes, as they often have no way to > > > > write files. > > > > > > > > This changes malloc to use utrace(2) for that. > > > > > > > > As kdump has no nice way to show those lines without all extras it > > > > normally shows, so add two options to it to just show the lines. > > > > > > > > To use, compile and install libc with MALLOC_STATS defined. > > > > > > > > Run : > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > ... > > > > $ kdump -hu > > > > > > > > Feedback appreciated. > > > > I can't really comment on malloc(3) stuff, but I agree that utrace(2) is a > > good > > way to get information outside a pledged process. > > > > I tend to think it is safe to use it, as the pledged process need > > cooperation > > from outside to exfiltrate informations (a process with permission to call > > ktrace(2) on this pid). > > > > Please note it is a somehow generic problem: at least profiled processes > > would > > also get advantage of using it. > > > > > > Regarding kdump options, I think that -u option should implies -h (no > > header). > > > > Does it would make sens to considere a process using utrace(2) with several > > interleaved records for different sources ? A process with MALLOC_OPTIONS=D > > and > > profiling enabled for example ? An (another) option on kdump to filter on > > utrace > > label would be useful in such case, or have -u mandate a label to filter on. > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > $ kdump -u mallocdumpline > > > > and for profiling: > > > > $ kdump -u profil > gmon.out > > $ gprof your_program gmon.out > > > > Thanks. > > Thanks! Your suggestions make a lot of sense. I'll rework the kdump > part to make it more flexable for different purposes. Anothew aspect of safety is the information availble in the heap itself. I'm pretty sure the addresses of call sites into malloc are interesting to attackers. To prevent a program having access to those (even if they are stored inside the malloc meta data that is not directly accesible to a program), I'm making sure the recording only takes place if malloc option D is used. This is included in a diff with the kdump changes and a few other things below. I'm also compiling with MALLOC_STATS if !SMALL. usage is now: $ MALLOC_OPTIONS=D ktrace -tu a.out $ kdump -u malloc -Otto Index: lib/libc/stdlib/malloc.3 === RCS file: /home/cvs/src/lib/libc/stdlib/malloc.3,v retrieving revision 1.130 diff -u -p -r1.130 malloc.3 --- lib/libc/stdlib/malloc.31 Apr 2023 18:47:51 - 1.130 +++ lib/libc/stdlib/malloc.39 Apr 2023 07:08:20 - @@ -284,10 +284,13 @@ If it has been corrupted, the process is .It Cm D .Dq Dump . .Fn malloc -will dump statistics to the file -.Pa ./malloc.out , -if it already exists, +will dump statistics using +.Xr utrace 2 at exit. +To record the dump: +.Dl $ MALLOC_OPTIONS=D ktrace -tu program ... +To view the dump: +.Dl $ kdump -u malloc ... This option requires the library to have been compiled with -DMALLOC_STATS in order to have any effect. .It Cm F Index: lib/libc/stdlib/malloc.c === RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v retrieving revision 1.280 diff -u -p -r1.280 malloc.c --- lib/libc/stdlib/malloc.c5 Apr 2023 06:25:38 - 1.280 +++ lib/libc/stdlib/malloc.c9 Apr 2023 07:08:20 - @@ -1,6 +1,6 @@ /* $OpenBSD: malloc.c,v 1.280 2023/04/05 06:25:38 otto Exp $ */ /* - * Copyright (c) 2008, 2010, 2011, 2016 Otto Moerbeek + * Copyright (c) 2008, 2010, 2011, 2016, 2023 Otto Moerbeek * Copyright (c) 2012 Matthew Dempsky * Copyright (c) 2008 Damien Miller * Copyright (c) 2000 Poul-Henning Kamp @@ -23,7 +23,9
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Sun, Apr 09, 2023 at 07:53:31AM +0200, Sebastien Marie wrote: > On Fri, Apr 07, 2023 at 09:52:52AM +0200, Otto Moerbeek wrote: > > > Hi, > > > > > > This is work in progress. I have to think if the flags to kdump I'm > > > introducing should be two or a single one. > > > > > > Currently, malloc.c can be compiled with MALLOC_STATS defined. If run > > > with option D it dumps its state to a malloc.out file at exit. This > > > state can be used to find leaks amongst other things. > > > > > > This is not ideal for pledged processes, as they often have no way to > > > write files. > > > > > > This changes malloc to use utrace(2) for that. > > > > > > As kdump has no nice way to show those lines without all extras it > > > normally shows, so add two options to it to just show the lines. > > > > > > To use, compile and install libc with MALLOC_STATS defined. > > > > > > Run : > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > ... > > > $ kdump -hu > > > > > > Feedback appreciated. > > I can't really comment on malloc(3) stuff, but I agree that utrace(2) is a > good > way to get information outside a pledged process. > > I tend to think it is safe to use it, as the pledged process need cooperation > from outside to exfiltrate informations (a process with permission to call > ktrace(2) on this pid). > > Please note it is a somehow generic problem: at least profiled processes > would > also get advantage of using it. > > > Regarding kdump options, I think that -u option should implies -h (no header). > > Does it would make sens to considere a process using utrace(2) with several > interleaved records for different sources ? A process with MALLOC_OPTIONS=D > and > profiling enabled for example ? An (another) option on kdump to filter on > utrace > label would be useful in such case, or have -u mandate a label to filter on. > > $ MALLOC_OPTIONS=D ktrace -tu your_program > $ kdump -u mallocdumpline > > and for profiling: > > $ kdump -u profil > gmon.out > $ gprof your_program gmon.out > > Thanks. Thanks! Your suggestions make a lot of sense. I'll rework the kdump part to make it more flexable for different purposes. -Otto > -- > Sebastien Marie
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Wed, Apr 05, 2023 at 10:54:19AM +0200, Otto Moerbeek wrote:
> Hi,
>
> This is work in progress. I have to think if the flags to kdump I'm
> introducing should be two or a single one.
>
> Currently, malloc.c can be compiled with MALLOC_STATS defined. If run
> with option D it dumps its state to a malloc.out file at exit. This
> state can be used to find leaks amongst other things.
>
> This is not ideal for pledged processes, as they often have no way to
> write files.
>
> This changes malloc to use utrace(2) for that.
>
> As kdump has no nice way to show those lines without all extras it
> normally shows, so add two options to it to just show the lines.
>
> To use, compile and install libc with MALLOC_STATS defined.
>
> Run :
>
> $ MALLOC_OPTIONS=D ktrace -tu your_program
> ...
> $ kdump -hu
>
> Feedback appreciated.
Second iteration. Main difference: docs and the leak report is greatly
enhanced by adding info that can be fed to addr2line. Exmaple:
...
Leak report:
f sum #avg
0x9fd0eb58abb 40960 10 4096 addr2line -e ./a.out 0x1abb
0x9fd0eb58ae98192 1 8192 addr2line -e ./a.out 0x1ae9
0x9ffa92d9daf 65536 1 65536 addr2line -e /usr/lib/libc.so.97.0
0x87daf
$ addr2line -e /usr/lib/libc.so.97.0 0x87daf
/usr/src/lib/libc/stdio/makebuf.c:62
Note that only the top caller is reported. I once wrote code to record
more stack frames, but I do not think I want that in. More elaborate
memory allocation analysis is better done by external tools or
instrumentation wrappers.
But even them, the current report is very useful IMO. It might be even
usefull enough to compile it always #ifndef SMALL. The runtime
overhead (if malloc option D is not used) is small. I'll be thinking
about possible reasons not to compile it in by default.
-Otto
Index: lib/libc/stdlib/malloc.3
===
RCS file: /home/cvs/src/lib/libc/stdlib/malloc.3,v
retrieving revision 1.130
diff -u -p -r1.130 malloc.3
--- lib/libc/stdlib/malloc.31 Apr 2023 18:47:51 - 1.130
+++ lib/libc/stdlib/malloc.37 Apr 2023 07:43:22 -
@@ -284,10 +284,13 @@ If it has been corrupted, the process is
.It Cm D
.Dq Dump .
.Fn malloc
-will dump statistics to the file
-.Pa ./malloc.out ,
-if it already exists,
+will dump statistics using
+.Xr utrace 2
at exit.
+To record the dump:
+.Dl $ MALLOC_OPTIONS=D ktrace -tu program ...
+To view the dump:
+.Dl $ kdump -hu ...
This option requires the library to have been compiled with -DMALLOC_STATS in
order to have any effect.
.It Cm F
Index: lib/libc/stdlib/malloc.c
===
RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.280
diff -u -p -r1.280 malloc.c
--- lib/libc/stdlib/malloc.c5 Apr 2023 06:25:38 - 1.280
+++ lib/libc/stdlib/malloc.c7 Apr 2023 07:43:22 -
@@ -23,7 +23,7 @@
* can buy me a beer in return. Poul-Henning Kamp
*/
-/* #define MALLOC_STATS */
+#define MALLOC_STATS
#include
#include
@@ -39,8 +39,10 @@
#include
#ifdef MALLOC_STATS
+#include
#include
-#include
+#include
+#include
#endif
#include "thread_private.h"
@@ -243,10 +245,8 @@ static __dead void wrterror(struct dir_i
__attribute__((__format__ (printf, 2, 3)));
#ifdef MALLOC_STATS
-void malloc_dump(int, int, struct dir_info *);
+void malloc_dump(void);
PROTO_NORMAL(malloc_dump);
-void malloc_gdump(int);
-PROTO_NORMAL(malloc_gdump);
static void malloc_exit(void);
#define CALLER __builtin_return_address(0)
#else
@@ -319,7 +319,7 @@ wrterror(struct dir_info *d, char *msg,
#ifdef MALLOC_STATS
if (mopts.malloc_stats)
- malloc_gdump(STDERR_FILENO);
+ malloc_dump();
#endif /* MALLOC_STATS */
errno = saved_errno;
@@ -2225,6 +2225,21 @@ aligned_alloc(size_t alignment, size_t s
#ifdef MALLOC_STATS
+static void
+ulog(const char *format, ...)
+{
+ va_list ap;
+ char buf[100];
+ int len;
+
+ va_start(ap, format);
+ len = vsnprintf(buf, sizeof(buf), format, ap);
+ if (len > (int)sizeof(buf))
+ len = sizeof(buf);
+ utrace("mallocdumpline", buf, len);
+ va_end(ap);
+}
+
struct malloc_leak {
void *f;
size_t total_size;
@@ -2280,21 +2295,32 @@ putleakinfo(void *f, size_t sz, int cnt)
static struct malloc_leak *malloc_leaks;
static void
-dump_leaks(int fd)
+dump_leaks(void)
{
struct leaknode *p;
unsigned int i = 0;
- dprintf(fd, "Leak report\n");
- dprintf(fd, " f sum #avg\n");
+ ulog("Leak report:\n");
+ ulog(" f sum #avg\n");
/* XXX only one page of summary */
if (malloc_leaks
MALLOC_STATS: dump internal state and leak info via utrace(2)
Hi,
This is work in progress. I have to think if the flags to kdump I'm
introducing should be two or a single one.
Currently, malloc.c can be compiled with MALLOC_STATS defined. If run
with option D it dumps its state to a malloc.out file at exit. This
state can be used to find leaks amongst other things.
This is not ideal for pledged processes, as they often have no way to
write files.
This changes malloc to use utrace(2) for that.
As kdump has no nice way to show those lines without all extras it
normally shows, so add two options to it to just show the lines.
To use, compile and install libc with MALLOC_STATS defined.
Run :
$ MALLOC_OPTIONS=D ktrace -tu your_program
...
$ kdump -hu
Feedback appreciated.
-Otto
Index: lib/libc/stdlib/malloc.c
===
RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.280
diff -u -p -r1.280 malloc.c
--- lib/libc/stdlib/malloc.c5 Apr 2023 06:25:38 - 1.280
+++ lib/libc/stdlib/malloc.c5 Apr 2023 08:23:42 -
@@ -23,7 +23,7 @@
* can buy me a beer in return. Poul-Henning Kamp
*/
-/* #define MALLOC_STATS */
+#define MALLOC_STATS
#include
#include
@@ -39,7 +39,9 @@
#include
#ifdef MALLOC_STATS
+#include
#include
+#include
#include
#endif
@@ -243,10 +245,8 @@ static __dead void wrterror(struct dir_i
__attribute__((__format__ (printf, 2, 3)));
#ifdef MALLOC_STATS
-void malloc_dump(int, int, struct dir_info *);
+void malloc_dump(void);
PROTO_NORMAL(malloc_dump);
-void malloc_gdump(int);
-PROTO_NORMAL(malloc_gdump);
static void malloc_exit(void);
#define CALLER __builtin_return_address(0)
#else
@@ -319,7 +319,7 @@ wrterror(struct dir_info *d, char *msg,
#ifdef MALLOC_STATS
if (mopts.malloc_stats)
- malloc_gdump(STDERR_FILENO);
+ malloc_dump();
#endif /* MALLOC_STATS */
errno = saved_errno;
@@ -2225,6 +2225,21 @@ aligned_alloc(size_t alignment, size_t s
#ifdef MALLOC_STATS
+static void
+ulog(const char *format, ...)
+{
+ va_list ap;
+ char buf[100];
+ int len;
+
+ va_start(ap, format);
+ len = vsnprintf(buf, sizeof(buf), format, ap);
+ if (len > (int)sizeof(buf))
+ len = sizeof(buf);
+ utrace("mallocdumpline", buf, len);
+ va_end(ap);
+}
+
struct malloc_leak {
void *f;
size_t total_size;
@@ -2280,20 +2295,20 @@ putleakinfo(void *f, size_t sz, int cnt)
static struct malloc_leak *malloc_leaks;
static void
-dump_leaks(int fd)
+dump_leaks(void)
{
struct leaknode *p;
unsigned int i = 0;
- dprintf(fd, "Leak report\n");
- dprintf(fd, " f sum #avg\n");
+ ulog("Leak report\n");
+ ulog(" f sum #avg\n");
/* XXX only one page of summary */
if (malloc_leaks == NULL)
malloc_leaks = MMAP(MALLOC_PAGESIZE, 0);
if (malloc_leaks != MAP_FAILED)
memset(malloc_leaks, 0, MALLOC_PAGESIZE);
RBT_FOREACH(p, leaktree, ) {
- dprintf(fd, "%18p %7zu %6u %6zu\n", p->d.f,
+ ulog("%18p %7zu %6u %6zu\n", p->d.f,
p->d.total_size, p->d.count, p->d.total_size / p->d.count);
if (malloc_leaks == MAP_FAILED ||
i >= MALLOC_PAGESIZE / sizeof(struct malloc_leak))
@@ -2306,10 +2321,10 @@ dump_leaks(int fd)
}
static void
-dump_chunk(int fd, struct chunk_info *p, void *f, int fromfreelist)
+dump_chunk(struct chunk_info *p, void *f, int fromfreelist)
{
while (p != NULL) {
- dprintf(fd, "chunk %18p %18p %4zu %d/%d\n",
+ ulog("chunk %18p %18p %4zu %d/%d\n",
p->page, ((p->bits[0] & 1) ? NULL : f),
B2SIZE(p->bucket), p->free, p->total);
if (!fromfreelist) {
@@ -2325,17 +2340,17 @@ dump_chunk(int fd, struct chunk_info *p,
}
p = LIST_NEXT(p, entries);
if (p != NULL)
- dprintf(fd, "");
+ ulog("");
}
}
static void
-dump_free_chunk_info(int fd, struct dir_info *d)
+dump_free_chunk_info(struct dir_info *d)
{
int i, j, count;
struct chunk_info *p;
- dprintf(fd, "Free chunk structs:\n");
+ ulog("Free chunk structs:\n");
for (i = 0; i <= BUCKETS; i++) {
count = 0;
LIST_FOREACH(p, >chunk_info_list[i], entries)
@@ -2345,99 +2360,97 @@ dump_free_chunk_info(int fd, struct dir_
if (p == NULL && count == 0)
continue;
if (j == 0)
- dprintf(fd, "%3d) %3d ", i, count);
+ ulog("%3d) %3d ", i, count);
else
- dprintf(fd, " ");
+
malloc: variation in junk locations
Hi,
by default an allocation isn't fully written with junk bytes, only at
certain spots. This introduces variations in the spot, so we have a
bigger chance of catching write-after-frees in specific spots.
After a remark from jsing@ that variation would be nice to have for
this.
-Otto
Index: stdlib/malloc.c
===
RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.279
diff -u -p -r1.279 malloc.c
--- stdlib/malloc.c 1 Apr 2023 18:47:51 - 1.279
+++ stdlib/malloc.c 1 Apr 2023 19:06:56 -
@@ -221,6 +221,7 @@ struct malloc_readonly {
u_int chunk_canaries; /* use canaries after chunks? */
int internal_funcs; /* use better recallocarray/freezero? */
u_int def_maxcache; /* free pages we cache */
+ u_int junk_loc; /* variation in location of junk */
size_t malloc_guard; /* use guard pages after allocations? */
#ifdef MALLOC_STATS
int malloc_stats; /* dump statistics at end */
@@ -493,6 +494,7 @@ omalloc_init(void)
while ((mopts.malloc_canary = arc4random()) == 0)
;
+ mopts.junk_loc = arc4random();
if (mopts.chunk_canaries)
do {
mopts.chunk_canaries = arc4random();
@@ -676,7 +678,9 @@ junk_free(int junk, void *p, size_t sz)
if (step == 0)
step = 1;
}
- for (i = 0; i < sz; i += step)
+ /* Do not always put the free junk bytes in the same spot.
+ There is modulo bias here, but we ignore that. */
+ for (i = mopts.junk_loc % step; i < sz; i += step)
lp[i] = SOME_FREEJUNK_ULL;
}
@@ -696,7 +700,8 @@ validate_junk(struct dir_info *pool, voi
if (step == 0)
step = 1;
}
- for (i = 0; i < sz; i += step) {
+ /* see junk_free */
+ for (i = mopts.junk_loc % step; i < sz; i += step) {
if (lp[i] != SOME_FREEJUNK_ULL)
wrterror(pool, "write after free %p", p);
}
malloc: write after free check in more cases
Hi,
this came up after a remark by tb@; it does a more thorough check of
the chunks in the delay list if malloc option F (which is included in
S).
Catches way more use-after-free's as otherwise we'll have to wait
until a chunk is actively re-used. With the new bucket code that might
take longer (or not happen at all because the program terminates). The
catch also happens closer (in time) to the actual write after free.
That might help finding the root cause.
-Otto
Index: stdlib/malloc.3
===
RCS file: /home/cvs/src/lib/libc/stdlib/malloc.3,v
retrieving revision 1.129
diff -u -p -r1.129 malloc.3
--- stdlib/malloc.3 31 Mar 2022 17:27:16 - 1.129
+++ stdlib/malloc.3 30 Mar 2023 14:47:19 -
@@ -293,7 +293,8 @@ order to have any effect.
.It Cm F
.Dq Freecheck .
Enable more extensive double free and use after free detection.
-All chunks in the delayed free list will be checked for double frees.
+All chunks in the delayed free list will be checked for double frees and
+write after frees.
Unused pages on the freelist are read and write protected to
cause a segmentation fault upon access.
.It Cm G
Index: stdlib/malloc.c
===
RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.278
diff -u -p -r1.278 malloc.c
--- stdlib/malloc.c 25 Mar 2023 15:22:06 - 1.278
+++ stdlib/malloc.c 30 Mar 2023 14:47:19 -
@@ -1554,11 +1554,25 @@ ofree(struct dir_info **argpool, void *p
find_chunknum(pool, info, p, mopts.chunk_canaries);
if (mopts.malloc_freecheck) {
- for (i = 0; i <= MALLOC_DELAYED_CHUNK_MASK; i++)
- if (p == pool->delayed_chunks[i])
+ for (i = 0; i <= MALLOC_DELAYED_CHUNK_MASK; i++) {
+ tmp = pool->delayed_chunks[i];
+ if (tmp == p)
wrterror(pool,
"double free %p", p);
+ if (tmp != NULL) {
+ size_t tmpsz;
+
+ r = find(pool, tmp);
+ if (r == NULL)
+ wrterror(pool,
+ "bogus pointer ("
+ "double free?) %p", tmp);
+ REALSIZE(tmpsz, r);
+ validate_junk(pool, tmp, tmpsz);
+ }
+ }
}
+
if (clear && argsz > 0)
explicit_bzero(p, argsz);
junk_free(pool->malloc_junk, p, sz);
@@ -1574,8 +1588,10 @@ ofree(struct dir_info **argpool, void *p
if (r == NULL)
wrterror(pool,
"bogus pointer (double free?) %p", p);
- REALSIZE(sz, r);
- validate_junk(pool, p, sz);
+ if (!mopts.malloc_freecheck) {
+ REALSIZE(sz, r);
+ validate_junk(pool, p, sz);
+ }
free_bytes(pool, r, p);
}
}
recv.c: consistency wrt NULL for pointers
Hi,
Last arg also is a pointer, so pass NULL.
-Otto
Index: recv.c
===
RCS file: /home/cvs/src/lib/libc/net/recv.c,v
retrieving revision 1.6
diff -u -p -r1.6 recv.c
--- recv.c 4 Oct 2015 07:17:27 - 1.6
+++ recv.c 23 Mar 2023 06:00:37 -
@@ -36,6 +36,6 @@
ssize_t
recv(int s, void *buf, size_t len, int flags)
{
- return (recvfrom(s, buf, len, flags, NULL, 0));
+ return (recvfrom(s, buf, len, flags, NULL, NULL));
}
DEF_WEAK(recv);
Re: malloc: change chunk sizes to be multiple of 16 instead of power of 2
On Tue, Mar 07, 2023 at 09:02:04AM +0100, Theo Buehler wrote: > > So here's the dif with the fix. > > The new diff went through an amd64 bulk without fallout and also works > fine on some dev machines. No noticeable performance impact for my > workloads. > > It also reads fine to me (ok tb). > > Do you want it to make it into the release or can/should it wait? > Either way, it would probably be good for it to see more eyes and > tests. Thanks for testing and reviewing. I think I won't push it into the upcoming release. This should benefit from a large part of the release cycle for testing and it's already quite late in the current cycle. -Otto
Re: malloc: change chunk sizes to be multiple of 16 instead of power of 2
On Wed, Mar 01, 2023 at 08:49:56AM +0100, Theo Buehler wrote:
> On Wed, Mar 01, 2023 at 08:39:08AM +0100, Otto Moerbeek wrote:
> > On Wed, Mar 01, 2023 at 08:31:47AM +0100, Theo Buehler wrote:
> >
> > > On Tue, Feb 28, 2023 at 05:52:28PM +0100, Otto Moerbeek wrote:
> > > > Second iteration.
> > > >
> > > > Gain back performance by allocation chunk_info pages in a bundle, and
> > > > use less buckets is !malloc option S. The chunk sizes used are 16, 32,
> > > > 48, 64, 80, 96, 112, 128, 160, 192, 224, 256, 320, 384, 448, 512, 640,
> > > > 768, 896, 1024, 1280, 1536, 1792, 2048 (and a few more for sparc84
> > > > with it's 8k sized pages and loongson with it's 16k pages).
> > > >
> > > > If malloc option S (or rather cache size 0) is used we use strict
> > > > multiple of 16 buckets, to get as many buckets as possible.
> > > >
> > > > See the find_bucket() and bin_of() functions. Thanks to Tony Finch for
> > > > pointing me to code to compute nice bucket sizes.
> > > >
> > > > I think this is ready for review and wide testing.
> > >
> > > Two vala-based ports, graphics/birdfont and productivity/minder, run out
> > > of memory when attempting to build them with this diff (and its previous
> > > version) on both amd64 and arm64:
> > >
> > > ***MEMORY-ERROR***: valac[93681]: GSlice: failed to allocate 2032 bytes
> > > (alignment: 2048): Cannot allocate memory
> >
> > Thanks, this smells like a bug in the aligned mem case.
> >
> > + pof2 = 1 << MALLOC_MINSIZE;
> >
> > should be
> >
> > + pof2 = MALLOC_MINSIZE;
> >
> > By the looks of it. I'll get back to this.
>
> I can confirm that changing this fixes this issue with both ports on
> amd64 and arm64.
So here's the dif with the fix.
-Otto
Index: stdlib/malloc.c
===
RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.277
diff -u -p -r1.277 malloc.c
--- stdlib/malloc.c 27 Feb 2023 06:47:54 - 1.277
+++ stdlib/malloc.c 1 Mar 2023 09:14:24 -
@@ -67,6 +67,11 @@
#define MALLOC_CHUNK_LISTS 4
#define CHUNK_CHECK_LENGTH 32
+#define B2SIZE(b) ((b) * MALLOC_MINSIZE)
+#define B2ALLOC(b) ((b) == 0 ? MALLOC_MINSIZE : \
+ (b) * MALLOC_MINSIZE)
+#define BUCKETS(MALLOC_MAXCHUNK / MALLOC_MINSIZE)
+
/*
* We move allocations between half a page and a whole page towards the end,
* subject to alignment constraints. This is the extra headroom we allow.
@@ -144,9 +149,9 @@ struct dir_info {
int mutex;
int malloc_mt; /* multi-threaded mode? */
/* lists of free chunk info structs */
- struct chunk_head chunk_info_list[MALLOC_MAXSHIFT + 1];
+ struct chunk_head chunk_info_list[BUCKETS + 1];
/* lists of chunks with free slots */
- struct chunk_head chunk_dir[MALLOC_MAXSHIFT + 1][MALLOC_CHUNK_LISTS];
+ struct chunk_head chunk_dir[BUCKETS + 1][MALLOC_CHUNK_LISTS];
/* delayed free chunk slots */
void *delayed_chunks[MALLOC_DELAYED_CHUNK_MASK + 1];
u_char rbytes[32]; /* random bytes */
@@ -155,6 +160,8 @@ struct dir_info {
size_t bigcache_used;
size_t bigcache_size;
struct bigcache *bigcache;
+ void *chunk_pages;
+ size_t chunk_pages_used;
#ifdef MALLOC_STATS
size_t inserts;
size_t insert_collisions;
@@ -195,8 +202,7 @@ struct chunk_info {
LIST_ENTRY(chunk_info) entries;
void *page; /* pointer to the page */
u_short canary;
- u_short size; /* size of this page's chunks */
- u_short shift; /* how far to shift for this size */
+ u_short bucket;
u_short free; /* how many free chunks */
u_short total; /* how many chunks */
u_short offset; /* requested size table offset */
@@ -247,11 +253,11 @@ static void malloc_exit(void);
#endif
/* low bits of r->p determine size: 0 means >= page size and r->size holding
- * real size, otherwise low bits are a shift count, or 1 for malloc(0)
+ * real size, otherwise low bits is the bucket + 1
*/
#define REALSIZE(sz, r)\
(sz) = (uintptr_t)(r)->p & MALLOC_PAGEMASK, \
- (sz) = ((sz) == 0 ? (r)->size : ((sz) == 1 ? 0 : (1 << ((sz)-1
+ (sz) = ((sz) == 0 ? (r)->
Re: malloc: change chunk sizes to be multiple of 16 instead of power of 2
On Wed, Mar 01, 2023 at 08:31:47AM +0100, Theo Buehler wrote: > On Tue, Feb 28, 2023 at 05:52:28PM +0100, Otto Moerbeek wrote: > > Second iteration. > > > > Gain back performance by allocation chunk_info pages in a bundle, and > > use less buckets is !malloc option S. The chunk sizes used are 16, 32, > > 48, 64, 80, 96, 112, 128, 160, 192, 224, 256, 320, 384, 448, 512, 640, > > 768, 896, 1024, 1280, 1536, 1792, 2048 (and a few more for sparc84 > > with it's 8k sized pages and loongson with it's 16k pages). > > > > If malloc option S (or rather cache size 0) is used we use strict > > multiple of 16 buckets, to get as many buckets as possible. > > > > See the find_bucket() and bin_of() functions. Thanks to Tony Finch for > > pointing me to code to compute nice bucket sizes. > > > > I think this is ready for review and wide testing. > > Two vala-based ports, graphics/birdfont and productivity/minder, run out > of memory when attempting to build them with this diff (and its previous > version) on both amd64 and arm64: > > ***MEMORY-ERROR***: valac[93681]: GSlice: failed to allocate 2032 bytes > (alignment: 2048): Cannot allocate memory Thanks, this smells like a bug in the aligned mem case. + pof2 = 1 << MALLOC_MINSIZE; should be + pof2 = MALLOC_MINSIZE; By the looks of it. I'll get back to this. -Otto > > Abort trap (core dumped) > > To be able to build birdfont with PORTS_PRIVSEP = Yes, I had to bump > _pbuild's datasize-cur to 15G, while 14G was not enough. That's nearly > double the current default. On amd64 without this diff, birdfont builds > comfortably with a datasize-cur of 1G. > > birdfont may be easier to investigate since the error happens early in > the build. You can get there relatively quickly by doing > > cd /usr/ports/graphics/birdfont > doas pkg_add birdfont > make FETCH_PACKAGES= prepare > make > > Not sure if the top of the trace is of much use. Here it is: > > #0 thrkill () at /tmp/-:3 > #1 0x486dd8c0aacac468 in ?? () > #2 0x0c3a34319d0e in _libc_abort () at > /usr/src/lib/libc/stdlib/abort.c:51 > #3 0x0c39b735724b in mem_error () from > /usr/local/lib/libglib-2.0.so.4201.9 > #4 0x0c39b735604f in slab_allocator_alloc_chunk () from > /usr/local/lib/libglib-2.0.so.4201.9 > #5 0x0c39b7355a95 in g_slice_alloc () from > /usr/local/lib/libglib-2.0.so.4201.9 > #6 0x0c39b735606e in g_slice_alloc0 () from > /usr/local/lib/libglib-2.0.so.4201.9 > #7 0x0c396c2675f5 in g_type_create_instance () from > /usr/local/lib/libgobject-2.0.so.4200.16 > #8 0x0c3a19ad in vala_data_type_construct_with_symbol () >from /usr/local/lib/libvala-0.56.so.0.0 > #9 0x0c3a19b4ecae in vala_integer_type_construct () from > /usr/local/lib/libvala-0.56.so.0.0 > #10 0x0c3a19b4f08c in vala_integer_type_real_copy () from > /usr/local/lib/libvala-0.56.so.0.0 > #11 0x0c3a19a9d46f in vala_assignment_real_check () from > /usr/local/lib/libvala-0.56.so.0.0 > #12 0x0c3a19ae54e5 in vala_expression_statement_real_check () >from /usr/local/lib/libvala-0.56.so.0.0 > #13 0x0c3a19aa6a0d in vala_block_real_check () from > /usr/local/lib/libvala-0.56.so.0.0
Re: malloc: change chunk sizes to be multiple of 16 instead of power of 2
Second iteration.
Gain back performance by allocation chunk_info pages in a bundle, and
use less buckets is !malloc option S. The chunk sizes used are 16, 32,
48, 64, 80, 96, 112, 128, 160, 192, 224, 256, 320, 384, 448, 512, 640,
768, 896, 1024, 1280, 1536, 1792, 2048 (and a few more for sparc84
with it's 8k sized pages and loongson with it's 16k pages).
If malloc option S (or rather cache size 0) is used we use strict
multiple of 16 buckets, to get as many buckets as possible.
See the find_bucket() and bin_of() functions. Thanks to Tony Finch for
pointing me to code to compute nice bucket sizes.
I think this is ready for review and wide testing.
-Otto
Index: stdlib/malloc.c
===
RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.277
diff -u -p -r1.277 malloc.c
--- stdlib/malloc.c 27 Feb 2023 06:47:54 - 1.277
+++ stdlib/malloc.c 28 Feb 2023 16:49:08 -
@@ -67,6 +67,11 @@
#define MALLOC_CHUNK_LISTS 4
#define CHUNK_CHECK_LENGTH 32
+#define B2SIZE(b) ((b) * MALLOC_MINSIZE)
+#define B2ALLOC(b) ((b) == 0 ? MALLOC_MINSIZE : \
+ (b) * MALLOC_MINSIZE)
+#define BUCKETS(MALLOC_MAXCHUNK / MALLOC_MINSIZE)
+
/*
* We move allocations between half a page and a whole page towards the end,
* subject to alignment constraints. This is the extra headroom we allow.
@@ -144,9 +149,9 @@ struct dir_info {
int mutex;
int malloc_mt; /* multi-threaded mode? */
/* lists of free chunk info structs */
- struct chunk_head chunk_info_list[MALLOC_MAXSHIFT + 1];
+ struct chunk_head chunk_info_list[BUCKETS + 1];
/* lists of chunks with free slots */
- struct chunk_head chunk_dir[MALLOC_MAXSHIFT + 1][MALLOC_CHUNK_LISTS];
+ struct chunk_head chunk_dir[BUCKETS + 1][MALLOC_CHUNK_LISTS];
/* delayed free chunk slots */
void *delayed_chunks[MALLOC_DELAYED_CHUNK_MASK + 1];
u_char rbytes[32]; /* random bytes */
@@ -155,6 +160,8 @@ struct dir_info {
size_t bigcache_used;
size_t bigcache_size;
struct bigcache *bigcache;
+ void *chunk_pages;
+ size_t chunk_pages_used;
#ifdef MALLOC_STATS
size_t inserts;
size_t insert_collisions;
@@ -195,8 +202,7 @@ struct chunk_info {
LIST_ENTRY(chunk_info) entries;
void *page; /* pointer to the page */
u_short canary;
- u_short size; /* size of this page's chunks */
- u_short shift; /* how far to shift for this size */
+ u_short bucket;
u_short free; /* how many free chunks */
u_short total; /* how many chunks */
u_short offset; /* requested size table offset */
@@ -247,11 +253,11 @@ static void malloc_exit(void);
#endif
/* low bits of r->p determine size: 0 means >= page size and r->size holding
- * real size, otherwise low bits are a shift count, or 1 for malloc(0)
+ * real size, otherwise low bits is the bucket + 1
*/
#define REALSIZE(sz, r)\
(sz) = (uintptr_t)(r)->p & MALLOC_PAGEMASK, \
- (sz) = ((sz) == 0 ? (r)->size : ((sz) == 1 ? 0 : (1 << ((sz)-1
+ (sz) = ((sz) == 0 ? (r)->size : B2SIZE((sz) - 1))
static inline void
_MALLOC_LEAVE(struct dir_info *d)
@@ -502,7 +508,7 @@ omalloc_poolinit(struct dir_info *d, int
d->r = NULL;
d->rbytesused = sizeof(d->rbytes);
d->regions_free = d->regions_total = 0;
- for (i = 0; i <= MALLOC_MAXSHIFT; i++) {
+ for (i = 0; i <= BUCKETS; i++) {
LIST_INIT(>chunk_info_list[i]);
for (j = 0; j < MALLOC_CHUNK_LISTS; j++)
LIST_INIT(>chunk_dir[i][j]);
@@ -720,7 +726,7 @@ unmap(struct dir_info *d, void *p, size_
/* don't look through all slots */
for (j = 0; j < d->bigcache_size / 4; j++) {
- i = (base + j) % d->bigcache_size;
+ i = (base + j) & (d->bigcache_size - 1);
if (d->bigcache_used <
BIGCACHE_FILL(d->bigcache_size)) {
if (d->bigcache[i].psize == 0)
@@ -764,10 +770,13 @@ unmap(struct dir_info *d, void *p, size_
}
cache = >smallcache[psz - 1];
if (cache->length == cache->max) {
+ int fresh;
/* use a random slot */
- i = getrbyte(d) % cache->max;
+ i = getrbyte(d) & (cache->max - 1);
r = cache->pages[i];
- if (!mopts.malloc_freeunmap)
+ fresh = (uintptr_t)r & 1;
+
Re: malloc: handle to be cleaned chunks the same as regular ones
On Sat, Feb 18, 2023 at 04:12:08PM +0100, Otto Moerbeek wrote:
> Hi,
>
> these recent sshd double free issue prompted me to look at malloc
> again. I have something bigger brewing, but this diff makes sure the
> to be cleaned chunks (via freezero() or malloc_conceal) particpate in
> the delayed freeing just as others.
I'd like to see tests/reviews of this.
>
> -Otto
>
> Index: stdlib/malloc.c
> ===
> RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.276
> diff -u -p -r1.276 malloc.c
> --- stdlib/malloc.c 27 Dec 2022 17:31:09 - 1.276
> +++ stdlib/malloc.c 16 Feb 2023 14:11:50 -
> @@ -1515,42 +1515,38 @@ ofree(struct dir_info **argpool, void *p
> unmap(pool, p, PAGEROUND(sz), clear ? argsz : 0);
> delete(pool, r);
> } else {
> + void *tmp;
> + u_int i;
> +
> /* Validate and optionally canary check */
> struct chunk_info *info = (struct chunk_info *)r->size;
> if (info->size != sz)
> wrterror(pool, "internal struct corrupt");
> find_chunknum(pool, info, p, mopts.chunk_canaries);
> - if (!clear) {
> - void *tmp;
> - int i;
>
> - if (mopts.malloc_freecheck) {
> - for (i = 0; i <= MALLOC_DELAYED_CHUNK_MASK; i++)
> - if (p == pool->delayed_chunks[i])
> - wrterror(pool,
> - "double free %p", p);
> - }
> - junk_free(pool->malloc_junk, p, sz);
> - i = getrbyte(pool) & MALLOC_DELAYED_CHUNK_MASK;
> - tmp = p;
> - p = pool->delayed_chunks[i];
> - if (tmp == p)
> - wrterror(pool, "double free %p", tmp);
> - pool->delayed_chunks[i] = tmp;
> - if (p != NULL) {
> - r = find(pool, p);
> - REALSIZE(sz, r);
> - if (r != NULL)
> - validate_junk(pool, p, sz);
> - }
> - } else if (argsz > 0) {
> - r = find(pool, p);
> - explicit_bzero(p, argsz);
> + if (mopts.malloc_freecheck) {
> + for (i = 0; i <= MALLOC_DELAYED_CHUNK_MASK; i++)
> + if (p == pool->delayed_chunks[i])
> + wrterror(pool,
> + "double free %p", p);
> }
> + if (clear && argsz > 0)
> + explicit_bzero(p, argsz);
> + junk_free(pool->malloc_junk, p, sz);
> +
> + i = getrbyte(pool) & MALLOC_DELAYED_CHUNK_MASK;
> + tmp = p;
> + p = pool->delayed_chunks[i];
> + if (tmp == p)
> + wrterror(pool, "double free %p", p);
> + pool->delayed_chunks[i] = tmp;
> if (p != NULL) {
> + r = find(pool, p);
> if (r == NULL)
> wrterror(pool,
> "bogus pointer (double free?) %p", p);
> + REALSIZE(sz, r);
> + validate_junk(pool, p, sz);
> free_bytes(pool, r, p);
> }
> }
>
malloc: change chunk sizes to be multiple of 16 instead of power of 2
Hi,
The basic idea is simple: one of the reasons the recent sshd bug is
potentially exploitable is that a (erroneously) freed malloc chunk
gets re-used in a different role. My malloc has power of two chunk
sizes and so one page of chunks holds many different types of
allocations. Userland malloc has no knowledge of types, we only know
about sizes. So I changed that to use finer-grained chunk sizes.
Originally I thought it would be a *lot* of work, but it's not too
bad: a couple of hours of thinking and a couple of hours coding, which
mostly consisted of hunting for silent assumptions that chunk sizes
are a power of two.
I suspect this is not the final diff, as there is some performance
impact. In particular, sparc64 seems sensitive to these changes. I'm
still investigating why but I wanted to share the current work in
progress anyway.
Yuu can help by testing this.
Thanks,
-Otto
Index: stdlib/malloc.c
===
RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.276
diff -u -p -r1.276 malloc.c
--- stdlib/malloc.c 27 Dec 2022 17:31:09 - 1.276
+++ stdlib/malloc.c 20 Feb 2023 07:33:29 -
@@ -67,6 +67,11 @@
#define MALLOC_CHUNK_LISTS 4
#define CHUNK_CHECK_LENGTH 32
+#define B2SIZE(b) ((b) * MALLOC_MINSIZE)
+#define B2ALLOC(b) ((b) == 0 ? MALLOC_MINSIZE : \
+ (b) * MALLOC_MINSIZE)
+#define BUCKETS(MALLOC_MAXCHUNK / MALLOC_MINSIZE)
+
/*
* We move allocations between half a page and a whole page towards the end,
* subject to alignment constraints. This is the extra headroom we allow.
@@ -144,9 +149,9 @@ struct dir_info {
int mutex;
int malloc_mt; /* multi-threaded mode? */
/* lists of free chunk info structs */
- struct chunk_head chunk_info_list[MALLOC_MAXSHIFT + 1];
+ struct chunk_head chunk_info_list[BUCKETS + 1];
/* lists of chunks with free slots */
- struct chunk_head chunk_dir[MALLOC_MAXSHIFT + 1][MALLOC_CHUNK_LISTS];
+ struct chunk_head chunk_dir[BUCKETS + 1][MALLOC_CHUNK_LISTS];
/* delayed free chunk slots */
void *delayed_chunks[MALLOC_DELAYED_CHUNK_MASK + 1];
u_char rbytes[32]; /* random bytes */
@@ -195,8 +200,7 @@ struct chunk_info {
LIST_ENTRY(chunk_info) entries;
void *page; /* pointer to the page */
u_short canary;
- u_short size; /* size of this page's chunks */
- u_short shift; /* how far to shift for this size */
+ u_short bucket;
u_short free; /* how many free chunks */
u_short total; /* how many chunks */
u_short offset; /* requested size table offset */
@@ -247,11 +251,11 @@ static void malloc_exit(void);
#endif
/* low bits of r->p determine size: 0 means >= page size and r->size holding
- * real size, otherwise low bits are a shift count, or 1 for malloc(0)
+ * real size, otherwise low bits is the bucket + 1
*/
#define REALSIZE(sz, r)\
(sz) = (uintptr_t)(r)->p & MALLOC_PAGEMASK, \
- (sz) = ((sz) == 0 ? (r)->size : ((sz) == 1 ? 0 : (1 << ((sz)-1
+ (sz) = ((sz) == 0 ? (r)->size : B2SIZE((sz) - 1))
static inline void
_MALLOC_LEAVE(struct dir_info *d)
@@ -502,7 +506,7 @@ omalloc_poolinit(struct dir_info *d, int
d->r = NULL;
d->rbytesused = sizeof(d->rbytes);
d->regions_free = d->regions_total = 0;
- for (i = 0; i <= MALLOC_MAXSHIFT; i++) {
+ for (i = 0; i <= BUCKETS; i++) {
LIST_INIT(>chunk_info_list[i]);
for (j = 0; j < MALLOC_CHUNK_LISTS; j++)
LIST_INIT(>chunk_dir[i][j]);
@@ -883,21 +887,13 @@ map(struct dir_info *d, size_t sz, int z
}
static void
-init_chunk_info(struct dir_info *d, struct chunk_info *p, int bits)
+init_chunk_info(struct dir_info *d, struct chunk_info *p, u_int bucket)
{
- int i;
+ u_int i;
- if (bits == 0) {
- p->shift = MALLOC_MINSHIFT;
- p->total = p->free = MALLOC_PAGESIZE >> p->shift;
- p->size = 0;
- p->offset = 0xdead;
- } else {
- p->shift = bits;
- p->total = p->free = MALLOC_PAGESIZE >> p->shift;
- p->size = 1U << bits;
- p->offset = howmany(p->total, MALLOC_BITS);
- }
+ p->bucket = bucket;
+ p->total = p->free = MALLOC_PAGESIZE / B2ALLOC(bucket);
+ p->offset = bucket == 0 ? 0xdead : howmany(p->total, MALLOC_BITS);
p->canary = (u_short)d->canary1;
/* set all valid bits in the bitmap */
@@ -907,18 +903,15 @@
malloc: handle to be cleaned chunks the same as regular ones
Hi,
these recent sshd double free issue prompted me to look at malloc
again. I have something bigger brewing, but this diff makes sure the
to be cleaned chunks (via freezero() or malloc_conceal) particpate in
the delayed freeing just as others.
-Otto
Index: stdlib/malloc.c
===
RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.276
diff -u -p -r1.276 malloc.c
--- stdlib/malloc.c 27 Dec 2022 17:31:09 - 1.276
+++ stdlib/malloc.c 16 Feb 2023 14:11:50 -
@@ -1515,42 +1515,38 @@ ofree(struct dir_info **argpool, void *p
unmap(pool, p, PAGEROUND(sz), clear ? argsz : 0);
delete(pool, r);
} else {
+ void *tmp;
+ u_int i;
+
/* Validate and optionally canary check */
struct chunk_info *info = (struct chunk_info *)r->size;
if (info->size != sz)
wrterror(pool, "internal struct corrupt");
find_chunknum(pool, info, p, mopts.chunk_canaries);
- if (!clear) {
- void *tmp;
- int i;
- if (mopts.malloc_freecheck) {
- for (i = 0; i <= MALLOC_DELAYED_CHUNK_MASK; i++)
- if (p == pool->delayed_chunks[i])
- wrterror(pool,
- "double free %p", p);
- }
- junk_free(pool->malloc_junk, p, sz);
- i = getrbyte(pool) & MALLOC_DELAYED_CHUNK_MASK;
- tmp = p;
- p = pool->delayed_chunks[i];
- if (tmp == p)
- wrterror(pool, "double free %p", tmp);
- pool->delayed_chunks[i] = tmp;
- if (p != NULL) {
- r = find(pool, p);
- REALSIZE(sz, r);
- if (r != NULL)
- validate_junk(pool, p, sz);
- }
- } else if (argsz > 0) {
- r = find(pool, p);
- explicit_bzero(p, argsz);
+ if (mopts.malloc_freecheck) {
+ for (i = 0; i <= MALLOC_DELAYED_CHUNK_MASK; i++)
+ if (p == pool->delayed_chunks[i])
+ wrterror(pool,
+ "double free %p", p);
}
+ if (clear && argsz > 0)
+ explicit_bzero(p, argsz);
+ junk_free(pool->malloc_junk, p, sz);
+
+ i = getrbyte(pool) & MALLOC_DELAYED_CHUNK_MASK;
+ tmp = p;
+ p = pool->delayed_chunks[i];
+ if (tmp == p)
+ wrterror(pool, "double free %p", p);
+ pool->delayed_chunks[i] = tmp;
if (p != NULL) {
+ r = find(pool, p);
if (r == NULL)
wrterror(pool,
"bogus pointer (double free?) %p", p);
+ REALSIZE(sz, r);
+ validate_junk(pool, p, sz);
free_bytes(pool, r, p);
}
}
Re: ifconfig.c redundancy the second
On Fri, Jan 13, 2023 at 08:04:36PM +0100, Mathias Koehler wrote:
> I hope the following message is a format more helpful for you
> guys. (Thanks to Otto Moerbeek who gave me a hint.)
/% ... %./ is not a comment marker in C.
-Otto
> Again my question is how should that code look like?
> Because I can remove the 'if' and the code still does the same.
>
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> retrieving revision 1.460
> diff -u -p -u -p -r1.460 ifconfig.c
> --- ifconfig.c 18 Dec 2022 18:56:38 - 1.460
> +++ ifconfig.c 13 Jan 2023 18:52:48 -
> @@ -1907,12 +1907,14 @@ delifjoinlist(const char *val, int d)
> memset(, 0, sizeof(join));
> join.i_flags |= (IEEE80211_JOIN_DEL | IEEE80211_JOIN_DEL_ALL);
>
> + /%
> if (d == -1) {
> ifr.ifr_data = (caddr_t)
> if (ioctl(sock, SIOCS80211JOIN, (caddr_t)) == -1)
> err(1, "SIOCS80211JOIN");
> return;
> }
> + %/
>
> ifr.ifr_data = (caddr_t)
> if (ioctl(sock, SIOCS80211JOIN, (caddr_t)) == -1)
>
Re: ifconfig.c - redundancy
On Fri, Jan 06, 2023 at 09:46:13PM +0100, Mathias Koehler wrote: > lines 1911 - 1913 and 1917 - 1919 are code blocks that are looking > identical. I cannot imagine that that is on purpose. What do you > really wanna express here? Just quoting line numbers does not help a lot. You were looking at the code, so quote the lines, making things easier for the readers of the list. Looking at the cvs history it seems it was simply overlooked the two blocks are the same. The best way to approach this is to come up with a diff. The makes your message explicit and helps to improve the code base. If the two blocks should *not* be the same, it is very likely a developer with knowledge of this area will be triggered by a diff as well. In short: we prefer to talk in diffs, because there are explicit, actionable, not ambiguous and so focus the discourse. -Otto
Re: malloc and immutable
On Sun, Dec 18, 2022 at 08:10:48PM +0100, Otto Moerbeek wrote:
> Hi,
>
> This is the latest verion of a diff I sent around previously, but now
> it's time this gets wider testing for real.
>
> The main purpose is to rearrange malloc init in such a way that a few
> pages containing mallic internal data structures can be made
> immutable. In addtion to that, each pools is inited on-demand instead
> of always.
>
> So please test! I did not get a lot of feedback on the earlier versions.
Many thanks to all who tested so far!
I do lacks test reports on some more exoctic platforms, these days
that is !(amd64 || arm64). Any takers?
-Otto
>
> Index: stdlib/malloc.c
> ===
> RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.275
> diff -u -p -r1.275 malloc.c
> --- stdlib/malloc.c 14 Oct 2022 04:38:39 - 1.275
> +++ stdlib/malloc.c 18 Dec 2022 18:57:16 -
> @@ -142,6 +142,7 @@ struct dir_info {
> int malloc_junk;/* junk fill? */
> int mmap_flag; /* extra flag for mmap */
> int mutex;
> + int malloc_mt; /* multi-threaded mode? */
> /* lists of free chunk info structs */
> struct chunk_head chunk_info_list[MALLOC_MAXSHIFT + 1];
> /* lists of chunks with free slots */
> @@ -181,8 +182,6 @@ struct dir_info {
> #endif /* MALLOC_STATS */
> u_int32_t canary2;
> };
> -#define DIR_INFO_RSZ ((sizeof(struct dir_info) + MALLOC_PAGEMASK) & \
> - ~MALLOC_PAGEMASK)
>
> static void unmap(struct dir_info *d, void *p, size_t sz, size_t clear);
>
> @@ -208,7 +207,6 @@ struct malloc_readonly {
> /* Main bookkeeping information */
> struct dir_info *malloc_pool[_MALLOC_MUTEXES];
> u_int malloc_mutexes; /* how much in actual use? */
> - int malloc_mt; /* multi-threaded mode? */
> int malloc_freecheck; /* Extensive double free check */
> int malloc_freeunmap; /* mprotect free pages PROT_NONE? */
> int def_malloc_junk;/* junk fill? */
> @@ -258,7 +256,7 @@ static void malloc_exit(void);
> static inline void
> _MALLOC_LEAVE(struct dir_info *d)
> {
> - if (mopts.malloc_mt) {
> + if (d->malloc_mt) {
> d->active--;
> _MALLOC_UNLOCK(d->mutex);
> }
> @@ -267,7 +265,7 @@ _MALLOC_LEAVE(struct dir_info *d)
> static inline void
> _MALLOC_ENTER(struct dir_info *d)
> {
> - if (mopts.malloc_mt) {
> + if (d->malloc_mt) {
> _MALLOC_LOCK(d->mutex);
> d->active++;
> }
> @@ -292,7 +290,7 @@ hash(void *p)
> static inline struct dir_info *
> getpool(void)
> {
> - if (!mopts.malloc_mt)
> + if (mopts.malloc_pool[1] == NULL || !mopts.malloc_pool[1]->malloc_mt)
> return mopts.malloc_pool[1];
> else/* first one reserved for special pool */
> return mopts.malloc_pool[1 + TIB_GET()->tib_tid %
> @@ -497,46 +495,22 @@ omalloc_init(void)
> }
>
> static void
> -omalloc_poolinit(struct dir_info **dp, int mmap_flag)
> +omalloc_poolinit(struct dir_info *d, int mmap_flag)
> {
> - char *p;
> - size_t d_avail, regioninfo_size;
> - struct dir_info *d;
> int i, j;
>
> - /*
> - * Allocate dir_info with a guard page on either side. Also
> - * randomise offset inside the page at which the dir_info
> - * lies (subject to alignment by 1 << MALLOC_MINSHIFT)
> - */
> - if ((p = MMAPNONE(DIR_INFO_RSZ + (MALLOC_PAGESIZE * 2), mmap_flag)) ==
> - MAP_FAILED)
> - wrterror(NULL, "malloc init mmap failed");
> - mprotect(p + MALLOC_PAGESIZE, DIR_INFO_RSZ, PROT_READ | PROT_WRITE);
> - d_avail = (DIR_INFO_RSZ - sizeof(*d)) >> MALLOC_MINSHIFT;
> - d = (struct dir_info *)(p + MALLOC_PAGESIZE +
> - (arc4random_uniform(d_avail) << MALLOC_MINSHIFT));
> -
> - rbytes_init(d);
> - d->regions_free = d->regions_total = MALLOC_INITIAL_REGIONS;
> - regioninfo_size = d->regions_total * sizeof(struct region_info);
> - d->r = MMAP(regioninfo_size, mmap_flag);
> - if (d->r == MAP_FAILED) {
> - d->regions_total = 0;
> - wrterror(NULL, "malloc init mmap failed");
> - }
> + d->r = NULL;
> + d->rbytesused = sizeof(d->rbytes);
> + d->regions_free = d->regions_total = 0;
> for (i
malloc and immutable
Hi,
This is the latest verion of a diff I sent around previously, but now
it's time this gets wider testing for real.
The main purpose is to rearrange malloc init in such a way that a few
pages containing mallic internal data structures can be made
immutable. In addtion to that, each pools is inited on-demand instead
of always.
So please test! I did not get a lot of feedback on the earlier versions.
-Otto
Index: stdlib/malloc.c
===
RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.275
diff -u -p -r1.275 malloc.c
--- stdlib/malloc.c 14 Oct 2022 04:38:39 - 1.275
+++ stdlib/malloc.c 18 Dec 2022 18:57:16 -
@@ -142,6 +142,7 @@ struct dir_info {
int malloc_junk;/* junk fill? */
int mmap_flag; /* extra flag for mmap */
int mutex;
+ int malloc_mt; /* multi-threaded mode? */
/* lists of free chunk info structs */
struct chunk_head chunk_info_list[MALLOC_MAXSHIFT + 1];
/* lists of chunks with free slots */
@@ -181,8 +182,6 @@ struct dir_info {
#endif /* MALLOC_STATS */
u_int32_t canary2;
};
-#define DIR_INFO_RSZ ((sizeof(struct dir_info) + MALLOC_PAGEMASK) & \
- ~MALLOC_PAGEMASK)
static void unmap(struct dir_info *d, void *p, size_t sz, size_t clear);
@@ -208,7 +207,6 @@ struct malloc_readonly {
/* Main bookkeeping information */
struct dir_info *malloc_pool[_MALLOC_MUTEXES];
u_int malloc_mutexes; /* how much in actual use? */
- int malloc_mt; /* multi-threaded mode? */
int malloc_freecheck; /* Extensive double free check */
int malloc_freeunmap; /* mprotect free pages PROT_NONE? */
int def_malloc_junk;/* junk fill? */
@@ -258,7 +256,7 @@ static void malloc_exit(void);
static inline void
_MALLOC_LEAVE(struct dir_info *d)
{
- if (mopts.malloc_mt) {
+ if (d->malloc_mt) {
d->active--;
_MALLOC_UNLOCK(d->mutex);
}
@@ -267,7 +265,7 @@ _MALLOC_LEAVE(struct dir_info *d)
static inline void
_MALLOC_ENTER(struct dir_info *d)
{
- if (mopts.malloc_mt) {
+ if (d->malloc_mt) {
_MALLOC_LOCK(d->mutex);
d->active++;
}
@@ -292,7 +290,7 @@ hash(void *p)
static inline struct dir_info *
getpool(void)
{
- if (!mopts.malloc_mt)
+ if (mopts.malloc_pool[1] == NULL || !mopts.malloc_pool[1]->malloc_mt)
return mopts.malloc_pool[1];
else/* first one reserved for special pool */
return mopts.malloc_pool[1 + TIB_GET()->tib_tid %
@@ -497,46 +495,22 @@ omalloc_init(void)
}
static void
-omalloc_poolinit(struct dir_info **dp, int mmap_flag)
+omalloc_poolinit(struct dir_info *d, int mmap_flag)
{
- char *p;
- size_t d_avail, regioninfo_size;
- struct dir_info *d;
int i, j;
- /*
-* Allocate dir_info with a guard page on either side. Also
-* randomise offset inside the page at which the dir_info
-* lies (subject to alignment by 1 << MALLOC_MINSHIFT)
-*/
- if ((p = MMAPNONE(DIR_INFO_RSZ + (MALLOC_PAGESIZE * 2), mmap_flag)) ==
- MAP_FAILED)
- wrterror(NULL, "malloc init mmap failed");
- mprotect(p + MALLOC_PAGESIZE, DIR_INFO_RSZ, PROT_READ | PROT_WRITE);
- d_avail = (DIR_INFO_RSZ - sizeof(*d)) >> MALLOC_MINSHIFT;
- d = (struct dir_info *)(p + MALLOC_PAGESIZE +
- (arc4random_uniform(d_avail) << MALLOC_MINSHIFT));
-
- rbytes_init(d);
- d->regions_free = d->regions_total = MALLOC_INITIAL_REGIONS;
- regioninfo_size = d->regions_total * sizeof(struct region_info);
- d->r = MMAP(regioninfo_size, mmap_flag);
- if (d->r == MAP_FAILED) {
- d->regions_total = 0;
- wrterror(NULL, "malloc init mmap failed");
- }
+ d->r = NULL;
+ d->rbytesused = sizeof(d->rbytes);
+ d->regions_free = d->regions_total = 0;
for (i = 0; i <= MALLOC_MAXSHIFT; i++) {
LIST_INIT(>chunk_info_list[i]);
for (j = 0; j < MALLOC_CHUNK_LISTS; j++)
LIST_INIT(>chunk_dir[i][j]);
}
- STATS_ADD(d->malloc_used, regioninfo_size + 3 * MALLOC_PAGESIZE);
d->mmap_flag = mmap_flag;
d->malloc_junk = mopts.def_malloc_junk;
d->canary1 = mopts.malloc_canary ^ (u_int32_t)(uintptr_t)d;
d->canary2 = ~d->canary1;
-
- *dp = d;
}
static int
@@ -551,7 +525,8 @@ omalloc_grow(struct dir_info *d)
if (d->regions_total > SIZE_MAX / sizeof(struct region_info) / 2)
return 1;
- newtotal = d->regions_total * 2;
+ newtotal =
Re: More on mimmutable
On Thu, Nov 17, 2022 at 08:10:05PM -0700, Theo de Raadt wrote: > So this static executable is completely immutable, except for the > OPENBSD_MUTABLE region. This annotation is used in one place now, deep > inside libc's malloc(3) code, where a piece of code flips a data structure > between readonly and read-write as a security measure. That does not become > immutable. It happens to be an page. This will change. I have code ready to change the init of that malloc data structure so that the one page wil be modified to contain the right data, then made readonly and then made immutable. -Otto
Re: M1 Macmini lost hw.cpuspeed
On Mon, Oct 24, 2022 at 04:15:40PM +0200, Mark Kettenis wrote: > > Date: Mon, 24 Oct 2022 14:52:00 +0200 > > From: Robert Nagy > > > > On 24/10/22 14:49 +0200, Theo Buehler wrote: > > > On Mon, Oct 24, 2022 at 09:24:14AM +0200, Otto Moerbeek wrote: > > > > Hello, > > > > > > > > after updating my M1 macmini after my vacatiuon to the latest snap it > > > > seems to have lost a few sysctl nodes, making apm(8) fail: > > > > > > > > [otto@macmini:4]$ ktrace apm > > > > Battery state: unknown, 0% remaining, unknown life estimate > > > > AC adapter state: not known > > > > Performance adjustment mode: invalid (0 MHz) > > > > > > > > ... > > > > 88255 apm CALL > > > > sysctl(6.12,0x7f10d4,0x7f10c8,0,0) > > > > 88255 apm RET sysctl -1 errno 45 Operation not supported > > > > ... > > > > > > > > > > > > Beforre I spend time on this, does anybody have a clue? > > > > > > > > -Otto > > > > > > > > > > I'm seeing the same on my m1 mini. Reverting the commit below fixes it > > > for me. > > > > Maybe you need a new u-boot that has the new tree? > > Yes there has been some churn in this area in the device tree and I > removed backwards compatibility for the old old way this was done. > > Now the question is when did you install OpenBSD? If you have a > directory called M1N1 on your msdos partition with the file BOOT.BIN > in it, this will be fairly easy to fix. Download > > https://cdn.asahilinux.org/os/uefi-only-20220717-1.zip > > unzip it and copy esp/m1n1/boot.bin to your msdos partition. Maybe > make a copy of the old file first (and leave it in that directory) > just in case. > > If not, fixing this will be a little bit more involved, and I need to > figure out instructions. I'm pretty sure this is the case for tb@. > > Cheers, > > Mark Hi, the instructions worked for me, thanks! -Otto
M1 Macmini lost hw.cpuspeed
Hello, after updating my M1 macmini after my vacatiuon to the latest snap it seems to have lost a few sysctl nodes, making apm(8) fail: [otto@macmini:4]$ ktrace apm Battery state: unknown, 0% remaining, unknown life estimate AC adapter state: not known Performance adjustment mode: invalid (0 MHz) ... 88255 apm CALL sysctl(6.12,0x7f10d4,0x7f10c8,0,0) 88255 apm RET sysctl -1 errno 45 Operation not supported ... Beforre I spend time on this, does anybody have a clue? -Otto
malloc diff
Hi,
this diff has been sent out earlier, but now that more or the
immutable parts are in, it is time to test it in this new environment.
Thanks,
-Otto
Index: stdlib/malloc.c
===
RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.274
diff -u -p -r1.274 malloc.c
--- stdlib/malloc.c 30 Jun 2022 17:15:48 - 1.274
+++ stdlib/malloc.c 29 Sep 2022 09:42:57 -
@@ -142,6 +142,7 @@ struct dir_info {
int malloc_junk;/* junk fill? */
int mmap_flag; /* extra flag for mmap */
int mutex;
+ int malloc_mt; /* multi-threaded mode? */
/* lists of free chunk info structs */
struct chunk_head chunk_info_list[MALLOC_MAXSHIFT + 1];
/* lists of chunks with free slots */
@@ -181,8 +182,6 @@ struct dir_info {
#endif /* MALLOC_STATS */
u_int32_t canary2;
};
-#define DIR_INFO_RSZ ((sizeof(struct dir_info) + MALLOC_PAGEMASK) & \
- ~MALLOC_PAGEMASK)
static void unmap(struct dir_info *d, void *p, size_t sz, size_t clear);
@@ -208,7 +207,6 @@ struct malloc_readonly {
/* Main bookkeeping information */
struct dir_info *malloc_pool[_MALLOC_MUTEXES];
u_int malloc_mutexes; /* how much in actual use? */
- int malloc_mt; /* multi-threaded mode? */
int malloc_freecheck; /* Extensive double free check */
int malloc_freeunmap; /* mprotect free pages PROT_NONE? */
int def_malloc_junk;/* junk fill? */
@@ -257,7 +255,7 @@ static void malloc_exit(void);
static inline void
_MALLOC_LEAVE(struct dir_info *d)
{
- if (mopts.malloc_mt) {
+ if (d->malloc_mt) {
d->active--;
_MALLOC_UNLOCK(d->mutex);
}
@@ -266,7 +264,7 @@ _MALLOC_LEAVE(struct dir_info *d)
static inline void
_MALLOC_ENTER(struct dir_info *d)
{
- if (mopts.malloc_mt) {
+ if (d->malloc_mt) {
_MALLOC_LOCK(d->mutex);
d->active++;
}
@@ -291,7 +289,7 @@ hash(void *p)
static inline struct dir_info *
getpool(void)
{
- if (!mopts.malloc_mt)
+ if (mopts.malloc_pool[1] == NULL || !mopts.malloc_pool[1]->malloc_mt)
return mopts.malloc_pool[1];
else/* first one reserved for special pool */
return mopts.malloc_pool[1 + TIB_GET()->tib_tid %
@@ -496,46 +494,22 @@ omalloc_init(void)
}
static void
-omalloc_poolinit(struct dir_info **dp, int mmap_flag)
+omalloc_poolinit(struct dir_info *d, int mmap_flag)
{
- char *p;
- size_t d_avail, regioninfo_size;
- struct dir_info *d;
int i, j;
- /*
-* Allocate dir_info with a guard page on either side. Also
-* randomise offset inside the page at which the dir_info
-* lies (subject to alignment by 1 << MALLOC_MINSHIFT)
-*/
- if ((p = MMAPNONE(DIR_INFO_RSZ + (MALLOC_PAGESIZE * 2), mmap_flag)) ==
- MAP_FAILED)
- wrterror(NULL, "malloc init mmap failed");
- mprotect(p + MALLOC_PAGESIZE, DIR_INFO_RSZ, PROT_READ | PROT_WRITE);
- d_avail = (DIR_INFO_RSZ - sizeof(*d)) >> MALLOC_MINSHIFT;
- d = (struct dir_info *)(p + MALLOC_PAGESIZE +
- (arc4random_uniform(d_avail) << MALLOC_MINSHIFT));
-
- rbytes_init(d);
- d->regions_free = d->regions_total = MALLOC_INITIAL_REGIONS;
- regioninfo_size = d->regions_total * sizeof(struct region_info);
- d->r = MMAP(regioninfo_size, mmap_flag);
- if (d->r == MAP_FAILED) {
- d->regions_total = 0;
- wrterror(NULL, "malloc init mmap failed");
- }
+ d->r = NULL;
+ d->rbytesused = sizeof(d->rbytes);
+ d->regions_free = d->regions_total = 0;
for (i = 0; i <= MALLOC_MAXSHIFT; i++) {
LIST_INIT(>chunk_info_list[i]);
for (j = 0; j < MALLOC_CHUNK_LISTS; j++)
LIST_INIT(>chunk_dir[i][j]);
}
- STATS_ADD(d->malloc_used, regioninfo_size + 3 * MALLOC_PAGESIZE);
d->mmap_flag = mmap_flag;
d->malloc_junk = mopts.def_malloc_junk;
d->canary1 = mopts.malloc_canary ^ (u_int32_t)(uintptr_t)d;
d->canary2 = ~d->canary1;
-
- *dp = d;
}
static int
@@ -550,7 +524,8 @@ omalloc_grow(struct dir_info *d)
if (d->regions_total > SIZE_MAX / sizeof(struct region_info) / 2)
return 1;
- newtotal = d->regions_total * 2;
+ newtotal = d->regions_total == 0 ? MALLOC_INITIAL_REGIONS :
+ d->regions_total * 2;
newsize = PAGEROUND(newtotal * sizeof(struct region_info));
mask = newtotal - 1;
@@ -575,10 +550,12 @@ omalloc_grow(struct dir_info *d)
}
}
-
Re: malloc: prep for immutable pages
On Wed, Oct 05, 2022 at 02:47:19PM +0200, Marc Espie wrote: > On Tue, Oct 04, 2022 at 10:15:51AM -0600, Theo de Raadt wrote: > > A note on why this chance is coming. > > > > malloc.c (as it is today), does mprotects back and forth between RW and > > R, to protect an internal object. This object is in bss, it is not > > allocated with mmap. With the upcoming mimmutable change, the bss will > > become immutable by default, at program load time. mimmutable even prevents > > changing a RW object to R. > > I'm probably missing something here, but for me, traditionally, > BSS is the "set to 0" section of global variables of a program... which are > usually going to be changed to some other value. > > Or are we talking at cross purposes ? > malloc sets up a few values in a bss struct and then sets the page its in to r/o. But when switching to multi-threaded mode a few variables in the struct are changed (in current), so the page has to be made r/w again, modified and then back to r/o. The diff changes things so that the pages can be set to r/o and immutable after malloc init. -Otto
malloc: prep for immutable pages
Hi,
Rearrange things so that we do not have to flip protection of r/o
pages back and forth when swicthing from single-threaded to
multi-threaded. Also saves work in many cases by not initing pools
until they are used: the pool used for MAP_CONCEAL pages is not used
by very many processes and if you have just a few threads only a few
pools will be set up.
The actual mimmutable calls are to be added later. I marked the places where
they should end up.
Please test,
-Otto
Index: stdlib/malloc.c
===
RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.274
diff -u -p -r1.274 malloc.c
--- stdlib/malloc.c 30 Jun 2022 17:15:48 - 1.274
+++ stdlib/malloc.c 29 Sep 2022 09:42:57 -
@@ -142,6 +142,7 @@ struct dir_info {
int malloc_junk;/* junk fill? */
int mmap_flag; /* extra flag for mmap */
int mutex;
+ int malloc_mt; /* multi-threaded mode? */
/* lists of free chunk info structs */
struct chunk_head chunk_info_list[MALLOC_MAXSHIFT + 1];
/* lists of chunks with free slots */
@@ -181,8 +182,6 @@ struct dir_info {
#endif /* MALLOC_STATS */
u_int32_t canary2;
};
-#define DIR_INFO_RSZ ((sizeof(struct dir_info) + MALLOC_PAGEMASK) & \
- ~MALLOC_PAGEMASK)
static void unmap(struct dir_info *d, void *p, size_t sz, size_t clear);
@@ -208,7 +207,6 @@ struct malloc_readonly {
/* Main bookkeeping information */
struct dir_info *malloc_pool[_MALLOC_MUTEXES];
u_int malloc_mutexes; /* how much in actual use? */
- int malloc_mt; /* multi-threaded mode? */
int malloc_freecheck; /* Extensive double free check */
int malloc_freeunmap; /* mprotect free pages PROT_NONE? */
int def_malloc_junk;/* junk fill? */
@@ -257,7 +255,7 @@ static void malloc_exit(void);
static inline void
_MALLOC_LEAVE(struct dir_info *d)
{
- if (mopts.malloc_mt) {
+ if (d->malloc_mt) {
d->active--;
_MALLOC_UNLOCK(d->mutex);
}
@@ -266,7 +264,7 @@ _MALLOC_LEAVE(struct dir_info *d)
static inline void
_MALLOC_ENTER(struct dir_info *d)
{
- if (mopts.malloc_mt) {
+ if (d->malloc_mt) {
_MALLOC_LOCK(d->mutex);
d->active++;
}
@@ -291,7 +289,7 @@ hash(void *p)
static inline struct dir_info *
getpool(void)
{
- if (!mopts.malloc_mt)
+ if (mopts.malloc_pool[1] == NULL || !mopts.malloc_pool[1]->malloc_mt)
return mopts.malloc_pool[1];
else/* first one reserved for special pool */
return mopts.malloc_pool[1 + TIB_GET()->tib_tid %
@@ -496,46 +494,22 @@ omalloc_init(void)
}
static void
-omalloc_poolinit(struct dir_info **dp, int mmap_flag)
+omalloc_poolinit(struct dir_info *d, int mmap_flag)
{
- char *p;
- size_t d_avail, regioninfo_size;
- struct dir_info *d;
int i, j;
- /*
-* Allocate dir_info with a guard page on either side. Also
-* randomise offset inside the page at which the dir_info
-* lies (subject to alignment by 1 << MALLOC_MINSHIFT)
-*/
- if ((p = MMAPNONE(DIR_INFO_RSZ + (MALLOC_PAGESIZE * 2), mmap_flag)) ==
- MAP_FAILED)
- wrterror(NULL, "malloc init mmap failed");
- mprotect(p + MALLOC_PAGESIZE, DIR_INFO_RSZ, PROT_READ | PROT_WRITE);
- d_avail = (DIR_INFO_RSZ - sizeof(*d)) >> MALLOC_MINSHIFT;
- d = (struct dir_info *)(p + MALLOC_PAGESIZE +
- (arc4random_uniform(d_avail) << MALLOC_MINSHIFT));
-
- rbytes_init(d);
- d->regions_free = d->regions_total = MALLOC_INITIAL_REGIONS;
- regioninfo_size = d->regions_total * sizeof(struct region_info);
- d->r = MMAP(regioninfo_size, mmap_flag);
- if (d->r == MAP_FAILED) {
- d->regions_total = 0;
- wrterror(NULL, "malloc init mmap failed");
- }
+ d->r = NULL;
+ d->rbytesused = sizeof(d->rbytes);
+ d->regions_free = d->regions_total = 0;
for (i = 0; i <= MALLOC_MAXSHIFT; i++) {
LIST_INIT(>chunk_info_list[i]);
for (j = 0; j < MALLOC_CHUNK_LISTS; j++)
LIST_INIT(>chunk_dir[i][j]);
}
- STATS_ADD(d->malloc_used, regioninfo_size + 3 * MALLOC_PAGESIZE);
d->mmap_flag = mmap_flag;
d->malloc_junk = mopts.def_malloc_junk;
d->canary1 = mopts.malloc_canary ^ (u_int32_t)(uintptr_t)d;
d->canary2 = ~d->canary1;
-
- *dp = d;
}
static int
@@ -550,7 +524,8 @@ omalloc_grow(struct dir_info *d)
if (d->regions_total > SIZE_MAX / sizeof(struct region_info) / 2)
return 1;
-
Re: Request for testing malloc and multi-threaded applications
On Tue, Sep 27, 2022 at 03:31:12PM +0200, Renaud Allard wrote: > On 1/16/19 19:09, Otto Moerbeek wrote: > > On Wed, Jan 16, 2019 at 01:25:25PM +, Stuart Henderson wrote: > > > > > On 2019/01/04 08:09, Otto Moerbeek wrote: > > > > On Thu, Dec 27, 2018 at 09:39:56AM +0100, Otto Moerbeek wrote: > > > > > > > > > > > > > > Very little feedback so far. This diff can only give me valid feedback > > > > > if the coverage of systems and use cases is wide. If I do not get > > > > > more feedback, I have to base my decisions on my own testing, which > > > > > will benefit my systems and use cases, but might harm yours. > > > > > > > > > > So, ladies and gentlemen, start your tests! > > > > > > > > Another reminder. I like to make progress on this. That means I need > > > > tests for various use-cases. > > > > > > I have a map based website I use that is quite good at stressing things > > > (high spin% cpu) and have been timing from opening chromium (I'm using > > > this for the test because it typically performs less well than firefox). > > > Time is real time from starting the browser set to 'start with previously > > > opened windows' and the page open, until when the page reports that it's > > > finished loading (i.e. fetching data from the server and rendering it). > > > > > > It's not a perfect test - depends on network/server conditions etc - and > > > it's a visualisation of conditions in a game so may change slightly from > > > run to run but there shouldn't be huge changes between the times I've > > > run it - but is a bit more repeatable than a subjective "does the browser > > > feel slow". > > > > > > 4x "real" cores, Xeon E3-1225v3, 16GB ram (not going into swap). > > > > > > I've mixed up the test orders so it's not 3x +++, 2x ++, 3x + etc in > > > order, > > > more like +++, -, '', -, ++ etc. > > > > > > +++ 90 98 68 > > > ++ 85 82 > > > + 87 56 71 > > > '' 76 60 69 88 > > > - 77 74 85 > > > -- 48 86 77 67 > > > > > > So while it's not very consistent, the fastest times I've seen are on > > > runs with fewer pools, and the slowest times on runs with more pools, > > > with '' possibly seeming a bit more consistent from run to run. But > > > there's not enough consistency with any of it to be able to make any > > > clear conclusion (and I get the impression it would be hard to > > > tell without some automated test that can be repeated many times > > > and carrying out a statistical analysis on results). > > > > > > > Thanks for testing. To be clear: this is with the diff I posted and not the > > committed code, right? (There is a small change in the committed code > > to change the default to what 1 plus was with the diff). > > > > -Otto > > > > Hello, > > Given that code is in base for about 4 years, shouldn't be the man page > modified to add an explanation for those ++--? Or is there a reason why it's > not documented? > > Best Regards > No, this is for internal/development use only and might be removed any time. It's undocumented on purpose. -Otto
Re: installer: zap fdisk.8.gz and disklabel.8.gz
On Thu, Aug 25, 2022 at 07:32:16PM +, Miod Vallat wrote: > > > The ability to be able to read the manual pages from the binaries > > > themselves, when running is interactive mode, is an intentional feature > > > and the reason they embed a gzipped version of the formatted manpage. > > > > Even in the installer? > > Especially in the installer, because you might not have another machine > nearby to access documentation. (and there were no smartphones when this > was introduced close to 25 years ago) > > > The manual feature is handy, but I don't think it's worth having in > > install media. > > This feature is *especially* intended for installation media. > I might even go one step furter: the regular programs do not need the embedded man pages (as man(1) is available), but the installboot versions sure do. -Otto
Re: Race in disk_attach_callback?
On Wed, Jul 13, 2022 at 02:18:53PM +0200, Otto Moerbeek wrote:
> Hi,
>
> after a prompt from stsp@ and florian@, reporting that newfs_msdos
> fails when given an $duid.i argument, I set down to see what could be
> going on. My test using an USB stick failed to reprdouce the problem.
> Then I started using a vnd, and that shows the issue (once in a
> while). The feeling is that any disk devcied created on the fly might
> show this issue.
>
> What I have found out so far: sometimes, disk_subr.c:disk_map()
> fails, because the device in question does not have the DKF_LABELVALID
> flag set, so it is skipped in the duid search.
>
> The only place where DKF_LABELVALID is set is in
> disk_attach_callback().
>
> Often the disk_readlabel() call in this function succeeds and the flag
> is set, but also often it does not succeed.
>
> I have observed it failing setting the message to "cannot read disk
> label, 0xe32/0x2932, error 25", which seems to come from the
> DIOCGPDINFO NOTTY case in dev/vnc.c
>
> This is how far I got.
Looking a bit more, I see that the attachment is done from a task. I
suppose that if the open happens before the task is run, we end up
with a flag not set.
>
> I am using the test script below. In my case, instrumenting
> subr_disk.c with printf made the problem occur less often, so I
> suspect a race between the disk attachment and the label becoming
> available.
>
> The vnconfig triggers the disk attachment message (but only the first time a
> vnd is created, it seems, I am not seeing any messages after a vnconfig
> -u and re-running the script).
>
>
> === script ===
> set -e
> dd if=/dev/random of=/tmp/image$1 bs=1m count=100
> vnconfig vnd$1 /tmp/image$1
> fdisk -ig vnd$1
> echo "a\ni\n\n\n\nw\nl\nq\n" | disklabel -E vnd$1
> ==
>
> Try to run newfs_msdos on the duid.i reported. Note that you need to
> reboot to see a change a behaviour on a specific vnd. If it starts out
> OK, it keeps on being OK, if it fails, it keeps on failing. This
> corresponds to my observation that the disk_attach_callback() is
> called only once for a specifc vnd, even after unconfig and reconfig.
>
> Hope somebody who knows how this all shoudl work can see where the
> issue is.
>
> -Otto
>
> Index: subr_disk.c
> ===
> RCS file: /cvs/src/sys/kern/subr_disk.c,v
> retrieving revision 1.248
> diff -u -p -r1.248 subr_disk.c
> --- subr_disk.c 2 Jan 2022 17:26:14 - 1.248
> +++ subr_disk.c 13 Jul 2022 12:02:05 -
> @@ -1118,8 +1118,10 @@ disk_attach_callback(void *xdat)
> /* Read disklabel. */
> if (disk_readlabel(, dk->dk_devno, errbuf, sizeof(errbuf)) == NULL) {
> enqueue_randomness(dl.d_checksum);
> + printf("disk_attach_callback %s: seting DKF_LABELVALID\n",
> dk->dk_name);
> dk->dk_flags |= DKF_LABELVALID;
> }
> + else printf("disk_attach_callback %s: NOT seting DKF_LABELVALID
> (%s)\n", dk->dk_name, errbuf);
>
> done:
> dk->dk_flags |= DKF_OPENED;
> @@ -1785,6 +1787,7 @@ disk_map(char *path, char *mappath, int
>
> mdk = NULL;
> TAILQ_FOREACH(dk, , dk_link) {
> + printf("YYY %p %s %x %p\n", dk, dk->dk_name, dk->dk_flags,
> dk->dk_label);
> if ((dk->dk_flags & DKF_LABELVALID) && dk->dk_label &&
> memcmp(dk->dk_label->d_uid, uid,
> sizeof(dk->dk_label->d_uid)) == 0) {
> @@ -1795,8 +1798,10 @@ disk_map(char *path, char *mappath, int
> }
> }
>
> - if (mdk == NULL || mdk->dk_name == NULL)
> + if (mdk == NULL || mdk->dk_name == NULL) {
> + printf("XXX %p\n", mdk);
> return -1;
> + }
>
> snprintf(mappath, size, "/dev/%s%s%c",
> (flags & DM_OPENBLCK) ? "" : "r", mdk->dk_name, part);
>
Race in disk_attach_callback?
Hi,
after a prompt from stsp@ and florian@, reporting that newfs_msdos
fails when given an $duid.i argument, I set down to see what could be
going on. My test using an USB stick failed to reprdouce the problem.
Then I started using a vnd, and that shows the issue (once in a
while). The feeling is that any disk devcied created on the fly might
show this issue.
What I have found out so far: sometimes, disk_subr.c:disk_map()
fails, because the device in question does not have the DKF_LABELVALID
flag set, so it is skipped in the duid search.
The only place where DKF_LABELVALID is set is in
disk_attach_callback().
Often the disk_readlabel() call in this function succeeds and the flag
is set, but also often it does not succeed.
I have observed it failing setting the message to "cannot read disk
label, 0xe32/0x2932, error 25", which seems to come from the
DIOCGPDINFO NOTTY case in dev/vnc.c
This is how far I got.
I am using the test script below. In my case, instrumenting
subr_disk.c with printf made the problem occur less often, so I
suspect a race between the disk attachment and the label becoming
available.
The vnconfig triggers the disk attachment message (but only the first time a
vnd is created, it seems, I am not seeing any messages after a vnconfig
-u and re-running the script).
=== script ===
set -e
dd if=/dev/random of=/tmp/image$1 bs=1m count=100
vnconfig vnd$1 /tmp/image$1
fdisk -ig vnd$1
echo "a\ni\n\n\n\nw\nl\nq\n" | disklabel -E vnd$1
==
Try to run newfs_msdos on the duid.i reported. Note that you need to
reboot to see a change a behaviour on a specific vnd. If it starts out
OK, it keeps on being OK, if it fails, it keeps on failing. This
corresponds to my observation that the disk_attach_callback() is
called only once for a specifc vnd, even after unconfig and reconfig.
Hope somebody who knows how this all shoudl work can see where the
issue is.
-Otto
Index: subr_disk.c
===
RCS file: /cvs/src/sys/kern/subr_disk.c,v
retrieving revision 1.248
diff -u -p -r1.248 subr_disk.c
--- subr_disk.c 2 Jan 2022 17:26:14 - 1.248
+++ subr_disk.c 13 Jul 2022 12:02:05 -
@@ -1118,8 +1118,10 @@ disk_attach_callback(void *xdat)
/* Read disklabel. */
if (disk_readlabel(, dk->dk_devno, errbuf, sizeof(errbuf)) == NULL) {
enqueue_randomness(dl.d_checksum);
+ printf("disk_attach_callback %s: seting DKF_LABELVALID\n",
dk->dk_name);
dk->dk_flags |= DKF_LABELVALID;
}
+ else printf("disk_attach_callback %s: NOT seting DKF_LABELVALID
(%s)\n", dk->dk_name, errbuf);
done:
dk->dk_flags |= DKF_OPENED;
@@ -1785,6 +1787,7 @@ disk_map(char *path, char *mappath, int
mdk = NULL;
TAILQ_FOREACH(dk, , dk_link) {
+ printf("YYY %p %s %x %p\n", dk, dk->dk_name, dk->dk_flags,
dk->dk_label);
if ((dk->dk_flags & DKF_LABELVALID) && dk->dk_label &&
memcmp(dk->dk_label->d_uid, uid,
sizeof(dk->dk_label->d_uid)) == 0) {
@@ -1795,8 +1798,10 @@ disk_map(char *path, char *mappath, int
}
}
- if (mdk == NULL || mdk->dk_name == NULL)
+ if (mdk == NULL || mdk->dk_name == NULL) {
+ printf("XXX %p\n", mdk);
return -1;
+ }
snprintf(mappath, size, "/dev/%s%s%c",
(flags & DM_OPENBLCK) ? "" : "r", mdk->dk_name, part);
Re: quiz(6): update european countries
On Mon, Jul 04, 2022 at 05:44:33PM -0400, Daniel Dickman wrote:
>
>
> On Sun, 3 Jul 2022, Daniel Dickman wrote:
>
> > On Sat, Jul 2, 2022 at 9:26 PM Ben Fuller wrote:
> > >
> > > I noticed Montenegro doesn't have an entry. Presumably this file hasn't
> > > been updated since before 2006!
> >
> > These files could use a big overhaul and are very dated.
> >
> > In the below, make sure to keep the file sorted.
> >
> > In the Europe file there are other issues. For example I think
> > Macedonia is now North Macedonia, etc.
> >
> > There are many issues in the other files. For example africa doesn't
> > have Eswatini, etc, etc.
> >
> > Do you want to take a stab at doing a bigger update of the 4 quiz
> > files? africa, america, europe, asia?
> >
>
> I had some time on my hand and took a stab on this. A few notes:
>
> Added multiple capitals for countries with more than one capital city that
> were missing these:
> - Eswatini
> - South Africa
> - Malaysia
> - Sri Lanka
>
> Removed duplicate entries:
> - Turkey: remove from europe, keep in asia
> - Georgia: remove from europe, keep in asia
Both are debatable. Certainly Instanbul is in Europe.
Many view Georgia as European.
>
> For the above I followed this wikipedia classification:
> https://en.wikipedia.org/wiki/List_of_sovereign_states_and_dependent_territories_by_continent
Which list both Georgia and Turkey as European (and Turkey also as
Asian).
> africa:
> - removed Dahomey as the the pre-1975 name for Benin; it's been close to
> 50 years since the rename.
> - updated Swaziland -> Eswatini. I left Swaziland since Eswatini rename
> happened in 2018. So going by the Benin example, seems like when we get
> closer to 50 years from the rename we can drop the old name
> - added "The" as an optional prefix for Gambia
> - removed "Republic of" from South Africa for consistency with other
> countries which don't generally include the formal prefix as part of the
> name.
>
> americas:
> - added many missing countries
> - added "The" as an optional prefix for Bahamas
> - added missing suffix of "City" from many capitals like Guatemala,
> Mexico, and Panama
>
> asia:
> - added many missing countries in Oceania (like New Zealand, Fiji, etc)
> - added UAE in addition to United Arab Emirates
>
> europe:
> - added Montenegro
> - added Vatican City
> - renamed Macedonia to North Macedonia
> - Yugoslavia -> Serbia
>
> I also looked at NetBSD a bit and noticed they added a lot of territories
> and their capitals, but I decided not to add those in this update.
>
> I also noticed NetBSD removed The Hague as a capital of The Netherlands,
> but I disagree with that. Maybe some of the Dutch folks on this mailing
> list can weigh in which is right.
The Dutch constitiution defines Amsterdam as the capital. The Hague
only is the seat of gorvernment.
-Otto
>
> anyone want to ok this update?
>
> Index: africa
> ===
> RCS file: /cvs/src/games/quiz/datfiles/africa,v
> retrieving revision 1.5
> diff -u -p -u -r1.5 africa
> --- africa2 Dec 2014 12:43:09 - 1.5
> +++ africa4 Jul 2022 21:34:34 -
> @@ -1,6 +1,6 @@
> Algeria:Alg[iers|er]
> Angola:Luanda
> -Benin|Dahomey:Porto[-| ]Novo
> +Benin:Porto[-| ]Novo
> Botswana:Gaborone
> Burkina Faso:Ouagadougou
> Burundi:Bujumbura
> @@ -16,9 +16,10 @@ Djibouti:Djibouti City
> Egypt:Cairo
> Equatorial Guinea:Malabo
> Eritrea:Asmara
> +Eswatini|Swaziland:Mbabane|Lobamba
> Ethiopia:Addis Ababa
> Gabon:Libreville
> -Gambia:Banjul
> +{The }Gambia:Banjul
> Ghana:Accra
> Guinea-Bissau:Bissau
> Guinea:Conakry
> @@ -42,10 +43,9 @@ Senegal:Dakar
> Seychelles:Victoria
> Sierra Leone:Freetown
> Somalia:Mogadishu
> -{Rep{ublic} of }South Africa:Pretoria
> +South Africa:Pretoria|Cape Town|Bloemfontein
> South Sudan:Juba
> Sudan:Khartoum
> -Swaziland:Mbabane
> Tanzania:Dodoma
> Togo:Lome
> Tunisia:Tunis
> Index: america
> ===
> RCS file: /cvs/src/games/quiz/datfiles/america,v
> retrieving revision 1.2
> diff -u -p -u -r1.2 america
> --- america 2 Dec 2014 12:43:09 - 1.2
> +++ america 4 Jul 2022 21:34:34 -
> @@ -1,6 +1,8 @@
> +Antigua and Barbuda:St. John's
> Argentina:Buenos Aires
> -Bahamas:Nassau
> +{The }Bahamas:Nassau
> Barbados:Bridgetown
> +Belize:Belmopan
> Bolivia:La Paz|Sucre
> Bra[z|s]il:Brasilia
> Canada:Ottawa
> @@ -8,19 +10,25 @@ Chile:Santiago
> Colombia:Bogota
> Costa Rica:San Jose
> Cuba:Ha[v|b]ana
> +Dominica:Roseau
> Dominican Republic:Santo Domingo
> Ecuador:Quito
> El Salvador:San Salvador
> -Guatemala:Guatemala
> +Greenland:Nuuk
> +Grenada:St. George's
> +Guatemala:Guatemala City
> Guyana:Georgetown
> -Haiti:Port au Prince
> +Haiti:Port[-| ]au[-| ]Prince
> Honduras:Tegucigalpa
> Jamaica:Kingston
> -Mexico:Mexico
> +Mexico:Mexico City
> Nicaragua:Managua
> -Panama:Panama
> +Panama:Panama
Re: dig(1): SVCB and HTTPS RR types
On Sun, Jul 03, 2022 at 07:47:27AM +0200, Florian Obser wrote:
> anyone?
Looks good and works for me, ok.
-Otto
>
> On 2022-06-25 13:15 +02, Florian Obser wrote:
> > See https://datatracker.ietf.org/doc/draft-ietf-dnsop-svcb-https/
> >
> > $ ./obj/dig @8.8.8.8 +norec _dns.resolver.arpa svcb
> >
> > ; <<>> dig 9.10.8-P1 <<>> @8.8.8.8 +norec _dns.resolver.arpa svcb
> > ; (1 server found)
> > ;; global options: +cmd
> > ;; Got answer:
> > ;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 21245
> > ;; flags: qr aa ra; QUERY: 1, ANSWER: 2, AUTHORITY: 0, ADDITIONAL: 4
> >
> > ;; QUESTION SECTION:
> > ;_dns.resolver.arpa.IN SVCB
> >
> > ;; ANSWER SECTION:
> > _dns.resolver.arpa. 86400 IN SVCB1 dns.google. alpn="dot"
> > _dns.resolver.arpa. 86400 IN SVCB 2 dns.google. alpn="h2,h3"
> > dohpath="/dns-query{?dns}"
> >
> > ;; ADDITIONAL SECTION:
> > dns.google. 86400 IN A 8.8.8.8
> > dns.google. 86400 IN A 8.8.4.4
> > dns.google. 86400 IN 2001:4860:4860::
> > dns.google. 86400 IN 2001:4860:4860::8844
> >
> > ;; Query time: 11 msec
> > ;; SERVER: 8.8.8.8#53(8.8.8.8)
> > ;; WHEN: Sat Jun 25 13:08:21 CEST 2022
> > ;; MSG SIZE rcvd: 224
> >
> > $ ./obj/dig +dnssec cloudflare.com https
> >
> > ; <<>> dig 9.10.8-P1 <<>> +dnssec cloudflare.com https
> > ;; global options: +cmd
> > ;; Got answer:
> > ;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 22508
> > ;; flags: qr rd ra ad; QUERY: 1, ANSWER: 2, AUTHORITY: 0, ADDITIONAL: 0
> >
> > ;; QUESTION SECTION:
> > ;cloudflare.com.IN HTTPS
> >
> > ;; ANSWER SECTION:
> > cloudflare.com. 217 IN HTTPS 1 . alpn="h3,h3-29,h2"
> > ipv4hint=104.16.132.229,104.16.133.229
> > ipv6hint=2606:4700::6810:84e5,2606:4700::6810:85e5
> > cloudflare.com. 217 IN RRSIG HTTPS 13 2 300 20220626120906
> > 20220624100906 34505
> > cloudflare.com. PbQwTGVBW2MIXubouK2vUo92UNvlJ874KCrqah/Or21Jo2oDxfgI15jA
> > 8z/Q6mseLPWIlTxex+KoIqv9y+FNjg==
> >
> > ;; Query time: 0 msec
> > ;; SERVER: 127.0.0.1#53(127.0.0.1)
> > ;; WHEN: Sat Jun 25 13:10:29 CEST 2022
> > ;; MSG SIZE rcvd: 221
> >
> > OK?
>
> diff --git lib/dns/include/dns/types.h lib/dns/include/dns/types.h
> index 63ea8d67f51..7085ce29f2e 100644
> --- lib/dns/include/dns/types.h
> +++ lib/dns/include/dns/types.h
> @@ -139,6 +139,8 @@ enum {
> dns_rdatatype_openpgpkey = 61,
> dns_rdatatype_csync = 62,
> dns_rdatatype_zonemd = 63,
> + dns_rdatatype_svcb = 64,
> + dns_rdatatype_https = 65,
> dns_rdatatype_spf = 99,
> dns_rdatatype_unspec = 103,
> dns_rdatatype_nid = 104,
> diff --git lib/dns/rdata.c lib/dns/rdata.c
> index c27409efc3c..d731eb3a846 100644
> --- lib/dns/rdata.c
> +++ lib/dns/rdata.c
> @@ -775,6 +775,7 @@ dns_rdatatype_fromtext(dns_rdatatype_t *typep,
> isc_textregion_t *source) {
> {"gpos",27},
> {"hinfo", 13},
> {"hip", 55},
> + {"https", 65},
> {"ipseckey",45},
> {"isdn",20},
> {"ixfr",251},
> @@ -822,6 +823,7 @@ dns_rdatatype_fromtext(dns_rdatatype_t *typep,
> isc_textregion_t *source) {
> {"spf", 99},
> {"srv", 33},
> {"sshfp", 44},
> + {"svcb",64},
> {"ta", 32768},
> {"talink", 58},
> {"tkey",249},
> @@ -1006,6 +1008,10 @@ dns_rdatatype_totext(dns_rdatatype_t type,
> isc_buffer_t *target) {
> return (isc_str_tobuffer("CSYNC", target));
> case 63:
> return (isc_str_tobuffer("ZONEMD", target));
> + case 64:
> + return (isc_str_tobuffer("SVCB", target));
> + case 65:
> + return (isc_str_tobuffer("HTTPS", target));
> case 99:
> return (isc_str_tobuffer("SPF", target));
> case 100:
> diff --git lib/dns/rdata/in_1/https_65.c lib/dns/rdata/in_1/https_65.c
> new file mode 100644
> index 000..23d80f8d352
> --- /dev/null
> +++ lib/dns/rdata/in_1/https_65.c
> @@ -0,0 +1,48 @@
> +/*
> + * Copyright (C) 2022 Florian Obser
> + *
> + * Permission to use, copy, modify, and/or distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
> + * REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> MERCHANTABILITY
> + * AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
> + * INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING
> FROM
> + * LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
> + * OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE
Re: a few fixes for cat bugs
On Sat, Jul 02, 2022 at 08:38:53AM +0100, Leah Rowe wrote: > > Hi Otto, > > > Your fixes are not ok. See comment inline. > > The other person (Theo) who responded, raised the same concerns as you. > Sorry for wasting your time. I've reverted the patches myself, locally, > knowing now that I made a few false assumptions. > > I actually overlooked that the buf variable was static. I'm now of the > opinion that OpenBSD's cat implementation is already perfect, and not > in need of new changes. It never hurts going back to code and reread it. To improve you code reviewing skills, I suggest you keep doing it, but be critical of your findings. You'll get better with more experience. Often code reading skills are not valued high enough as opposed to code writing skills, while the former is often more important than the latter, as anybody doing maintenance of a large body of code will tell you. -Otto
Re: a few fixes for cat bugs
On Sat, Jul 02, 2022 at 03:15:21AM +0100, Leah Rowe wrote:
>
> Hi,
>
> I've been playing around with a few OpenBSD userland programs. By that,
> I mean I've been studying them extensively. I'm working on creating a
> busybox-like userland for Linux with musl libc, and I need quality code
> so I decided I'd start ripping code out of OpenBSD for this purpose.
>
> I spent today obsessing over your cat implementation. As part of my
> optimization efforts (I've been removing non-POSIX features, because I
> need the code to be as small as possible), I found several minor issues
> in cat:
>
> 1) Unitialized variables that are assumed to be zero, judging by the
> logic in the code that uses them.
>
> 2) Memory leak; in practise, most people will run can on a few files
> and it won't take more than a few moments, then cat terminates and the
> memory is freed
>
> 3) Unnecessary check in raw_cat on a variable being NULL, when it will
> always evaluate true because it's explicitly already initialized to null
>
> I fixed all of these, in the attached patches which I present for your
> enjoyment. I've done these on top of your github mirror, on top of
> commit 88e033f9985d9aec739c4497f51fb9348247d4db of the main OpenBSD git
> repository (I don't know how to use CVS, I've never used it, sorry! I
> got into programming in the late 2000s right when git was made lol)
Your fixes are not ok. See comment inline.
-Otto
> --
> Leah Rowe,
> Company Director
>
> Minifree Ltd, trading as Ministry of Freedom.
> Registered in England, registration No. 9361826
> VAT Registration No. GB202190462
> Minifree Ltd, 19 Hilton Road, Canvey Island
> Essex SS8 9QA, United Kingdom
> United Kingdom
> From 63163120b6881f9d8ea2b03f722cd31e946f5704 Mon Sep 17 00:00:00 2001
> From: Leah Rowe
> Date: Sat, 2 Jul 2022 02:27:51 +0100
> Subject: [PATCH 1/3] cat: fix uninitialized variables
>
> ---
> bin/cat/cat.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/bin/cat/cat.c b/bin/cat/cat.c
> index a2eff2e1b30..42960812ead 100644
> --- a/bin/cat/cat.c
> +++ b/bin/cat/cat.c
> @@ -62,6 +62,8 @@ main(int argc, char *argv[])
> if (pledge("stdio rpath", NULL) == -1)
> err(1, "pledge");
>
> + rval = bflag = eflag = nflag = sflag = tflag = vflag = 0;
> +
These are globals, which are initialized to zero by the runtime before
main() is called.
> while ((ch = getopt(argc, argv, "benstuv")) != -1) {
> switch (ch) {
> case 'b':
> @@ -210,7 +212,7 @@ void
> raw_cat(int rfd, const char *filename)
> {
> int wfd;
> - ssize_t nr, nw, off;
> + ssize_t nr, off, nw = 0;
nw is initialized by the code below (with the return value of write())
before being used.
> static size_t bsize;
> static char *buf = NULL;
> struct stat sbuf;
> --
> 2.25.1
>
> From e36d05353d66cb42d33f9b5d5eb3e4dc44175e39 Mon Sep 17 00:00:00 2001
> From: Leah Rowe
> Date: Sat, 2 Jul 2022 02:28:33 +0100
> Subject: [PATCH 2/3] cat: fix memory leak
>
> ---
> bin/cat/cat.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/bin/cat/cat.c b/bin/cat/cat.c
> index 42960812ead..dad337d5f68 100644
> --- a/bin/cat/cat.c
> +++ b/bin/cat/cat.c
> @@ -235,4 +235,5 @@ raw_cat(int rfd, const char *filename)
> warn("%s", filename);
> rval = 1;
> }
> + free(buf);
> }
> --
> 2.25.1
>
> From b4e0ef740724523ed3752c6f5b82741d1acc5230 Mon Sep 17 00:00:00 2001
> From: Leah Rowe
> Date: Sat, 2 Jul 2022 02:29:20 +0100
> Subject: [PATCH 3/3] cat: remove unnecessary check
>
> ---
> bin/cat/cat.c | 13 ++---
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/bin/cat/cat.c b/bin/cat/cat.c
> index dad337d5f68..97edcad2101 100644
> --- a/bin/cat/cat.c
> +++ b/bin/cat/cat.c
> @@ -218,13 +218,12 @@ raw_cat(int rfd, const char *filename)
> struct stat sbuf;
>
> wfd = fileno(stdout);
> - if (buf == NULL) {
> - if (fstat(wfd, ) == -1)
> - err(1, "stdout");
> - bsize = MAXIMUM(sbuf.st_blksize, BUFSIZ);
> - if ((buf = malloc(bsize)) == NULL)
> - err(1, NULL);
> - }
You don't seem to understand how static vars work. The idea is that
that raw cat re-uses the buffer as buf is a static. No need to free
it, after process exit the memory will be cleaned up.
> + if (fstat(wfd, ) == -1)
> + err(1, "stdout");
> + bsize = MAXIMUM(sbuf.st_blksize, BUFSIZ);
> + if ((buf = malloc(bsize)) == NULL)
> + err(1, NULL);
> +
> while ((nr = read(rfd, buf, bsize)) != -1 && nr != 0) {
> for (off = 0; nr; nr -= nw, off += nw) {
> if ((nw = write(wfd, buf + off, nr)) == -1 || nw == 0)
> --
> 2.25.1
>
Re: Picky, but much more efficient arc4random_uniform!
On Wed, May 18, 2022 at 05:08:02PM +1000, Damien Miller wrote:
> On Wed, 18 May 2022, Otto Moerbeek wrote:
>
> > instrumenting the code to count the number of arc4random calls I see thsi:
> >
> > openbsd; elapsed = 2.835819; calls = 12340949
> > bitmask; elapsed = 4.335576; calls = 17836216
> > bitmask+reuse; elapsed = 3.710277; calls = 15245337
> >
> > (this is a different number of trials on a slow machine).
> >
> > So the promise of less calls is not fulfilled. Sounds like a bug.
>
> Well, I don't know whether the simple bitmasking approach will really
> result in fewer calls. I *think* our current approach has a higher
> probability per loop of suceeding (at least for small upper_bounds) than
> mask-and-test, but my brain is too addled with a cold to calculate it :/
I *assumed* the speedups seen in the web page were because of less rnd
calls. That was a mistake.
> What Theo said is 100% right - the cost is dominated by that of the
> underlying RNG. If anyone wants a faster arc4random_uniform() then the
> first place to look it at arc4random().
One more reason to keep the current implementation of arc4random_uniform().
>
> It's entirely possible that the speedups measured in that article
> are because they are using a omgoptimised (non-crypto) RNG and the
> improvements they saw were due solely to reduction the overhead inside
> the uniformity code even if it came at the cost of more RNG queries.
Right.
> Anyway, here's a tweaked up version of the test harness that fakes out
> arc4random() with a deterministic RNG that counts how often it is called
> in case anyone wants to play with it further.
-Otto
>
>
>
>
> #include
> #include
>
> #include
> #include
>
> static uint32_t nqueries;
>
> /* deterministic, query-counting arc4random() replacement for benchmarking */
> static uint32_t
> fake_random(void)
> {
> static uint32_t ready, remain, msb;
> uint32_t ret;
>
> if (!ready) {
> srand_deterministic(31337);
> ready = 1;
> }
> if (remain == 0) {
> msb = (uint32_t)rand();
> remain = 31; /* XXX makes assumption re RAND_MAX */
> }
> ret = (uint32_t)rand() | (msb << 31);
> msb >>= 1;
> remain--;
> nqueries++;
> return ret;
> }
>
> #define arc4random() fake_random()
>
> #ifndef __has_builtin
> #define __has_builtin(x) 0
> #endif
> #if __has_builtin(__builtin_clz)
> #define arc4random_clz(x) __builtin_clz(x)
> #else
> #warning lacks __builtin_clz()
> /* Count most-significant zero bits, like __builtin_clz() */
> static int
> arc4random_clz(unsigned int x)
> {
> int ret = 0;
> unsigned int n;
> const int lut[16] = { 4, 3, 2, 2, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0 };
>
> while (x != 0) {
> n = (x >> 28) & 0xf;
> if (n != 0)
> return ret + lut[n];
> x <<= 4;
> ret += 4;
> }
> return 32;
> }
> #endif
>
> /* Current OpenBSD implementation */
> static uint32_t
> arc4random_uniform1(uint32_t upper_bound)
> {
> uint32_t r, min;
>
> if (upper_bound < 2)
> return 0;
>
> /* 2**32 % x == (2**32 - x) % x */
> min = -upper_bound % upper_bound;
>
> /*
>* This could theoretically loop forever but each retry has
>* p > 0.5 (worst case, usually far better) of selecting a
>* number inside the range we need, so it should rarely need
>* to re-roll.
>*/
> for (;;) {
> r = arc4random();
> if (r >= min)
> break;
> }
>
> return r % upper_bound;
> }
>
> /*
> * Like "Bitmask with Rejection" implementation from
> * https://www.pcg-random.org/posts/bounded-rands.html
> */
> static uint32_t
> arc4random_uniform2(uint32_t upper_bound)
> {
> uint32_t mask, r;
>
> if (upper_bound < 2)
> return 0;
>
> mask = 0x >> arc4random_clz((upper_bound - 1) | 1);;
> for (;;) {
> r = arc4random() & mask;
> if (r < upper_bound)
> return r;
> }
> /* NOTREACHED */
> }
>
> /*
> * Like Apple's
> *
> https://opensource.apple.com/source/Libc/Libc-1158.50.2/gen/FreeBSD/arc4random.c
> */
> static uint32_t
> arc4random_uniform3(uint32_t upper_bound)
> {
> int zeroes, bits,
Re: Picky, but much more efficient arc4random_uniform!
On Wed, May 18, 2022 at 02:50:28PM +1000, Damien Miller wrote: > On Tue, 17 May 2022, Raimo Niskanen wrote: > > > Why reinvent the wheel? > > > > Here is a pretty good walkthrough of established methods: > > > > https://www.pcg-random.org/posts/bounded-rands.html > > > > It sounds to me as if your suggested methor essentially is > > "Bitmask with Rejection -- Apple's Method" with the added twist > > to keep the unused bits of the base generator's word and append > > new, which might be an optimization for small ranges, but might > > be just overhead for large ones. > > > > In that post one can see that there might be other suggested smaller > > optimizations that can be applied to the OpenBSD method, and that > > the bitmask method is effective in many cases but not a clear winner. > > Oh nice, I wasn't aware that Apple had an improved algorithm. I always > thought the best avenue for a speedup was to consider only the bits that > could satisfy the constraint, but it never occurred to me how to actually > make use of this :) > > The toy benchmark below compares the existing implementation with > reimplementations of both their version as well as something more close > to Apple's actual method (which reuses the high bits). > > However, I don't see a speedup for either of the alternate approaches. > > [djm@djm ~]$ cc -O2 -Werror -Wall -Wextra -o rb rb.c && doas nice -n -20 ./rb > openbsd; elapsed = 8.327954 > bitmask; elapsed = 13.306228 > bitmask+reuse; elapsed = 11.567389 instrumenting the code to count the number of arc4random calls I see thsi: openbsd; elapsed = 2.835819; calls = 12340949 bitmask; elapsed = 4.335576; calls = 17836216 bitmask+reuse; elapsed = 3.710277; calls = 15245337 (this is a different number of trials on a slow machine). So the promise of less calls is not fulfilled. Sounds like a bug. -Otto
Re: Picky, but much more efficient arc4random_uniform!
On Sun, May 15, 2022 at 08:57:09AM -0300, Crystal Kolipe wrote: > On Sun, May 15, 2022 at 11:44:29AM +0200, Otto Moerbeek wrote: > > On Sun, May 15, 2022 at 04:27:30AM -0500, Luke Small wrote: > > > How did someone prove the current implementation was cryptographically > > > sound? Did they run massive simulations which ran the gamut of the > > > uint32_t > > > range which demanded tight tolerances over varying length runs? > > > > > > How was rc4 cipher proven bad for pseudorandom numbers? I???d be willing > > > to > > > bet it wasn???t from a mathematical proof; it was from bad data. > > > You miss the point completely. The point is: how do you derive a > > uniformly distributed random function for a smaller range, given a > > uniformly distibuted random function over the range [0-2^32-1]. > > > > The current implementation does exactly that and has all the > > information in the comments to verify the uniformity claim. You only > > need to use basic mathematical properties of modulo arithmetic to > > do the verification. > > You do all realise that uniform distribution alone does not make a > good random number generator, don't you? > > For example: > > 1, 2, 3, 4, 5, 1, 2, 3, 4, 5, 1, 2, 3, 4, 5, 1, 2, 3, 4, 5... > > That's uniformly distributed, and also useless as a random number > stream. > > Further more, _cryptographically secure_ random number generation is > not the same as _mathematically good_ random number generation. > > There are plenty of random number generation formulas which are > considered good and useful from a mathematical basis, but which are > useless for cryptography. > > So random, (pun intended), hacks at the arc4random code are not > likely to 'improve' it from the general standpoint, (although if you > have a specific need for a specific private application, that's > different). I think Stuart has already more or less made that point. > I think I can say we know here uniformity is only *one* of the desirable properties of a secure random generator. But I don't think anybody else execpt Luke was talking about "improving". The sole purpose of arc4random_uniform() is to give a good implementation of a random number function in a specific range using arc4random() as the source. This is needed because the naive implementation arc4random() % upper_bound is not uniform. -Otto
Re: Picky, but much more efficient arc4random_uniform!
On Sun, May 15, 2022 at 04:27:30AM -0500, Luke Small wrote:
> I have a feeling that making it threadsafe under any quantity of threads
> and still perform is likely not feasible, but if I could; or if making a
> nonthreadsafe variant was possible:
>
> Creating a mathematical proof that somehow is “simple and probably correct”
> enough, would be an impossible task even for a PhD mathematician.
>
> How did someone prove the current implementation was cryptographically
> sound? Did they run massive simulations which ran the gamut of the uint32_t
> range which demanded tight tolerances over varying length runs?
>
> How was rc4 cipher proven bad for pseudorandom numbers? I’d be willing to
> bet it wasn’t from a mathematical proof; it was from bad data.
>
> I’m guessing that large upperbounds approaching 2**32 don’t feed very
> soundly in the current implementation using a modulus; although I suspect
> that there isn’t much of a call for such things, I’m pretty sure I saw a
> 3,000,000,000 upperbound in the src partition tonight.
You miss the point completely. The point is: how do you derive a
uniformly distributed random function for a smaller range, given a
uniformly distibuted random function over the range [0-2^32-1].
The current implementation does exactly that and has all the
information in the comments to verify the uniformity claim. You only
need to use basic mathematical properties of modulo arithmetic to
do the verification.
-Otto
>
> On Sun, May 15, 2022 at 3:15 AM Otto Moerbeek wrote:
>
> > On Sun, May 15, 2022 at 01:12:28AM -0500, Luke Small wrote:
> >
> > > This is version 1, which I was pretty sure was secure.
> > >
> > > I revamped it with a few features and implanted the binary search for
> > 'bit'
> > >
> > > in most cases, which aren't intentionally worst-case, it's pretty darned
> > > fast!
> > >
> > > This is a sample run of my program with your piece of code included:
> > >
> > >
> > > 1 99319 100239
> > > 2 100353 99584
> > > 3 100032 99879
> > > 4 99704 100229
> > > 5 99530 99796
> > > 6 99617 100355
> > > 7 100490 100319
> > > 8 99793 100114
> > > 9 100426 99744
> > > 10 100315 100558
> > > 11 99279 99766
> > > 12 99572 99750
> > > 13 99955 100017
> > > 14 100413 15
> > > 15 100190 100052
> > > 16 101071 100195
> > > 17 100322 100224
> > > 18 99637 99540
> > > 19 100323 99251
> > > 20 99841 100177
> > > 21 99948 99504
> > > 22 100065 100031
> > > 23 100026 99827
> > > 24 99836 99818
> > > 25 100245 99822
> > > 26 100088 99678
> > > 27 99957 3
> > > 28 100065 99961
> > > 29 100701 100679
> > > 30 99756 99587
> > > 31 100220 100076
> > > 32 100067 15
> > > 33 99547 99984
> > > 34 100124 100031
> > > 35 99547 100661
> > > 36 99801 99963
> > > 37 100189 100230
> > > 38 99878 99579
> > > 39 99864 99442
> > > 40 99683 14
> > > 41 99907 100094
> > > 42 100291 99817
> > > 43 99908 99984
> > > 44 100044 100606
> > > 45 100065 100120
> > > 46 99358 100141
> > > 47 100152 100442
> > > 48 10 100279
> > > 49 100486 99848
> >
> > Sadly a sample run cannot be used to proof your implementation is
> > correct. It can only be used to show it is not correct, like Philip
> > did. To show your implementation produces uniform results in all
> > cases, you need to provide a solid argumentation that is easy to
> > follow. So far you failed to do that and I do not see it coming, given
> > the complexituy of your implementation. The current implementation
> > has a straightforward argumentation as it uses relatively simple
> > mathematical properties of modulo arithmetic.
> >
> > It is also clear your code (as it uses statics) is not thread safe.
> >
> > So to answer you original question clearly: no, we will not accept this.
> >
> > -Otto
> >
> --
> -Luke
Re: Picky, but much more efficient arc4random_uniform!
On Sun, May 15, 2022 at 01:12:28AM -0500, Luke Small wrote: > This is version 1, which I was pretty sure was secure. > > I revamped it with a few features and implanted the binary search for 'bit' > > in most cases, which aren't intentionally worst-case, it's pretty darned > fast! > > This is a sample run of my program with your piece of code included: > > > 1 99319 100239 > 2 100353 99584 > 3 100032 99879 > 4 99704 100229 > 5 99530 99796 > 6 99617 100355 > 7 100490 100319 > 8 99793 100114 > 9 100426 99744 > 10 100315 100558 > 11 99279 99766 > 12 99572 99750 > 13 99955 100017 > 14 100413 15 > 15 100190 100052 > 16 101071 100195 > 17 100322 100224 > 18 99637 99540 > 19 100323 99251 > 20 99841 100177 > 21 99948 99504 > 22 100065 100031 > 23 100026 99827 > 24 99836 99818 > 25 100245 99822 > 26 100088 99678 > 27 99957 3 > 28 100065 99961 > 29 100701 100679 > 30 99756 99587 > 31 100220 100076 > 32 100067 15 > 33 99547 99984 > 34 100124 100031 > 35 99547 100661 > 36 99801 99963 > 37 100189 100230 > 38 99878 99579 > 39 99864 99442 > 40 99683 14 > 41 99907 100094 > 42 100291 99817 > 43 99908 99984 > 44 100044 100606 > 45 100065 100120 > 46 99358 100141 > 47 100152 100442 > 48 10 100279 > 49 100486 99848 Sadly a sample run cannot be used to proof your implementation is correct. It can only be used to show it is not correct, like Philip did. To show your implementation produces uniform results in all cases, you need to provide a solid argumentation that is easy to follow. So far you failed to do that and I do not see it coming, given the complexituy of your implementation. The current implementation has a straightforward argumentation as it uses relatively simple mathematical properties of modulo arithmetic. It is also clear your code (as it uses statics) is not thread safe. So to answer you original question clearly: no, we will not accept this. -Otto
Re: Picky, but much more efficient arc4random_uniform!
On Sat, May 14, 2022 at 05:03:17PM -0700, Philip Guenther wrote:
> On Sun, 15 May 2022, Steffen Nurpmeso wrote:
> > Stuart Henderson wrote in
> ...
> > |what's the perceived problem you're wanting to solve? and does that
> > |problem actually exist in the first place?
> >
> > The problem is that if have a low upper bound then modulo will "remove a
> > lot of randomization". For example if you have a program which
> > generates Lotto numbers (1..49), then using _uniform() as it is will
> > generate many duplicates.
>
> Wut. The *WHOLE POINT* of arc4random_uniform() is that it has uniform
> distribution. Says right so in the manpage. If an implementation of that
> API fails to do that, it's a broken implementation.
>
> The OpenBSD implementation achieves that. NetBSD's implementation has the
> same core logic. Here's my quick lotto pick demo program, doing 490
> picks, so they should all be somewhere near 10:
>
> #include
> #include
> #define LIMIT 49
> int
> main(void)
> {
> unsigned counts[LIMIT] = { 0 };
> int i;
> for (i = 0; i < 10 * LIMIT; i++)
> counts[arc4random_uniform(LIMIT)]++;
> for (i = 0; i < LIMIT; i++)
> printf("%2d\t%7u\n", i+1, counts[i]);
> return 0;
> }
>
> And a sample run:
>
> : bleys; ./a.out
>
> 1 100100
> 2 100639
> 399965
> 499729
> 599641
> 699650
> 7 100299
> 8 100164
> 999791
> 10 100101
> 11 100657
> 12 100042
> 1399661
> 1499927
> 1599426
> 1699491
> 1799646
> 18 100133
> 19 100013
> 2099942
> 2199873
> 2299924
> 2399567
> 24 100152
> 25 100688
> 26 100011
> 27 100481
> 2899980
> 29 100406
> 3099726
> 3199808
> 3299929
> 33 100050
> 3499983
> 35 100048
> 3699771
> 3799906
> 38 100215
> 39 100261
> 40 100426
> 4199847
> 4299533
> 43 100368
> 4499695
> 45 100041
> 46 100465
> 4799875
> 48 100034
> 4999920
> : bleys;
>
> Looks pretty good to me, with repeated runs showing the values bouncing
> around.
>
>
> ...
> > I was a bit interested when i saw Luke's message, but i am no
> > mathematician and, like i said, i in fact never needed the
> > functionality. And the question i would have is how small
> > upper_bound has to be for the modulo problem to really kick in.
>
> If the implementation isn't broken, it's not a problem, period.
>
>
> > And even if, whether it matters. For example, if you have
> > a bitset of 1024 "in-flight things", and the small upper bound
> > hits a used slot, you could simply search forward until you find
> > a free one. I mean, with 1024 possible values the number of
> > possibilities is anyhow restricted.
>
> Well hey, let's give Luke's a try and see how uniform it is:
>
>
> #include
> #include
> #define LIMIT 49
> int
> main(void)
> {
> unsigned counts[LIMIT] = { 0 };
> unsigned counts2[LIMIT] = { 0 };
> int i;
> for (i = 0; i < 10 * LIMIT; i++) {
> counts[arc4random_uniform(LIMIT)]++;
> counts2[arc4random_uniform_fast_simple(LIMIT)]++;
> }
> for (i = 0; i < LIMIT; i++)
> printf("%2d\t%7u\t%7u\n", i+1, counts[i], counts2[i]);
> return 0;
> }
>
> : bleys; ./a.out
> 1 100188 76502
> 299983 76602
> 3 100521 76522
> 4 100416 76682
> 5 100171 76387
> 6 100163 76759
> 7 100024 76336
> 8 19 76703
> 999769 76237
> 1099892 76532
> 11 100197 76730
> 12 100483 76398
> 1399769 76310
> 14 100075 76474
> 1599781 76599
> 1699846 76439
> 1799814 76430
> 18 100313 76648
> 19 100259 76813
> 2099885 77068
> 21 100302 76546
> 228 76698
> 2399491 76678
> 24 100340 76324
> 2599763 115263
> 2699872 153008
> 27 100022 152979
> 2899481 153793
> 29 100018 210714
> 3099617 229286
> 31 100167 297003
> 32 100270 449664
> 33 100468 76790
> 3499115 76452
> 3599921 76392
> 3699862 76140
> 37 100485 76607
> 38 100029 75885
> 3999577 76498
> 4099479 76727
> 41 100139 76746
> 42 100883 76698
> 43 100102 76474
> 4499801 76592
> 45 100117 76124
> 4699678 76417
> 4799770 76639
> 4899524 77034
> 49 100151 76658
> : bleys;
>
> Wow, that last column is *bad*.
Re: Picky, but much more efficient arc4random_uniform!
On Sat, May 14, 2022 at 05:48:10AM -0500, Luke Small wrote: > arc4random_uniform_fast2 that I made, streams in data from arc4random() and > uses the datastream directly and uses it as a bit by bit right "sliding > window" in the last loop. arc4random_uniform() uses a modulus which I is > simple to implement, but I wonder how cryptographically sound or even how > evenly it distributes. Adding a modulus seems sloppy without something > better. I did make arc4random_fast_simple() which merely takes an > upperbound. I integrated arc4random_uniform_fast_bitsearch() or whatever > the top function was into it which binary searches to find the correct size > bitfield (return value) needed to barely fit the upperbound while also > being able to discover every possible value below the upperbound. It isn't > as fast as arc4random_uniform_fast2 if it were used repeatedly after a > single use of arc4random_uniform_fast_bitsearch() , but it does exactly the > same thing and appears faster than repeatedly using arc4random_uniform() > and it's wasteful use of arc4random() and calling the expensive rekeying > function more often. > > It may be interesting to determine even without looking at performance, > whether arc4random_fast_simple() creates a more superior, secure use of the > chacha20 stream than arc4random_uniform() with the modulus. what exactly > does all that extra data from the modulus do to the random distribution? > > -Luke I don't follow you at all. Your blabbering does not even use the terms "uniform" and "modulo bias". I wonder even if you realize what they mean in this context. -Otto
Re: Picky, but much more efficient arc4random_uniform!
In general, wasting some arc4random bits is not a problem at all.
I see a lot of code with no demonstration, explanation or proof why it
would be correct (i.e. produces uniform results). You only talk about
speed.
The current code might not be optimal, but at least it is very clear
it's correct.
-Otto
On Fri, May 13, 2022 at 09:43:26AM -0500, Luke Small wrote:
> I made a couple new versions of a new kind of arc4random_uniform-like
> function and some example functions which use them. Instead of having a
> sufficiently large random number greater than the modulus, I pick a random
> number using arc4random() from a bitfield where the length of the bitfield
> is just below or slightly beyond the value of the modulus and returns the
> bitfield it if it is less than the value of the modulus.
>
> In both versions, I retrieve a bitfield from a static uint64_t which is
> refreshed from periodic arc4random() calls. The functions demand a bit
> length. but I provide a convenient bitsize search function:
> arc4random_uniform_fast_bitsearch()
>
> in the first version, if the chosen return value isn't less than the
> modulus, the entire bitfield is trashed and a completely new bitfield is
> refreshed from the cache. This can be very inefficient and for certain
> upperbounds where the functions demand that all returned values less than
> the upperbound are possible. This can perform worse than
> arc4random_uniform() even with more random number generation churn.
>
> in the second version, if the chosen return value isn't less than the
> modulus, the bitfield is shifted right, losing the least significant bit
> and a new random-value bit from the least significant bit of the cache is
> put into the most significant bit of the bitfield and it tries again. For
> significant expenditures of effort, this version is always more efficient
> than arc4random_uniform() and slightly to much more efficient than my first
> version.
>
>
> The performance boost comes from less pseudorandom number generation by not
> trashing perfectly good random data and preserving it so that rekeying is
> performed less.
>
>
> I suspect that the first version may be secure, but I'm now sure what you
> think of the second version, for being secure. Is there a way to test how
> well distributed and secure it is?!
>
> Perhaps it's even better than the typical use case of arc4random_uniform
> since it feeds it a bitstream instead of performing a modulous operation on
> it!
>
> I pledge("stdio") and unveil(NULL, NULL) at the beginning, so you know it
> doesn't do anything but malloc() an array, do stuff on the array and print
> stuff; if you don't profile, anyway.
>
> What do you think of having such a thing in OpenBSD?
>
>
>
>
> newdata() generates random a-z characters performing arc4random_uniform(26);
>
> newdataTypableFilename() generates random filename typable characters
> performing arc4random_uniform(92);
>
> I perform other tests including presumed worst-cases on large numbers and
> numbers which will have a lot of misses.
>
>
>
>
> -Luke
Re: stop using mquery(2) inside realloc(3)
On Fri, May 13, 2022 at 08:21:19PM -0700, Philip Guenther wrote:
> If you try to grow a 'large' (at least half a page) allocation with
> realloc(3), it'll try to extend the memory mapping for it and if that
> works it won't need to move it.
(on a side note: it does not do anything at all if the # of pages does
not grow).
>
> Currently, it does that by using mquery(2) to see if that area is
> available and if so then trying to mmap it, munmaping it if mmap(2) didn't
> return the desired address (perhaps due to a race with another thread).
> Instead of doing mquery/mmap/munmap, this path can use the MAP_FIXED and
> __MAP_NOREPLACE flags to directly request the specific address but not
> change anything if it's not available.
>
>
> (We still use mquery in ld.so on i386 as a performance optimization, but
> that use case differs in needing to find a base address for *multiple*
> mappings, where unrolling partial hits grows to be more expensive than
> probing with mquery and only trying to do all the mapping on a successful
> set of probes: recall that on x86 munmap() is more expensive than mmap()
> due to TLB shootdowns. This case in realloc() only has a single mapping to
> probe/extend so it avoids those costs. Indeed, this diff eliminates the
> current "munmap on failed attempt" path/cost.)
>
>
> The regress/lib/libc/malloc tests continue to pass on my amd64 box, with
> ktrace confirming the change in calls.
>
> oks?
I was looking through the commit log to see when this flag was introduced.
It was in sys/mman.h rev 1.21, but a bit hidden becuase the commit
message talks about __MAP_NOREMAP.
Anyway, I cannot oversee if it is relevant if all pmap implementations
implement this or if this is done in a higher layer.
Also a tiny bit worried the __MAP_NOREPLACE path in uvm isn't
exercised as much, so it might reveal bugs. But thats no reason not to
do this now.
So if my first question can be answered with: it's in a higher layer,
OK.
-Otto
>
>
> Philip
>
>
> Index: stdlib/malloc.c
> ===
> RCS file: /data/src/openbsd/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.273
> diff -u -p -r1.273 malloc.c
> --- stdlib/malloc.c 26 Feb 2022 16:14:42 - 1.273
> +++ stdlib/malloc.c 14 May 2022 02:35:04 -
> @@ -100,9 +100,6 @@
> #define MMAPA(a,sz,f)mmap((a), (sz), PROT_READ | PROT_WRITE, \
> MAP_ANON | MAP_PRIVATE | (f), -1, 0)
>
> -#define MQUERY(a,sz,f) mquery((a), (sz), PROT_READ | PROT_WRITE, \
> -MAP_ANON | MAP_PRIVATE | MAP_FIXED | (f), -1, 0)
> -
> struct region_info {
> void *p;/* page; low bits used to mark chunks */
> uintptr_t size; /* size for pages, or chunk_info pointer */
> @@ -1687,11 +1684,7 @@ orealloc(struct dir_info **argpool, void
> size_t needed = rnewsz - roldsz;
>
> STATS_INC(pool->cheap_realloc_tries);
> - q = MQUERY(hint, needed, pool->mmap_flag);
> - if (q == hint)
> - q = MMAPA(hint, needed,
> pool->mmap_flag);
> - else
> - q = MAP_FAILED;
> + q = MMAPA(hint, needed, MAP_FIXED |
> __MAP_NOREPLACE | pool->mmap_flag);
> if (q == hint) {
> STATS_ADD(pool->malloc_used, needed);
> if (pool->malloc_junk == 2)
> @@ -1709,9 +1702,6 @@ orealloc(struct dir_info **argpool, void
> STATS_INC(pool->cheap_reallocs);
> ret = p;
> goto done;
> - } else if (q != MAP_FAILED) {
> - if (munmap(q, needed))
> - wrterror(pool, "munmap %p", q);
> }
> }
> } else if (rnewsz < roldsz) {
>
Re: pf igmp icmp6 multicast router alert
On Thu, Apr 28, 2022 at 12:36:40AM +0200, Alexander Bluhm wrote:
> On Wed, Apr 27, 2022 at 11:47:45PM +0200, Alexander Bluhm wrote:
> > New diff:
> > - make off and end relative to opts array
> > - check length of IPv4 options
> > - fix call to pf_walk_option
> > - add case IP6OPT_PADN
> > - add case MLDV2_LISTENER_REPORT
>
> - pf_pull_hdr() before pf_walk_option6() was missing
>
> ok?
tested this version on my home net that had this issue after
installing a new CPE. It works fine. Can't comment on the
code itself, this is too far out of my comfort zone.
-Otto
>
> bluhm
>
> Index: net/pf.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
> retrieving revision 1.1126
> diff -u -p -r1.1126 pf.c
> --- net/pf.c 17 Mar 2022 18:27:55 - 1.1126
> +++ net/pf.c 27 Apr 2022 22:28:38 -
> @@ -227,6 +227,8 @@ u_int16_t pf_calc_mss(struct pf_addr *
> static __inline int pf_set_rt_ifp(struct pf_state *, struct pf_addr *,
> sa_family_t, struct pf_src_node **);
> struct pf_divert *pf_get_divert(struct mbuf *);
> +int pf_walk_option(struct pf_pdesc *, struct ip *,
> + int, int, u_short *);
> int pf_walk_header(struct pf_pdesc *, struct ip *,
> u_short *);
> int pf_walk_option6(struct pf_pdesc *, struct ip6_hdr *,
> @@ -3956,7 +3958,7 @@ pf_test_rule(struct pf_pdesc *pd, struct
> rtable_l2(ctx.act.rtableid) != pd->rdomain)
> pd->destchg = 1;
>
> - if (r->action == PF_PASS && pd->badopts && ! r->allow_opts) {
> + if (r->action == PF_PASS && pd->badopts != 0 && ! r->allow_opts) {
> REASON_SET(, PFRES_IPOPTIONS);
> #if NPFLOG > 0
> pd->pflog |= PF_LOG_FORCE;
> @@ -6382,6 +6384,55 @@ pf_get_divert(struct mbuf *m)
> }
>
> int
> +pf_walk_option(struct pf_pdesc *pd, struct ip *h, int off, int end,
> +u_short *reason)
> +{
> + uint8_t type, length, opts[15 * 4 - sizeof(struct ip)];
> +
> + KASSERT(end - off <= sizeof(opts));
> + m_copydata(pd->m, off, end - off, opts);
> + end -= off;
> + off = 0;
> +
> + while (off < end) {
> + type = opts[off];
> + if (type == IPOPT_EOL)
> + break;
> + if (type == IPOPT_NOP) {
> + off++;
> + continue;
> + }
> + if (off + 2 > end) {
> + DPFPRINTF(LOG_NOTICE, "IP length opt");
> + REASON_SET(reason, PFRES_IPOPTIONS);
> + return (PF_DROP);
> + }
> + length = opts[off + 1];
> + if (length < 2) {
> + DPFPRINTF(LOG_NOTICE, "IP short opt");
> + REASON_SET(reason, PFRES_IPOPTIONS);
> + return (PF_DROP);
> + }
> + if (off + length > end) {
> + DPFPRINTF(LOG_NOTICE, "IP long opt");
> + REASON_SET(reason, PFRES_IPOPTIONS);
> + return (PF_DROP);
> + }
> + switch (type) {
> + case IPOPT_RA:
> + SET(pd->badopts, PF_OPT_ROUTER_ALERT);
> + break;
> + default:
> + SET(pd->badopts, PF_OPT_OTHER);
> + break;
> + }
> + off += length;
> + }
> +
> + return (PF_PASS);
> +}
> +
> +int
> pf_walk_header(struct pf_pdesc *pd, struct ip *h, u_short *reason)
> {
> struct ip6_ext ext;
> @@ -6393,11 +6444,20 @@ pf_walk_header(struct pf_pdesc *pd, stru
> REASON_SET(reason, PFRES_SHORT);
> return (PF_DROP);
> }
> - if (hlen != sizeof(struct ip))
> - pd->badopts++;
> + if (hlen != sizeof(struct ip)) {
> + if (pf_walk_option(pd, h, pd->off + sizeof(struct ip),
> + pd->off + hlen, reason) != PF_PASS)
> + return (PF_DROP);
> + /* header options which contain only padding is fishy */
> + if (pd->badopts == 0)
> + SET(pd->badopts, PF_OPT_OTHER);
> + }
> end = pd->off + ntohs(h->ip_len);
> pd->off += hlen;
> pd->proto = h->ip_p;
> + /* IGMP packets have router alert options, allow them */
> + if (pd->proto == IPPROTO_IGMP)
> + CLR(pd->badopts, PF_OPT_ROUTER_ALERT);
> /* stop walking over non initial fragments */
> if ((h->ip_off & htons(IP_OFFMASK)) != 0)
> return (PF_PASS);
> @@ -6455,7 +6515,10 @@ pf_walk_option6(struct pf_pdesc *pd, str
> return (PF_DROP);
> }
> switch (opt.ip6o_type) {
> + case IP6OPT_PADN:
> + break;
> case IP6OPT_JUMBO:
> +
Re: errata70.html - update the 7.1 link
On Fri, Apr 22, 2022 at 07:22:18PM -0400, Horia Racoviceanu wrote: > - change 7.1 link from errata70.html to errata71.html > Index: errata70.html > === > RCS file: /cvs/www/errata70.html,v > retrieving revision 1.25 > diff -u -p -r1.25 errata70.html > --- errata70.html 21 Apr 2022 03:23:04 - 1.25 > +++ errata70.html 22 Apr 2022 23:10:33 - > @@ -74,7 +74,7 @@ For errata on a certain release, click b > > 6.8, > 6.9, > -7.1. > +7.1. > > > That should keep both 7.0. and 7.1 lines -Otto
Re: pf igmp icmp6 multicast router alert
On Thu, Apr 21, 2022 at 07:08:38PM +0200, Alexander Bluhm wrote:
> Hi,
>
> IGMP and ICMP6 for multicast packets have router alert options.
> Per default pf drops all IP packets with options. Usually people
> ask what is wrong until someone points out that they have to use a
> pf rule with allow-opts.
>
> As this is normal behavior and our kernel generates such packets,
> the pf default is bad.
>
> Diff is untested, but otto@ and florian@ could try it.
Tested this with success on two hosts that hasd issues because their
outgoing icmp6 messages were blcoked.
> Currently it allows all options. Should I make it specific to
> router alert with IGMP or ICMP6?
To me it looks like the icmp6 case already is limited to MLD?
-Otto
>
> bluhm
>
> Index: net/pf.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
> retrieving revision 1.1126
> diff -u -p -r1.1126 pf.c
> --- net/pf.c 17 Mar 2022 18:27:55 - 1.1126
> +++ net/pf.c 21 Apr 2022 16:30:18 -
> @@ -6398,6 +6398,9 @@ pf_walk_header(struct pf_pdesc *pd, stru
> end = pd->off + ntohs(h->ip_len);
> pd->off += hlen;
> pd->proto = h->ip_p;
> + /* IGMP packets have router alert options, allow them */
> + if (pd->proto == IPPROTO_IGMP)
> + pd->badopts = 0;
> /* stop walking over non initial fragments */
> if ((h->ip_off & htons(IP_OFFMASK)) != 0)
> return (PF_PASS);
> @@ -6494,6 +6497,7 @@ pf_walk_header6(struct pf_pdesc *pd, str
> {
> struct ip6_frag frag;
> struct ip6_ext ext;
> + struct icmp6_hdr icmp6;
> struct ip6_rthdr rthdr;
> u_int32_tend;
> int hdr_cnt, fraghdr_cnt = 0, rthdr_cnt = 0;
> @@ -6607,9 +6611,23 @@ pf_walk_header6(struct pf_pdesc *pd, str
> pd->off += (ext.ip6e_len + 1) * 8;
> pd->proto = ext.ip6e_nxt;
> break;
> + case IPPROTO_ICMPV6:
> + if (!pf_pull_hdr(pd->m, pd->off, , sizeof(icmp6),
> + NULL, reason, AF_INET6)) {
> + DPFPRINTF(LOG_NOTICE, "IPv6 short icmp6hdr");
> + return (PF_DROP);
> + }
> + /* ICMP multicast packets have router alert options */
> + switch (icmp6.icmp6_type) {
> + case MLD_LISTENER_QUERY:
> + case MLD_LISTENER_REPORT:
> + case MLD_LISTENER_DONE:
> + pd->badopts = 0;
> + break;
> + }
> + /* FALLTHROUGH */
> case IPPROTO_TCP:
> case IPPROTO_UDP:
> - case IPPROTO_ICMPV6:
> /* fragments may be short, ignore inner header then */
> if (pd->fragoff != 0 && end < pd->off +
> (pd->proto == IPPROTO_TCP ? sizeof(struct tcphdr) :
>
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)
> > @@
Re: fix very small ntpd leak
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? :-)
This is my diff that uses an approach I like a litle bit better.
-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)
fatal("couldn't bind to IPv4 query address: %s",
log_sockaddr(qa4));
} else if (p->addr->ss.ss_family == qa6->sa_family) {
- if (bind(p->query->fd, qa6, SA_LEN(qa6)) == -1)
+ if (bind(p->query.fd, qa6, SA_LEN(qa6)) == -1)
fatal("couldn't bind to IPv6 query address: %s",
log_sockaddr(qa6));
}
- if (connect(p->query->fd, sa, SA_LEN(sa)) == -1) {
+ if (connect(p->query.fd, sa, SA_LEN(sa)) == -1) {
if (errno == ECONNREFUSED || errno == ENETUNREACH ||
errno == EHOSTUNREACH || errno == EADDRNOTAVAIL) {
/* cycle through
Re: request for testing: malloc and large allocations
On Tue, Feb 01, 2022 at 08:00:36AM +0100, Otto Moerbeek wrote:
> On Fri, Jan 28, 2022 at 05:17:48PM +0100, Otto Moerbeek wrote:
>
> > On Fri, Jan 28, 2022 at 04:33:28PM +0100, Alexander Bluhm wrote:
> >
> > > On Sun, Jan 09, 2022 at 02:54:43PM +0100, Otto Moerbeek wrote:
> > > > currently malloc does cache a number of free'ed regions up to 128k in
> > > > size. This cache is indexed by size (in # of pages), so it is very
> > > > quick to check.
> > > >
> > > > Some programs allocate and deallocate larger allocations in a frantic
> > > > way. Accodomate those programs by also keeping a cache of regions
> > > > betwen 128k and 2M, in a cache of variable sized regions.
> > > >
> > > > My test case speeds up about twice. A make build gets a small speedup.
> > > >
> > > > This has been tested by myself on amd64 quite intensively. I am asking
> > > > for more tests, especialy on more "exotic" platforms. I wil do arm64
> > > > myself soon. Test can be running your favorite programs, doing make
> > > > builds or running tests in regress/lib/libc/malloc.
> > >
> > > I see openssl and tmux crash with this diff.
> > > /usr/src/regress/usr.sbin/openssl reproduces it on arm64, amd64,
> > > i386.
> >
> > Are you running with any malloc flags?
>
> This bug report enabled me to find a bug that would pop up if G mode
> is enabled.
>
> New diff below. New tests appreciated.
This has been in snaps for a while.
Any body willing to review and OK?
-Otto
> Index: stdlib/malloc.c
> ===
> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.272
> diff -u -p -r1.272 malloc.c
> --- stdlib/malloc.c 19 Sep 2021 09:15:22 - 1.272
> +++ stdlib/malloc.c 31 Jan 2022 16:27:31 -
> @@ -113,13 +113,27 @@ struct region_info {
>
> LIST_HEAD(chunk_head, chunk_info);
>
> -#define MAX_CACHEABLE_SIZE 32
> -struct cache {
> - void *pages[MALLOC_MAXCACHE];
> +/*
> + * Two caches, one for "small" regions, one for "big".
> + * Small cache is an array per size, big cache is one array with different
> + * sized regions
> + */
> +#define MAX_SMALLCACHEABLE_SIZE 32
> +#define MAX_BIGCACHEABLE_SIZE512
> +/* If the total # of pages is larger than this, evict before inserting */
> +#define BIGCACHE_FILL(sz)(MAX_BIGCACHEABLE_SIZE * (sz) / 4)
> +
> +struct smallcache {
> + void **pages;
> ushort length;
> ushort max;
> };
>
> +struct bigcache {
> + void *page;
> + size_t psize;
> +};
> +
> struct dir_info {
> u_int32_t canary1;
> int active; /* status of malloc */
> @@ -139,7 +153,10 @@ struct dir_info {
> void *delayed_chunks[MALLOC_DELAYED_CHUNK_MASK + 1];
> u_char rbytes[32]; /* random bytes */
> /* free pages cache */
> - struct cache cache[MAX_CACHEABLE_SIZE];
> + struct smallcache smallcache[MAX_SMALLCACHEABLE_SIZE];
> + size_t bigcache_used;
> + size_t bigcache_size;
> + struct bigcache *bigcache;
> #ifdef MALLOC_STATS
> size_t inserts;
> size_t insert_collisions;
> @@ -207,7 +224,7 @@ struct malloc_readonly {
> #ifdef MALLOC_STATS
> int malloc_stats; /* dump statistics at end */
> #endif
> - u_int32_t malloc_canary;/* Matched against ones in malloc_pool
> */
> + u_int32_t malloc_canary;/* Matched against ones in pool */
> };
>
> /* This object is mapped PROT_READ after initialisation to prevent tampering
> */
> @@ -714,18 +731,61 @@ unmap(struct dir_info *d, void *p, size_
> size_t psz = sz >> MALLOC_PAGESHIFT;
> void *r;
> u_short i;
> - struct cache *cache;
> + struct smallcache *cache;
>
> if (sz != PAGEROUND(sz) || psz == 0)
> wrterror(d, "munmap round");
>
> - if (psz > MAX_CACHEABLE_SIZE || d->cache[psz - 1].max == 0) {
> + if (d->bigcache_size > 0 && psz > MAX_SMALLCACHEABLE_SIZE &&
> + psz <= MAX_BIGCACHEABLE_SIZE) {
> + u_short base = getrbyte(d);
> + u_short j;
> +
> + /* don't look through all slots */
> + for (j = 0; j < d->bigcache_size / 4; j++) {
> + i = (base + j) % d->bigcache_size;
> + if (d->bigcache_used <
> + BIGCACHE_
Re: tcpdump core's on the latest snapshot
On Sun, Feb 13, 2022 at 12:54:19PM +0100, Otto Moerbeek wrote: > On Sun, Feb 13, 2022 at 01:12:34PM +0300, Mikhail wrote: > > > Running this command on the latest snapshot produces core file for me: > > > > doas tcpdump -i urtwn0 proto ip6 BTW, the proper way to filter ipv6 packets is either doas tcpdump -i urtwn0 ip6 or doas tcpdump -i urtwn0 ether proto 0x86dd The ethernet protocol numbers are defined in /usr/include/net/ethertypes.h -Otto
Re: tcpdump core's on the latest snapshot
On Sun, Feb 13, 2022 at 01:12:34PM +0300, Mikhail wrote:
> Running this command on the latest snapshot produces core file for me:
>
> doas tcpdump -i urtwn0 proto ip6
>
> Core details:
>
> misha:/home/misha:3959$ doas lldb --core tcpdump.core tcpdump
> (lldb) target create "tcpdump" --core "tcpdump.core"
> Core file '/home/misha/tcpdump.core' (x86_64) was loaded.
> (lldb) bt
> * thread #1, stop reason = signal SIGSEGV
> * frame #0: 0x07b299d82607 libpcap.so.9.0`pcap_compile [inlined]
> freechunks at gencode.c:209:9
> frame #1: 0x07b299d825c2 libpcap.so.9.0`pcap_compile(p=,
> program=, buf=, optimize=,
> mask=) at gencode.c:287:3
> frame #2: 0x07aff4d7a74f tcpdump`___lldb_unnamed_symbol311 + 159
> frame #3: 0x07aff4d783a6 tcpdump`___lldb_unnamed_symbol286 + 3510
> frame #4: 0x07aff4d73f61 tcpdump`___lldb_unnamed_symbol259 + 97
> frame #5: 0x07aff4d73b32 tcpdump`___lldb_unnamed_symbol253 + 290
>
> It looks like setjmp() call on gencode.c:286 returns "1", while the man
> page says it must return only "0".
I can see two problems:
1. setjump returning 1
2. freechunks() segfaulting.
Here I'll concentrate on 2), as I suspect 1) has a cause that is already in
the process of being diagnosed/fixed elsewhere.
The offensding statement is:
#0 0x002428da032c in freechunks () at /usr/src/lib/libpcap/gencode.c:209
209 free(membag[i].ptrs[j]);
(gdb) print membag
$1 = {{total = 0, slot = 0, ptrs = 0x0} }
It looks like this happens when no allocation has happened at all.
The diff below fixes the core dump for me.
-Otto
Index: gencode.c
===
RCS file: /cvs/src/lib/libpcap/gencode.c,v
retrieving revision 1.59
diff -u -p -r1.59 gencode.c
--- gencode.c 5 Dec 2021 16:40:24 - 1.59
+++ gencode.c 13 Feb 2022 11:52:55 -
@@ -205,6 +205,8 @@ freechunks(void)
int i, j;
for (i = 0; i <= cur_membag; i++) {
+ if (membag[i].ptrs == NULL)
+ continue;
for (j = 0; j <= membag[i].slot; j++)
free(membag[i].ptrs[j]);
free(membag[i].ptrs);
Re: request for testing: malloc and large allocations
On Sat, Feb 05, 2022 at 08:07:42PM +0100, Jan Stary wrote: > On Feb 05 17:35:46, o...@drijf.net wrote: > > On Sat, Feb 05, 2022 at 05:22:50PM +0100, Jan Stary wrote: > > > > > On Feb 02 00:04:37, alexander.bl...@gmx.net wrote: > > > > On Tue, Feb 01, 2022 at 08:00:36AM +0100, Otto Moerbeek wrote: > > > > > > Are you running with any malloc flags? > > > > > > > > > > This bug report enabled me to find a bug that would pop up if G mode > > > > > is enabled. > > > > > > > > > > New diff below. New tests appreciated. > > > > > > > > > Passed a make build on macppc (Mac Mini, dmesg below). > > > > > > 4288m32.43s real 3309m41.24s user 802m02.37s system > > > Recompiling now with the new malloc to see the difference. > > > > Note that during a make build, libs get installed after building them > > so all dynamically linked programs will use the new malloc from that > > point, as there is no revision bump. > > Does that mean that both passes of compiling the compiler > (i.e., compiling clang with the current clang, > and then compiling clang with the new clang, > if I remember correctly) already use the new libc? > > (Compiling clang has taken most of the ~3 days of make build.) Clang does a single pass afaik, it does not do a full bootstrap. That means clang will be compiled with the currently installed (old) clang, but that clang will use the newly built and installed libc. -Otto
Re: request for testing: malloc and large allocations
On Sat, Feb 05, 2022 at 05:22:50PM +0100, Jan Stary wrote: > On Feb 02 00:04:37, alexander.bl...@gmx.net wrote: > > On Tue, Feb 01, 2022 at 08:00:36AM +0100, Otto Moerbeek wrote: > > > > Are you running with any malloc flags? > > > > > > This bug report enabled me to find a bug that would pop up if G mode > > > is enabled. > > > > > > New diff below. New tests appreciated. > > > Passed a make build on macppc (Mac Mini, dmesg below). > > 4288m32.43s real 3309m41.24s user 802m02.37s system > Recompiling now with the new malloc to see the difference. Note that during a make build, libs get installed after building them so all dynamically linked programs will use the new malloc from that point, as there is no revision bump. -Otto > > > Jan > > > [ using 1308496 bytes of bsd ELF symbol table ] > console out [ATY,RockHopper2_A] console in [keyboard], using USB > using parent ATY,RockHopper2Paren:: memaddr 9800, size 800 : consaddr > 9c008000 : ioaddr 9002, size 2: width 1920 linebytes 2048 height 1080 > depth 8 > Copyright (c) 1982, 1986, 1989, 1991, 1993 > The Regents of the University of California. All rights reserved. > Copyright (c) 1995-2022 OpenBSD. All rights reserved. https://www.OpenBSD.org > > OpenBSD 7.0-current (GENERIC) #0: Mon Jan 31 13:22:27 CET 2022 > h...@ppc.stare.cz:/usr/src/sys/arch/macppc/compile/GENERIC > real mem = 1073741824 (1024MB) > avail mem = 1025527808 (978MB) > random: good seed from bootblocks > mpath0 at root > scsibus0 at mpath0: 256 targets > mainbus0 at root: model PowerMac10,2 > cpu0 at mainbus0: 7447A (Revision 0x102): 1499 MHz: 512KB L2 cache > mem0 at mainbus0 > spdmem0 at mem0: 1GB DDR SDRAM non-parity PC3200CL3.0 > memc0 at mainbus0: uni-n rev 0xd2 > "hw-clock" at memc0 not configured > kiic0 at memc0 offset 0xf8001000 > iic0 at kiic0 > mpcpcibr0 at mainbus0 pci: uni-north > pci0 at mpcpcibr0 bus 0 > pchb0 at pci0 dev 11 function 0 "Apple UniNorth AGP" rev 0x00 > agp at pchb0 not configured > radeondrm0 at pci0 dev 16 function 0 "ATI Radeon 9200" rev 0x01 > drm0 at radeondrm0 > radeondrm0: irq 48 > mpcpcibr1 at mainbus0 pci: uni-north > pci1 at mpcpcibr1 bus 0 > macobio0 at pci1 dev 23 function 0 "Apple Intrepid" rev 0x00 > openpic0 at macobio0 offset 0x4: version 0x4614 feature 3f0302 LE > macgpio0 at macobio0 offset 0x50 > "modem-reset" at macgpio0 offset 0x1d not configured > "modem-power" at macgpio0 offset 0x1c not configured > macgpio1 at macgpio0 offset 0x9: irq 47 > "programmer-switch" at macgpio0 offset 0x11 not configured > "gpio5" at macgpio0 offset 0x6f not configured > "gpio6" at macgpio0 offset 0x70 not configured > "extint-gpio15" at macgpio0 offset 0x67 not configured > "escc-legacy" at macobio0 offset 0x12000 not configured > zs0 at macobio0 offset 0x13000: irq 22,23 > zstty0 at zs0 channel 0 > zstty1 at zs0 channel 1 > aoa0 at macobio0 offset 0x1: irq 30,1,2 > "timer" at macobio0 offset 0x15000 not configured > adb0 at macobio0 offset 0x16000 > apm0 at adb0: battery flags 0x0, 0% charged > piic0 at adb0 > iic1 at piic0 > maxtmp0 at iic1 addr 0xc8: max6642 > kiic1 at macobio0 offset 0x18000 > iic2 at kiic1 > wdc0 at macobio0 offset 0x2 irq 24: DMA > audio0 at aoa0 > ohci0 at pci1 dev 26 function 0 "Apple Intrepid USB" rev 0x00: irq 29, > version 1.0, legacy support > ohci1 at pci1 dev 27 function 0 "NEC USB" rev 0x43: irq 63, version 1.0 > ohci2 at pci1 dev 27 function 1 "NEC USB" rev 0x43: irq 63, version 1.0 > ehci0 at pci1 dev 27 function 2 "NEC USB" rev 0x04: irq 63 > usb0 at ehci0: USB revision 2.0 > uhub0 at usb0 configuration 1 interface 0 "NEC EHCI root hub" rev 2.00/1.00 > addr 1 > usb1 at ohci0: USB revision 1.0 > uhub1 at usb1 configuration 1 interface 0 "Apple OHCI root hub" rev 1.00/1.00 > addr 1 > usb2 at ohci1: USB revision 1.0 > uhub2 at usb2 configuration 1 interface 0 "NEC OHCI root hub" rev 1.00/1.00 > addr 1 > usb3 at ohci2: USB revision 1.0 > uhub3 at usb3 configuration 1 interface 0 "NEC OHCI root hub" rev 1.00/1.00 > addr 1 > mpcpcibr2 at mainbus0 pci: uni-north > pci2 at mpcpcibr2 bus 0 > kauaiata0 at pci2 dev 13 function 0 "Apple Intrepid ATA" rev 0x00 > wdc1 at kauaiata0 irq 39: DMA > wd0 at wdc1 channel 0 drive 0: > wd0: 16-sector PIO, LBA48, 152627MB, 312581808 sectors > wd0(wdc1:0:0): using PIO mode 4, DMA mode 2, Ultra-DMA mode 5 > "Apple UniNorth Firewire" rev 0x81 at pci2 dev 14 f
Re: request for testing: malloc and large allocations
On Fri, Jan 28, 2022 at 05:17:48PM +0100, Otto Moerbeek wrote:
> On Fri, Jan 28, 2022 at 04:33:28PM +0100, Alexander Bluhm wrote:
>
> > On Sun, Jan 09, 2022 at 02:54:43PM +0100, Otto Moerbeek wrote:
> > > currently malloc does cache a number of free'ed regions up to 128k in
> > > size. This cache is indexed by size (in # of pages), so it is very
> > > quick to check.
> > >
> > > Some programs allocate and deallocate larger allocations in a frantic
> > > way. Accodomate those programs by also keeping a cache of regions
> > > betwen 128k and 2M, in a cache of variable sized regions.
> > >
> > > My test case speeds up about twice. A make build gets a small speedup.
> > >
> > > This has been tested by myself on amd64 quite intensively. I am asking
> > > for more tests, especialy on more "exotic" platforms. I wil do arm64
> > > myself soon. Test can be running your favorite programs, doing make
> > > builds or running tests in regress/lib/libc/malloc.
> >
> > I see openssl and tmux crash with this diff.
> > /usr/src/regress/usr.sbin/openssl reproduces it on arm64, amd64,
> > i386.
>
> Are you running with any malloc flags?
This bug report enabled me to find a bug that would pop up if G mode
is enabled.
New diff below. New tests appreciated.
-Otto
Index: stdlib/malloc.c
===
RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.272
diff -u -p -r1.272 malloc.c
--- stdlib/malloc.c 19 Sep 2021 09:15:22 - 1.272
+++ stdlib/malloc.c 31 Jan 2022 16:27:31 -
@@ -113,13 +113,27 @@ struct region_info {
LIST_HEAD(chunk_head, chunk_info);
-#define MAX_CACHEABLE_SIZE 32
-struct cache {
- void *pages[MALLOC_MAXCACHE];
+/*
+ * Two caches, one for "small" regions, one for "big".
+ * Small cache is an array per size, big cache is one array with different
+ * sized regions
+ */
+#define MAX_SMALLCACHEABLE_SIZE32
+#define MAX_BIGCACHEABLE_SIZE 512
+/* If the total # of pages is larger than this, evict before inserting */
+#define BIGCACHE_FILL(sz) (MAX_BIGCACHEABLE_SIZE * (sz) / 4)
+
+struct smallcache {
+ void **pages;
ushort length;
ushort max;
};
+struct bigcache {
+ void *page;
+ size_t psize;
+};
+
struct dir_info {
u_int32_t canary1;
int active; /* status of malloc */
@@ -139,7 +153,10 @@ struct dir_info {
void *delayed_chunks[MALLOC_DELAYED_CHUNK_MASK + 1];
u_char rbytes[32]; /* random bytes */
/* free pages cache */
- struct cache cache[MAX_CACHEABLE_SIZE];
+ struct smallcache smallcache[MAX_SMALLCACHEABLE_SIZE];
+ size_t bigcache_used;
+ size_t bigcache_size;
+ struct bigcache *bigcache;
#ifdef MALLOC_STATS
size_t inserts;
size_t insert_collisions;
@@ -207,7 +224,7 @@ struct malloc_readonly {
#ifdef MALLOC_STATS
int malloc_stats; /* dump statistics at end */
#endif
- u_int32_t malloc_canary;/* Matched against ones in malloc_pool
*/
+ u_int32_t malloc_canary;/* Matched against ones in pool */
};
/* This object is mapped PROT_READ after initialisation to prevent tampering */
@@ -714,18 +731,61 @@ unmap(struct dir_info *d, void *p, size_
size_t psz = sz >> MALLOC_PAGESHIFT;
void *r;
u_short i;
- struct cache *cache;
+ struct smallcache *cache;
if (sz != PAGEROUND(sz) || psz == 0)
wrterror(d, "munmap round");
- if (psz > MAX_CACHEABLE_SIZE || d->cache[psz - 1].max == 0) {
+ if (d->bigcache_size > 0 && psz > MAX_SMALLCACHEABLE_SIZE &&
+ psz <= MAX_BIGCACHEABLE_SIZE) {
+ u_short base = getrbyte(d);
+ u_short j;
+
+ /* don't look through all slots */
+ for (j = 0; j < d->bigcache_size / 4; j++) {
+ i = (base + j) % d->bigcache_size;
+ if (d->bigcache_used <
+ BIGCACHE_FILL(d->bigcache_size)) {
+ if (d->bigcache[i].psize == 0)
+ break;
+ } else {
+ if (d->bigcache[i].psize != 0)
+ break;
+ }
+ }
+ /* if we didn't find a preferred slot, use random one */
+ if (d->bigcache[i].psize != 0) {
+ size_t tmp;
+
+ r = d->bigcache[i].page;
+ d->bigcache_used -= d->bigcache[i].psize;
+
Re: UBSan instrumentation vs -fno-wrapv
On Sun, Jan 30, 2022 at 04:46:36PM -0800, Greg Steuck wrote:
> In case somebody hits this, here's a resolved issue: -fno-wrapv is
> matters for UBSan coverage.
>
> Confusion starts with:
>
> $ uname -srm; cat a.c && clang -fsanitize=undefined a.c -c -o a.o && nm a.o
> OpenBSD 7.0 amd64
> int main(int argc, char **argv) {
> int k = 0x7fff;
> k += argc;
> return 0;
> }
> W __llvm_retpoline_r11
> W __retguard_2371
> F a.c
> T main
>
> Notice the lack of `__ubsan` symbols. Adding -fno-wrav (which I found in
> kernel Makefile.amd64) restores the desired instrumentation:
>
> % clang -fsanitize=undefined -fno-wrapv a.c -c -o a.o && nm a.o
> W __llvm_retpoline_r11
> W __retguard_2371
> U __ubsan_handle_add_overflow
> F a.c
> T main
>
With -fwrapv the addition is no longer undefined behaviour, so the
compiler does not need to insert the __ubsan_handle_add_overflow hook.
-Otto
Re: request for testing: malloc and large allocations
On Fri, Jan 28, 2022 at 04:33:28PM +0100, Alexander Bluhm wrote:
> On Sun, Jan 09, 2022 at 02:54:43PM +0100, Otto Moerbeek wrote:
> > currently malloc does cache a number of free'ed regions up to 128k in
> > size. This cache is indexed by size (in # of pages), so it is very
> > quick to check.
> >
> > Some programs allocate and deallocate larger allocations in a frantic
> > way. Accodomate those programs by also keeping a cache of regions
> > betwen 128k and 2M, in a cache of variable sized regions.
> >
> > My test case speeds up about twice. A make build gets a small speedup.
> >
> > This has been tested by myself on amd64 quite intensively. I am asking
> > for more tests, especialy on more "exotic" platforms. I wil do arm64
> > myself soon. Test can be running your favorite programs, doing make
> > builds or running tests in regress/lib/libc/malloc.
>
> I see openssl and tmux crash with this diff.
> /usr/src/regress/usr.sbin/openssl reproduces it on arm64, amd64,
> i386.
Are you running with any malloc flags?
-Otto
>
> #0 L1 () at /usr/src/lib/libc/arch/amd64/string/memset.S:52
> 52 L1: rep
> (gdb) bt
> #0 L1 () at /usr/src/lib/libc/arch/amd64/string/memset.S:52
> #1 0x0fde4ed058d8 in _libc_explicit_bzero (buf=0xfded1f8e000,
> len=18446744073709549600) at /usr/src/lib/libc/string/explicit_bzero.c:17
> #2 0x0fde4ed6f84f in unmap (d=0xfdead893830, p=Variable "p" is not
> available.
> ) at /usr/src/lib/libc/stdlib/malloc.c:805
> #3 0x0fde4ed6ceca in ofree (argpool=0x7f7c2268, p=Variable "p" is
> not available.
> ) at /usr/src/lib/libc/stdlib/malloc.c:1511
> #4 0x0fde4ed6e4cb in _libc_recallocarray (ptr=0xfded1f8e7e0,
> oldnmemb=Variable "oldnmemb" is not available.
> ) at /usr/src/lib/libc/stdlib/malloc.c:1908
> #5 0x0fdbd4787afe in xrecallocarray (ptr=Unhandled dwarf expression
> opcode 0xa3
> ) at /usr/src/usr.bin/tmux/xmalloc.c:81
> #6 0x0fdbd4703ce6 in cmd_parse_build_commands (cmds=Unhandled dwarf
> expression opcode 0xa3
> ) at cmd-parse.y:815
> #7 0x0fdbd4703d47 in cmd_parse_build_commands (cmds=Unhandled dwarf
> expression opcode 0xa3
> ) at cmd-parse.y:823
> #8 0x0fdbd47040f7 in cmd_parse_from_buffer (buf=Unhandled dwarf
> expression opcode 0xa3
> ) at cmd-parse.y:1036
> #9 0x0fdbd4703ffd in cmd_parse_from_string (
> s=0xfdbd46d6522 "bind > { display-menu -xP -yP -T
> '#[align=centre]#{pane_index} (#{pane_id})'
> '#{?#{m/r:(copy|view)-mode,#{pane_mode}},Go To Top,}' '<' {send -X
> history-top} '#{?#{m/r:(copy|view)-mode,#{pane_mode}},G"...,
> pi=0x7f7c24a0) at cmd-parse.y:959
> #10 0x0fdbd473506e in key_bindings_init () at
> /usr/src/usr.bin/tmux/key-bindings.c:636
> #11 0x0fdbd47564c8 in server_start (client=0xfdec1056000,
> flags=402653184, base=0xfdec103f400, lockfd=5, lockfile=0xfdec1041b80
> "/tmp/tmux-0/default.lock")
> at /usr/src/usr.bin/tmux/server.c:210
> #12 0x0fdbd46f8788 in client_main (base=0xfdec103f400, argc=Unhandled
> dwarf expression opcode 0xa3
> ) at /usr/src/usr.bin/tmux/client.c:165
> #13 0x0fdbd4761b62 in main (argc=0, argv=0x7f7c2880) at
> /usr/src/usr.bin/tmux/tmux.c:529
>
> bluhm
Re: request for testing: malloc and large allocations
On Sat, Jan 22, 2022 at 09:25:25AM +0100, Otto Moerbeek wrote: > On Mon, Jan 17, 2022 at 08:42:47AM +0100, Otto Moerbeek wrote: > > > On Sun, Jan 09, 2022 at 02:54:43PM +0100, Otto Moerbeek wrote: > > > > > Hi, > > > > > > currently malloc does cache a number of free'ed regions up to 128k in > > > size. This cache is indexed by size (in # of pages), so it is very > > > quick to check. > > > > > > Some programs allocate and deallocate larger allocations in a frantic > > > way. Accodomate those programs by also keeping a cache of regions > > > betwen 128k and 2M, in a cache of variable sized regions. > > > > > > My test case speeds up about twice. A make build gets a small speedup. > > > > > > This has been tested by myself on amd64 quite intensively. I am asking > > > for more tests, especialy on more "exotic" platforms. I wil do arm64 > > > myself soon. Test can be running your favorite programs, doing make > > > builds or running tests in regress/lib/libc/malloc. > > > > > > Thanks in advance! > > > > > > I received several success reports and one failure on macppc report > > from Miod. I'm investiging that report to see if this diff can be > > blamed. > > > > In the meantime: keep on testing! > > > > -Otto > > So far I have been unable to reproduce. I would like confirmation > either way from somebody else having a macppc machine. Any volunteer? > > Thanks, > > -Otto It turns out the macppc troubles have ben resolved by an uvm commit. -Otto
Re: request for testing: malloc and large allocations
On Mon, Jan 17, 2022 at 08:42:47AM +0100, Otto Moerbeek wrote: > On Sun, Jan 09, 2022 at 02:54:43PM +0100, Otto Moerbeek wrote: > > > Hi, > > > > currently malloc does cache a number of free'ed regions up to 128k in > > size. This cache is indexed by size (in # of pages), so it is very > > quick to check. > > > > Some programs allocate and deallocate larger allocations in a frantic > > way. Accodomate those programs by also keeping a cache of regions > > betwen 128k and 2M, in a cache of variable sized regions. > > > > My test case speeds up about twice. A make build gets a small speedup. > > > > This has been tested by myself on amd64 quite intensively. I am asking > > for more tests, especialy on more "exotic" platforms. I wil do arm64 > > myself soon. Test can be running your favorite programs, doing make > > builds or running tests in regress/lib/libc/malloc. > > > > Thanks in advance! > > > I received several success reports and one failure on macppc report > from Miod. I'm investiging that report to see if this diff can be > blamed. > > In the meantime: keep on testing! > > -Otto So far I have been unable to reproduce. I would like confirmation either way from somebody else having a macppc machine. Any volunteer? Thanks, -Otto
Re: request for testing: malloc and large allocations
On Sun, Jan 09, 2022 at 02:54:43PM +0100, Otto Moerbeek wrote: > Hi, > > currently malloc does cache a number of free'ed regions up to 128k in > size. This cache is indexed by size (in # of pages), so it is very > quick to check. > > Some programs allocate and deallocate larger allocations in a frantic > way. Accodomate those programs by also keeping a cache of regions > betwen 128k and 2M, in a cache of variable sized regions. > > My test case speeds up about twice. A make build gets a small speedup. > > This has been tested by myself on amd64 quite intensively. I am asking > for more tests, especialy on more "exotic" platforms. I wil do arm64 > myself soon. Test can be running your favorite programs, doing make > builds or running tests in regress/lib/libc/malloc. > > Thanks in advance! I received several success reports and one failure on macppc report from Miod. I'm investiging that report to see if this diff can be blamed. In the meantime: keep on testing! -Otto
request for testing: malloc and large allocations
Hi,
currently malloc does cache a number of free'ed regions up to 128k in
size. This cache is indexed by size (in # of pages), so it is very
quick to check.
Some programs allocate and deallocate larger allocations in a frantic
way. Accodomate those programs by also keeping a cache of regions
betwen 128k and 2M, in a cache of variable sized regions.
My test case speeds up about twice. A make build gets a small speedup.
This has been tested by myself on amd64 quite intensively. I am asking
for more tests, especialy on more "exotic" platforms. I wil do arm64
myself soon. Test can be running your favorite programs, doing make
builds or running tests in regress/lib/libc/malloc.
Thanks in advance!
-Otto
Index: stdlib/malloc.c
===
RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.272
diff -u -p -r1.272 malloc.c
--- stdlib/malloc.c 19 Sep 2021 09:15:22 - 1.272
+++ stdlib/malloc.c 9 Jan 2022 13:10:35 -
@@ -113,13 +113,28 @@ struct region_info {
LIST_HEAD(chunk_head, chunk_info);
-#define MAX_CACHEABLE_SIZE 32
-struct cache {
- void *pages[MALLOC_MAXCACHE];
+/*
+ * Two caches, one for "small" regions, one for "big".
+ * Small cacche is an array per size, big cache is one array with different
+ * sizes regions
+ */
+#define MAX_SMALLCACHEABLE_SIZE32
+#define MAX_BIGCACHEABLE_SIZE 512
+#define BIGCACHE_SIZE MALLOC_MAXCACHE
+/* If the total # of pages is larger than this, evict before inserting */
+#define BIGCACHE_FILL(sz) (MAX_BIGCACHEABLE_SIZE * (sz) / 4)
+
+struct smallcache {
+ void **pages;
ushort length;
ushort max;
};
+struct bigcache {
+ void *page;
+ size_t psize;
+};
+
struct dir_info {
u_int32_t canary1;
int active; /* status of malloc */
@@ -139,7 +154,10 @@ struct dir_info {
void *delayed_chunks[MALLOC_DELAYED_CHUNK_MASK + 1];
u_char rbytes[32]; /* random bytes */
/* free pages cache */
- struct cache cache[MAX_CACHEABLE_SIZE];
+ struct smallcache smallcache[MAX_SMALLCACHEABLE_SIZE];
+ ushort bigcache_size;
+ size_t bigcache_used;
+ struct bigcache bigcache[BIGCACHE_SIZE];
#ifdef MALLOC_STATS
size_t inserts;
size_t insert_collisions;
@@ -714,18 +732,61 @@ unmap(struct dir_info *d, void *p, size_
size_t psz = sz >> MALLOC_PAGESHIFT;
void *r;
u_short i;
- struct cache *cache;
+ struct smallcache *cache;
if (sz != PAGEROUND(sz) || psz == 0)
wrterror(d, "munmap round");
- if (psz > MAX_CACHEABLE_SIZE || d->cache[psz - 1].max == 0) {
+ if (d->bigcache_size > 0 && psz > MAX_SMALLCACHEABLE_SIZE &&
+ psz <= MAX_BIGCACHEABLE_SIZE) {
+ u_short base = getrbyte(d);
+ u_short j;
+
+ /* don't look through all slots */
+ for (j = 0; j < d->bigcache_size / 4; j++) {
+ i = (base + j) % d->bigcache_size;
+ if (d->bigcache_used <
+ BIGCACHE_FILL(d->bigcache_size)) {
+ if (d->bigcache[i].psize == 0)
+ break;
+ } else {
+ if (d->bigcache[i].psize != 0)
+ break;
+ }
+ }
+ /* if we didn't find a preferred slot, use random one */
+ if (d->bigcache[i].psize != 0) {
+ size_t tmp;
+
+ r = d->bigcache[i].page;
+ d->bigcache_used -= d->bigcache[i].psize;
+ tmp = d->bigcache[i].psize << MALLOC_PAGESHIFT;
+ if (!mopts.malloc_freeunmap)
+ validate_junk(d, r, tmp);
+ if (munmap(r, tmp))
+wrterror(d, "munmap %p", r);
+ STATS_SUB(d->malloc_used, tmp);
+ }
+
+ if (clear > 0)
+ explicit_bzero(p, clear);
+ if (mopts.malloc_freeunmap) {
+ if (mprotect(p, sz, PROT_NONE))
+ wrterror(d, "mprotect %p", r);
+ } else
+ junk_free(d->malloc_junk, p, sz);
+ d->bigcache[i].page = p;
+ d->bigcache[i].psize = psz;
+ d->bigcache_used += psz;
+ return;
+ }
+ if (psz > MAX_SMALLCACHEABLE_SIZE || d->smallcache[psz - 1].max == 0) {
if (munmap(p, sz))
wrterror(d, "munmap %p", p);
STATS_SUB(d->malloc_used, sz);
return;
}
- cache = >cache[psz - 1];
+ cache =
Re: cat(1): always use a 64K buffer
On Mon, Dec 13, 2021 at 02:52:50AM -0600, Scott Cheloha wrote: > > On Dec 13, 2021, at 01:13, Otto Moerbeek wrote: > > > > On Sun, Dec 12, 2021 at 07:15:51PM -0600, Scott Cheloha wrote: > > > >> cat(1) sizes its I/O buffer according to the st_blksize of the first > >> file it processes. We don't do this very often in the tree. I'm not > >> sure if we should trust st_blksize. > >> > >> It would be simpler to just choose a value that works in practice and > >> always use it. > >> > >> 64K works well. We settled on that for tee(1) recently. > > > > Why are you also change the allocation to be per-file? You might as > > well go for a static buffer if you make it fixed size. > > Ottomalloc does canary checks when free(3) > is called if malloc_options is set up for it. > I always thought that was useful when > auditing my code. In this case (large allocation an exact multiple of the page size) any overflow will be caught by the MMU, independent of malloc options. So no big gain here to be gained from calling free. I'd say keep the original one time malloc call. -Otto
Re: cat(1): always use a 64K buffer
On Sun, Dec 12, 2021 at 07:15:51PM -0600, Scott Cheloha wrote:
> cat(1) sizes its I/O buffer according to the st_blksize of the first
> file it processes. We don't do this very often in the tree. I'm not
> sure if we should trust st_blksize.
>
> It would be simpler to just choose a value that works in practice and
> always use it.
>
> 64K works well. We settled on that for tee(1) recently.
Why are you also change the allocation to be per-file? You might as
well go for a static buffer if you make it fixed size.
-Otto
>
> Thoughts?
>
> Index: cat.c
> ===
> RCS file: /cvs/src/bin/cat/cat.c,v
> retrieving revision 1.32
> diff -u -p -r1.32 cat.c
> --- cat.c 24 Oct 2021 21:24:21 - 1.32
> +++ cat.c 13 Dec 2021 00:57:48 -
> @@ -33,9 +33,6 @@
> * SUCH DAMAGE.
> */
>
> -#include
> -#include
> -
> #include
> #include
> #include
> @@ -45,7 +42,7 @@
> #include
> #include
>
> -#define MAXIMUM(a, b)(((a) > (b)) ? (a) : (b))
> +#define BSIZE (64 * 1024)
>
> int bflag, eflag, nflag, sflag, tflag, vflag;
> int rval;
> @@ -221,26 +218,20 @@ raw_args(char **argv)
> void
> raw_cat(int rfd, const char *filename)
> {
> - int wfd;
> + char *buf;
> ssize_t nr, nw, off;
> - static size_t bsize;
> - static char *buf = NULL;
> - struct stat sbuf;
> + int wfd;
>
> wfd = fileno(stdout);
> - if (buf == NULL) {
> - if (fstat(wfd, ) == -1)
> - err(1, "stdout");
> - bsize = MAXIMUM(sbuf.st_blksize, BUFSIZ);
> - if ((buf = malloc(bsize)) == NULL)
> - err(1, NULL);
> - }
> - while ((nr = read(rfd, buf, bsize)) != -1 && nr != 0) {
> + if ((buf = malloc(BSIZE)) == NULL)
> + err(1, NULL);
> + while ((nr = read(rfd, buf, BSIZE)) != -1 && nr != 0) {
> for (off = 0; nr; nr -= nw, off += nw) {
> if ((nw = write(wfd, buf + off, nr)) == -1 || nw == 0)
> err(1, "stdout");
> }
> }
> + free(buf);
> if (nr == -1) {
> warn("%s", filename);
> rval = 1;
>
Re: asr(3): strip AD flag in responses
On Sun, Nov 21, 2021 at 04:51:45PM +0100, Florian Obser wrote:
> On 2021-11-20 21:16 +01, Otto Moerbeek wrote:
> > On Sat, Nov 20, 2021 at 06:44:58PM +0100, Florian Obser wrote:
> >
> >> On 2021-11-20 18:41 +01, Florian Obser wrote:
> >> > On 2021-11-20 18:19 +01, Florian Obser wrote:
> >> >
> >> >> +/*
> >> >> + * Clear AD flag in the answer.
> >> >> + */
> >> >> +static void
> >> >> +clear_ad(struct asr_result *ar)
> >> >> +{
> >> >> + struct asr_dns_header *h;
> >> >> + uint16_t flags;
> >> >> +
> >> >> + h = (struct asr_dns_header *)ar->ar_data;
> >> >> + flags = ntohs(h->flags);
> >> >> + flags &= ~(AD_MASK);
> >> >> + h->flags = htons(flags);
> >> >> +}
> >> >> +
> >> >
> >> > btw. is it possible that this is not alligned correctly on sparc64?
> >> >
> >> > should be do something like (not even compile tested)
> >> >
> >> > static void
> >> > clear_ad(struct asr_result *ar)
> >> > {
> >> > struct asr_dns_headerh;
> >> >
> >> > memmove(, ar->ar_data, sizeof(h));
> >> > h.flags = ntohs(h.flags);
> >> > h.flags &= ~(AD_MASK);
> >> > h.flags = htons(h.flags);
> >> > memmove(ar->ar_data, , sizeof(h));
> >> > }
> >> >
> >>
> >> memcpy obviously, I was distracted by the copious amount of memmove in
> >> asr code...
> >
> > It is not needed to copy the "whole" header just to change the flags.
> > You could just copy out, modify and copy back the flags field only.
> >
> > otoh, it's just 12 bytes, so no big deal.
>
> right. So I have tried my patch (without the memcpy dance) on sparc64
> over udp and tcp and I have also tracked this down in the code. This
> should be fine as is. ar->ar_data comes directly out of malloc
> (reallocarray) in ensure_ibuf() and the struct is defined thusly:
>
> struct asr_dns_header {
> uint16_tid;
> uint16_tflags;
> uint16_tqdcount;
> uint16_tancount;
> uint16_tnscount;
> uint16_tarcount;
> };
>
So that is indeed safe as long as nobody starts allocating packet
buffers in different ways,
-Otto
Re: asr(3): strip AD flag in responses
On Sat, Nov 20, 2021 at 06:44:58PM +0100, Florian Obser wrote:
> On 2021-11-20 18:41 +01, Florian Obser wrote:
> > On 2021-11-20 18:19 +01, Florian Obser wrote:
> >
> >> +/*
> >> + * Clear AD flag in the answer.
> >> + */
> >> +static void
> >> +clear_ad(struct asr_result *ar)
> >> +{
> >> + struct asr_dns_header *h;
> >> + uint16_t flags;
> >> +
> >> + h = (struct asr_dns_header *)ar->ar_data;
> >> + flags = ntohs(h->flags);
> >> + flags &= ~(AD_MASK);
> >> + h->flags = htons(flags);
> >> +}
> >> +
> >
> > btw. is it possible that this is not alligned correctly on sparc64?
> >
> > should be do something like (not even compile tested)
> >
> > static void
> > clear_ad(struct asr_result *ar)
> > {
> > struct asr_dns_headerh;
> >
> > memmove(, ar->ar_data, sizeof(h));
> > h.flags = ntohs(h.flags);
> > h.flags &= ~(AD_MASK);
> > h.flags = htons(h.flags);
> > memmove(ar->ar_data, , sizeof(h));
> > }
> >
>
> memcpy obviously, I was distracted by the copious amount of memmove in
> asr code...
It is not needed to copy the "whole" header just to change the flags.
You could just copy out, modify and copy back the flags field only.
otoh, it's just 12 bytes, so no big deal.
-Otto
Re: asr(3): strip AD flag in responses
On Sat, Nov 20, 2021 at 02:40:59PM +0100, Otto Moerbeek wrote:
> On Sat, Nov 20, 2021 at 12:20:32PM +0100, Florian Obser wrote:
>
> > The Authentic Data (AD) flag indicates that the nameserver validated
> > the response using DNSSEC. For clients to trust this the nameserver
> > and the path to the nameserver must be trusted. In the general case
> > this is not true.
> >
> > We can trust localhost so we set the AD flag on queries to request
> > validation and preserve the AD flag in answers. (*)
> >
> > If, and only if, trusted nameservers (that are not on localhost) have
> > been added to resolv.conf and the path to them is secure the trust-ad
> > flag may be used to request validation from them and trust answers with
> > the AD flag set.
> >
> > The trust-ad option first appeared in glibc 2.31.
> > ( https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/461 and
> > https://man7.org/linux/man-pages/man5/resolv.conf.5.html )
> >
> > Thomas Habets (thomas at habets.se) pointed out on bugs@ that
> > VerifyHostKeyDNS in ssh only works with unwind (which is good) but
> > only by accident (which is bad).
> > https://marc.info/?t=16371749593=1=2
> >
> > *) This is for people running unwind, unbound or some other validating
> > resolver on localhost. Yes, it is possible that someone set up some sort
> > of forwarder where they trust the DNS answers but not that they are
> > DNSSEC validated. This feels contrived and a case of DON'T DO THAT!
> >
> > OK?
>
> I like this much better than the sketch I posted on bugs@
>
> Two comment wrt the docs inline.
>
> Code looks and tests good.
>
> -Otto
>
> >
> > diff --git include/resolv.h include/resolv.h
> > index fb02483871e..2422deb5484 100644
> > --- include/resolv.h
> > +++ include/resolv.h
> > @@ -191,6 +191,7 @@ struct __res_state_ext {
> > /* DNSSEC extensions: use higher bit to avoid conflict with ISC use */
> > #defineRES_USE_DNSSEC 0x2000 /* use DNSSEC using OK bit in
> > OPT */
> > #defineRES_USE_CD 0x1000 /* set Checking Disabled flag */
> > +#defineRES_TRUSTAD 0x8000 /* Request AD, keep it in
> > responses. */
> >
> > #define RES_DEFAULT(RES_RECURSE | RES_DEFNAMES | RES_DNSRCH)
> >
> > diff --git lib/libc/asr/asr.c lib/libc/asr/asr.c
> > index 8bcb61b6000..77bc3854420 100644
> > --- lib/libc/asr/asr.c
> > +++ lib/libc/asr/asr.c
> > @@ -661,7 +661,8 @@ pass0(char **tok, int n, struct asr_ctx *ac)
> > d = strtonum(tok[i] + 6, 1, 16, );
> > if (e == NULL)
> > ac->ac_ndots = d;
> > - }
> > + } else if (!strcmp(tok[i], "trust-ad"))
> > + ac->ac_options |= RES_TRUSTAD;
> > }
> > }
> > }
> > @@ -672,7 +673,10 @@ pass0(char **tok, int n, struct asr_ctx *ac)
> > static int
> > asr_ctx_from_string(struct asr_ctx *ac, const char *str)
> > {
> > - char buf[512], *ch;
> > + struct sockaddr_in6 *sin6;
> > + struct sockaddr_in *sin;
> > + int i, trustad;
> > + char buf[512], *ch;
> >
> > asr_ctx_parse(ac, str);
> >
> > @@ -702,6 +706,27 @@ asr_ctx_from_string(struct asr_ctx *ac, const char
> > *str)
> > break;
> > }
> >
> > + trustad = 1;
> > + for (i = 0; i < ac->ac_nscount && trustad; i++) {
> > + switch (ac->ac_ns[i]->sa_family) {
> > + case AF_INET:
> > + sin = (struct sockaddr_in *)ac->ac_ns[i];
> > + if (sin->sin_addr.s_addr != htonl(INADDR_LOOPBACK))
> > + trustad = 0;
> > + break;
> > + case AF_INET6:
> > + sin6 = (struct sockaddr_in6 *)ac->ac_ns[i];
> > + if (!IN6_IS_ADDR_LOOPBACK(>sin6_addr))
> > + trustad = 0;
> > + break;
> > + default:
> > + trustad = 0;
> > + break;
> > + }
> > + }
> > + if (trustad)
> > + ac->ac_options |= RES_TRUSTAD;
> > +
> > return (0);
> > }
> >
> > diff --git lib/libc/asr/getrrsetbyname_async.c
> > lib
Re: asr(3): strip AD flag in responses
On Sat, Nov 20, 2021 at 12:20:32PM +0100, Florian Obser wrote:
> The Authentic Data (AD) flag indicates that the nameserver validated
> the response using DNSSEC. For clients to trust this the nameserver
> and the path to the nameserver must be trusted. In the general case
> this is not true.
>
> We can trust localhost so we set the AD flag on queries to request
> validation and preserve the AD flag in answers. (*)
>
> If, and only if, trusted nameservers (that are not on localhost) have
> been added to resolv.conf and the path to them is secure the trust-ad
> flag may be used to request validation from them and trust answers with
> the AD flag set.
>
> The trust-ad option first appeared in glibc 2.31.
> ( https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/461 and
> https://man7.org/linux/man-pages/man5/resolv.conf.5.html )
>
> Thomas Habets (thomas at habets.se) pointed out on bugs@ that
> VerifyHostKeyDNS in ssh only works with unwind (which is good) but
> only by accident (which is bad).
> https://marc.info/?t=16371749593=1=2
>
> *) This is for people running unwind, unbound or some other validating
> resolver on localhost. Yes, it is possible that someone set up some sort
> of forwarder where they trust the DNS answers but not that they are
> DNSSEC validated. This feels contrived and a case of DON'T DO THAT!
>
> OK?
I like this much better than the sketch I posted on bugs@
Two comment wrt the docs inline.
Code looks and tests good.
-Otto
>
> diff --git include/resolv.h include/resolv.h
> index fb02483871e..2422deb5484 100644
> --- include/resolv.h
> +++ include/resolv.h
> @@ -191,6 +191,7 @@ struct __res_state_ext {
> /* DNSSEC extensions: use higher bit to avoid conflict with ISC use */
> #define RES_USE_DNSSEC 0x2000 /* use DNSSEC using OK bit in
> OPT */
> #define RES_USE_CD 0x1000 /* set Checking Disabled flag */
> +#define RES_TRUSTAD 0x8000 /* Request AD, keep it in
> responses. */
>
> #define RES_DEFAULT (RES_RECURSE | RES_DEFNAMES | RES_DNSRCH)
>
> diff --git lib/libc/asr/asr.c lib/libc/asr/asr.c
> index 8bcb61b6000..77bc3854420 100644
> --- lib/libc/asr/asr.c
> +++ lib/libc/asr/asr.c
> @@ -661,7 +661,8 @@ pass0(char **tok, int n, struct asr_ctx *ac)
> d = strtonum(tok[i] + 6, 1, 16, );
> if (e == NULL)
> ac->ac_ndots = d;
> - }
> + } else if (!strcmp(tok[i], "trust-ad"))
> + ac->ac_options |= RES_TRUSTAD;
> }
> }
> }
> @@ -672,7 +673,10 @@ pass0(char **tok, int n, struct asr_ctx *ac)
> static int
> asr_ctx_from_string(struct asr_ctx *ac, const char *str)
> {
> - char buf[512], *ch;
> + struct sockaddr_in6 *sin6;
> + struct sockaddr_in *sin;
> + int i, trustad;
> + char buf[512], *ch;
>
> asr_ctx_parse(ac, str);
>
> @@ -702,6 +706,27 @@ asr_ctx_from_string(struct asr_ctx *ac, const char *str)
> break;
> }
>
> + trustad = 1;
> + for (i = 0; i < ac->ac_nscount && trustad; i++) {
> + switch (ac->ac_ns[i]->sa_family) {
> + case AF_INET:
> + sin = (struct sockaddr_in *)ac->ac_ns[i];
> + if (sin->sin_addr.s_addr != htonl(INADDR_LOOPBACK))
> + trustad = 0;
> + break;
> + case AF_INET6:
> + sin6 = (struct sockaddr_in6 *)ac->ac_ns[i];
> + if (!IN6_IS_ADDR_LOOPBACK(>sin6_addr))
> + trustad = 0;
> + break;
> + default:
> + trustad = 0;
> + break;
> + }
> + }
> + if (trustad)
> + ac->ac_options |= RES_TRUSTAD;
> +
> return (0);
> }
>
> diff --git lib/libc/asr/getrrsetbyname_async.c
> lib/libc/asr/getrrsetbyname_async.c
> index e5e7c23c261..06a998b0381 100644
> --- lib/libc/asr/getrrsetbyname_async.c
> +++ lib/libc/asr/getrrsetbyname_async.c
> @@ -32,7 +32,7 @@
> #include "asr_private.h"
>
> static int getrrsetbyname_async_run(struct asr_query *, struct asr_result *);
> -static void get_response(struct asr_result *, const char *, int);
> +static void get_response(struct asr_result *, const char *, int, int);
>
> struct asr_query *
> getrrsetbyname_async(const char *hostname, unsigned int rdclass,
> @@ -150,7 +150,8 @@ getrrsetbyname_async_run(struct asr_query *as, struct
> asr_result *ar)
> break;
> }
>
> - get_response(ar, ar->ar_data, ar->ar_datalen);
> + get_response(ar, ar->ar_data, ar->ar_datalen,
> + as->as_ctx->ac_options & RES_TRUSTAD);
> free(ar->ar_data);
>
Re: [PATCH] Change maximum size of /usr/src to 3G for autoinstall
On Sun, Nov 07, 2021 at 07:44:57PM +0300, Mikhail wrote: > On Sat, Oct 30, 2021 at 11:39:54AM +0300, Mikhail wrote: > > On Sun, Oct 24, 2021 at 02:17:25PM +0300, Mikhail wrote: > > > On Sun, Oct 24, 2021 at 11:32:26AM +0100, Stuart Henderson wrote: > > > > The minimum needs to go up too, a cvs checkout is 1.3G already. > > > > > > > > (Not that I use auto defaults without changes anyway, they don't > > > > work too well for ports dev..) > > > > > > Changed minimum to 1.5G. > > > > Weekly friendly ping. Comments, objections, feedback? > > > > Maybe someone has another opinion on max (3G) and min (1.5G) values? > > > > I think bumping them makes sense, since more and more users use git. > > Last ping, maybe interested committer appeared on this week. > I'll take a look. Remind me if I forget. Sorry for the delay. -Otto
Re: Missing semicolon in snmpd/parse.y
On Wed, Oct 20, 2021 at 01:58:03PM +0200, Gerhard Roth wrote:
> Hi,
>
> the rule for 'listen_udptcp' is missing a semicolon at its end.
>
> I have no idea what yacc does to the following 'port' rule without
> that semicolon.
Looks like the generated c code is the same;
ok otto@
-Otto
>
> Gerhard
>
>
> Index: usr.sbin/snmpd/parse.y
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/parse.y,v
> retrieving revision 1.70
> diff -u -p -u -p -r1.70 parse.y
> --- usr.sbin/snmpd/parse.y15 Oct 2021 15:01:29 - 1.70
> +++ usr.sbin/snmpd/parse.y20 Oct 2021 11:45:29 -
> @@ -350,6 +350,7 @@ listen_udptcp : listenproto STRING port
> free($2);
> free($3);
> }
> + ;
>
> port : /* empty */ {
> $$ = NULL;
Re: Unwind + NSD usage question
On Mon, Sep 27, 2021 at 08:50:06PM -0400, abyx...@mnetic.ch wrote:
> Hello, trying to set up unwind with nsd on the same machine serving a
> internal domain (home.arpa) with all my machines being part of that domain,
> eg router.home.arpa. If I point dig at my nsd instance (dig @127.0.0.1 -p
> 10053 router.home.arpa. A) I see my subdomains in the zone all being returned
> (router.home.arpa. -> 10.0.0.1). If I set nsd as a forwarder in unwind.conf
> (forwarder 127.0.0.1 port 10053) though, things get weird. My ISP doesn't
> return any results for home.arpa but some other servers (quad9 and
> cloudfare?) return a blackhole address pointing to prisoner.iana.org. If I
> limit unwind to preference {forwarder recursor} I now get my local nsd
> results for my domains as expected. If I comment out the preference line,
> unwind eventually learns a server that will answer to home.arpa with the
> blackhole prisoner.iana.org address (at least a minute in, sometimes longer,
> makes testing difficult). The use of force forwarder {home.arpa} and force
> accept bogus forwarder {home.arpa} don't appear to have any effect at all.
> (Full configs and dmesg below).
>
> I dug through the code a bit, if I'm following it correctly in
> sbin/unwind/resolver.c:check_resolver_done, nsd seems to be returning
> a SERVFAIL and being marked dead (as confirmed with unwindctl status.
> I am not sure I followed the code correctly at this point, but being
> set to DEAD and/or returning a SERVFAIL seems to preempt the use of
> force accept bogus. I am not sure what test unwind/libunbound are
> doing to check the health status of the different resolvers but I have
> yet to see my nsd forwarder not marked as "dead" in unwindctl status.
> Any ideas on how to debug this? This happens on both 6.9 and -current.
> The -current dmesg is posted below.
(Pleae wrap your lines).
Your issue might be that an NSD instance does not work as forwarding
target, since it is not an recursive resolver. unwind expects
forwarders to be able to resolve the whole DNS tree, even if they are
marked to be used for a subtree only.
I have a similar setup, but I am forwarding to a recursive resolver
that is authoritative for my local private domain. Any resolver I know
has that capability, e.g. with unbound you would use local.zone.
-Otto
>
>
>
> ---
> router# cat /etc/unwind.conf
>
> forwarder {
> 127.0.0.1 port 10053
> }
>
> force accept bogus forwarder { home.arpa }
> #force autoconf { home.arpa }
> preference { forwarder recursor }
> #preference { recursor DoT forwarder }
> ---
>
>
> ---
> router# cat /var/nsd/etc/nsd.conf
>
> # $OpenBSD: nsd.conf,v 1.13 2018/08/16 17:59:12 florian Exp $
>
> server:
> hide-version: yes
> verbosity: 1
> database: "" # disable database
>
> ## bind to a specific address/port
> ip-address: 127.0.0.1@10053
>
> ## make packets as small as possible, on by default
> # minimal-responses: yes
>
> ## respond with truncation for ANY queries over UDP and allow ANY over TCP,
> ## on by default
> # refuse-any: yes
>
> remote-control:
> control-enable: yes
> control-interface: /var/run/nsd.sock
>
> zone:
> name: "home.arpa."
> zonefile: "master/home.arpa"
> ---
>
>
> ---
> router# unwindctl status
>
> 1. recursorvalidating, 30ms 2. forwarder dead, 15ms
>
> histograms: lifetime[ms], decaying[ms]
> <10 <20 <40 <60 <80 <100 <200 <400 <600 <800 <1000 >
> rec 1634 1008 1014 619 292 339 973 667 15626 7 1
> 1614 8 6 1 3 6 5 0 0 0 0
> forw 223886 0 0 0 0 0 0 0 0 0 0
> 19 0 0 0 0 0 0 0 0 0 0 0
> ---
>
>
> ---
> router# dig @127.0.0.1 home.arpa. A
>
> ; <<>> dig 9.10.8-P1 <<>> @127.0.0.1 home.arpa. A
> ; (1 server found)
> ;; global options: +cmd
> ;; Got answer:
> ;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 41102
> ;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 0
>
> ;; QUESTION SECTION:
> ;home.arpa. IN A
>
> ;; ANSWER SECTION:
> home.arpa. 413 IN A 10.0.0.1
>
> ;; Query time: 62 msec
> ;; SERVER: 127.0.0.1#53(127.0.0.1)
> ;; WHEN: Mon Sep 27 20:46:38 EDT 2021
> ;; MSG SIZE rcvd: 43
> ---
>
>
> ---
> router# dig @9.9.9.9 home.arpa. A
>
> ; <<>> dig 9.10.8-P1 <<>> @9.9.9.9 home.arpa. A
> ; (1 server found)
> ;; global options: +cmd
> ;; Got answer:
> ;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 53702
> ;; flags: qr rd ra; QUERY: 1, ANSWER: 0, AUTHORITY: 1, ADDITIONAL: 1
>
> ;; OPT PSEUDOSECTION:
> ; EDNS: version: 0, flags:;
Re: libedit: stop ignoring SIGINT
On Mon, Aug 09, 2021 at 07:20:31AM -0600, Theo de Raadt wrote: > Ingo Schwarze wrote: > > > as mentioned earlier, deraadt@ reported that sftp(1) ignores Ctrl-C. > > Fixing that without longjmp(3) requires making editline(3) better > > behaved. > > If a library interface encourages use longjmp(), that library should be > thrown into the dustbin of history. Our src tree has very few longjmp > these days. Thank you for the effort to discourage addition of more. > > > The following patch causes el_gets(3) and el_wgets(3) to return > > failure when read(2)ing from the terminal is interrupted by a > > signal other than SIGCONT or SIGWINCH. That allows the calling > > program to decide what to do, usually either exit the program or > > provide a fresh prompt to the user. > > Looks good. > > > * bc(1) > >It behaves well with the patch: Ctrl-C discards the current > >input line and starts a new input line. > >The reason why this already works even without the patch > >is that bc(1) does very scary stuff inside the signal handler: > >pass a file-global EditLine pointer on the heap to el_line(3) > >and access fields inside the returned struct. Needless to > >say that no signal handler should do such things... > > Otto -- if you are short of time, at minimum mark the variable usage > line with /* XXX signal race */ as we have done throughout the tree. I > would encourage anyone who sees such problems inside any signal handler > to show such comment-adding diffs. If these problems are documented, > they can be fixed eventually, usually through event-loop logic, but I'll > admit many of the comments are in non-event-loop programs. Added the comment. Don't know what I was thinking when I did that change. -Otto > > > * ftp(1) > >It behaves well with the patch: Ctrl-C discards the current > >input line and provides a fresh prompt. > >The reason why it already works without the patch is that ftp(1) > >uses setjmp(3)/longjmp(3) to forcefully grab back control > >from el_gets(3) without waiting for it to return. > > Horrible isn't it. > > > * sftp(1) > >Behaviour is improved with the patch: Ctrl-C now exits sftp(1). > >If desired, i can supply a very simple follow-up patch to sftp.c > >to instead behave like ftp(1) and bc(1), i.e. discard the > >current input line and provide a fresh prompt. > >Neither doing undue work in the signal handler nor longjmp(3) > >will be required for that (if this patch gets committed). > > I suspect dtucker will want to decide on the interactive behaviour, > but what you describe sounds right. > > > Also note that deraadt@ pointed out in private mail to me that the > > fact that read__fixio() clears FIONBIO is probably a symptom of > > botched editline(3) API design. That might be worth fixing, too, > > as far as that is feasible, but it is unrelated to the sftp(1) > > Ctrl-C issue; let's address one topic at a time. > > I mentioned rarely having seen a good outcome from code mixing any of > the 3: FIONBIO, FIONREAD, and select/poll. Often the non-blocking was > added to select/poll code to hide some sort of bug, or the select/poll > code was added amateurishly to older code without removing the FIONBIO. > There are a few situations you need both approaches mixed, but it isn't > the general case, and thus FIONBIO has a "polled IO" smell for me. >
Re: [CAN IGNORE] Proposal for new bc(1) and dc(1)
On Fri, Jul 30, 2021 at 09:54:27AM -0600, Gavin Howard wrote: > Whoops; I thought Theo would make the decision, and his last email made > me think he might have. > > I am happy to help as much as I can to make the process easy for you. > > In the meantime, I think I will release 5.0.0 when it's ready. I'll take > into account your feedback in a future release. > > Gavin Howard > It's mostly a question of finding time. As for the decision to import itself, Theo listed important drawbacks of importing something from upstream into our base. The advantages of doing that must be very big to outweight the disadvantages. Given that current dc and bc are quite ok (if I may so myself), the chances of your code getting into base are quite slim indeed. -Otto
Re: [CAN IGNORE] Proposal for new bc(1) and dc(1)
On Thu, Jul 29, 2021 at 10:31:34PM -0600, Gavin Howard wrote: > Hello, > > At this point, because of the lack of reply, I am going to assume that > my proposal is rejected. While I am sad, I understand. > > Thank you for taking the time to consider my proposal. > > Gavin Howard > I just did not find the time to look at it. Sorry for that. I still might one day. -Otto
Re: [CAN IGNORE] Proposal for new bc(1) and dc(1)
On Thu, Jun 17, 2021 at 10:01:02AM -0600, Gavin Howard wrote: > Otto, > > > I think it is interesting. As for the incompatibilites, my memory says > > I followed the original dc and bc when deciding on those (GNU chose to > > differs in these cases). Bit it has been 18 years since I wrote the > > current versions, so I might misrememeber. > > I think that makes sense to me. Unfortunately, when I was building my > dc, I couldn't find any mention in the OpenBSD man pages, which I used > to ensure as much compatibility as I could, that arrays and registers > were not separate. Well, there was one (the `;` command mentions > registers, but the `:` command does not, so I thought that was a typo). > > Regarding the 0 having length 0 or 1, that was a decision I agonized > over. My dad, who is a mathematician, said that it could go either way. > Unfortunately for me, if this is a showstopper incompatibility (and it > might be based on how the test suite uses `length()` and `Z`), I do > think I would keep it as it is and accept that OpenBSD will not want my > bc(1) and dc(1). It looks like GNU dc and bc do not agree: $ dc -V dc (GNU bc 1.06) 1.3 Copyright 1994, 1997, 1998, 2000 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE, to the extent permitted by law. iMac:~ otto$ dc 0Zp 0 and $ bc bc 1.06 Copyright 1991-1994, 1997, 1998, 2000 Free Software Foundation, Inc. This is free software with ABSOLUTELY NO WARRANTY. For details type `warranty'. length(0) 1 I confirmed the original dc by Morris and Cherry indeed print 0 for the above test case. -Otto > > > As for moving to your version, I have no opinion yet. I have some > > attachment to the current code, but not so strong that I am opposing > > replacement upfront. OTOH the current implementaion is almost > > maintainance free for many years already. So I dunno. > > You have a right to have attachment to it; I have attachment to mine! > > In fact, I was pleasantly surprised at how clean and readable your code > was. I usually struggle to read code written by others, but I could > easily read yours. > > On that note, since last night, I thought of more disadvantages of > moving to my bc and dc, which I feel I must mention. > > More disadvantages: > > * The current dc(1) and bc(1) are from a known member of the OpenBSD > community with many contributions. I am an unknown quantity. > * The current dc(1) and bc(1) do not have ugly portability code that > OpenBSD probably doesn't care about. > * The current dc(1) and bc(1) do not have ugly code to support build > options that OpenBSD does not care about. > * The binary size of the OpenBSD dc(1) and bc(1) combined are 78% the > size of mine combined (on amd64). The size of OpenBSD combined is > 145440, and the size of mine combined are 185706. > * The current dc(1) and bc(1) have much less source code and have been > nearly maintenance-free for many years. Mine were started in 2018 and > do not have as long of a track record for being low maintenance. > > > I'll take a look at your code soon and maybe other devs have opinions. > > Thank you very much! > > Gavin Howard >
