Paths

Table of Contentst

-
sys/
-
security/mac/
-
mac/
-
mac_process.c
-
vm/
-
vm_map.h
-
vm_map.c

Differential D20814

Make entry simplification one-way
ClosedPublic
Actions

Authored by dougm on Jul 1 2019, 4:10 AM.

Details

Reviewers

markj
kib
alc

Commits

rS351476: vm_map_simplify_entry considers merging an entry with its two

Summary

vm_map_simplify_entry considers merging an entry with its two neighbors, and is used in a way so that if entries a and b cannot be merged, we consider them twice, first not-merging a with its successor b, and then not-merging b with its predecessor a. This change would mostly replace vm_map_simplify_entry with a new function that compares two adjacent entries only, and uses it to avoid duplicated merge-checks.

Change loops that use vm_map_simplify_entry to use vm_map_try_merge_entries instead.
Change other vm_map_simplify_entry calls to use a pair of vm_map_merge_entry calls.

Test Plan

Compiles, boots, passes simple tests. Stress testing by Peter Holm was performed on a much different patch, and those tests need to be repeated.

A performance test

mprotector.c752 BDownload

run 10 times with and without this change produces these results.

Without:
time 0.31
time 0.26
time 0.26
time 0.26
time 0.27
time 0.30
time 0.28
time 0.30
time 0.28
time 0.28

With:
time 0.27
time 0.27
time 0.26
time 0.25
time 0.26
time 0.26
time 0.26
time 0.26
time 0.27
time 0.26

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

dougm created this revision.Jul 1 2019, 4:10 AM

20190701 07:18:49 all (2/70): mmap10.sh
panic: _vm_map_clip_start: invalid clip of entry 0xfffff801496d34d0
cpuid = 9
time = 1561958339
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe00ad7307a0
vpanic() at vpanic+0x19d/frame 0xfffffe00ad7307f0
panic() at panic+0x43/frame 0xfffffe00ad730850
_vm_map_clip_start() at _vm_map_clip_start+0x81/frame 0xfffffe00ad730890
vm_map_unwire() at vm_map_unwire+0x350/frame 0xfffffe00ad730960
sys_munlockall() at sys_munlockall+0x71/frame 0xfffffe00ad730990
amd64_syscall() at amd64_syscall+0x291/frame 0xfffffe00ad730ab0
fast_syscall_common() at fast_syscall_common+0x101/frame 0xfffffe00ad730ab0

https://people.freebsd.org/~pho/stress/log/dougm043.txt

PS
Just a *very* minor point: When you generate diffs, would it be possible to always generate it from the top of the SVN or git tree?

dougm removed a reviewer: alc.Jul 6 2019, 6:30 PM

Drop all the changes related to wiring and unwiring.

Code formatting and comment changes.

I ran a brief 4 hour test with this patch.

dougm edited the summary of this revision. (Show Details)Jul 21 2019, 4:32 PM

dougm edited the test plan for this revision. (Show Details)

dougm added reviewers: markj, kib.

markj added inline comments.Jul 23 2019, 2:47 PM

sys/vm/vm_map.c
2570–2571	Don't we want to attempt simplification after updating `current->protection`?

markj added inline comments.Jul 23 2019, 3:19 PM

sys/vm/vm_map.c
2570–2571	Sorry, never mind.

markj added inline comments.Jul 23 2019, 3:26 PM

sys/vm/vm_map.c
2088	I'd suggest a name like vm_map_try_merge_entries() instead.

dougm added inline comments.Jul 23 2019, 3:26 PM

sys/vm/vm_map.c
2570–2571	We do. We are now. After this change, we still would be. Is this a suggestion about correctness or style - that, do you just want to move the simplification to line before "For user wired map entries...." to make clearer why we're simplifying, or are you suggesting that something gets broken here?

Rename vm_map_merge_entries to vm_map_try_merge_entries.

markj accepted this revision.Jul 24 2019, 3:30 PM

This revision is now accepted and ready to land.Jul 24 2019, 3:30 PM

dougm added a subscriber: alc.Jul 24 2019, 3:40 PM

I would suggest writing a microbenchmark to demonstrate that this change can reduce the execution time for one of the affected functions. For example, write a program that mmap(MAP_ANON)'s a gigabyte of PROT_WRITE virtual address space, iterates over that space removing PROT_WRITE on every other page, and then measure the time to perform a single mprotect() restoring PROT_WRITE to the entire gigabyte of virtual address space.

sys/vm/vm_map.c
2464–2469	What happens here when "start == 0"?

In looking for the map entry that includes the page before page 0, use &map->header explicitly instead of using vm_map_lookup_entry.

This revision now requires review to proceed.Jul 26 2019, 4:23 AM

dougm edited the test plan for this revision. (Show Details)Jul 26 2019, 4:28 AM

dougm added inline comments.Jul 26 2019, 4:33 AM

sys/vm/vm_map.c
2464–2469	We get prev==&map->header. Before the most recent change, something bad would have happened.

dougm edited the test plan for this revision. (Show Details)Jul 26 2019, 4:38 AM

Use vm_map_try_merge_entries instead of vm_simplify_entry in vm_map_inherit.

I tested D20814.60178.diff for 13 hours on amd64 without seeing any problems.

dougm added a reviewer: alc.Jul 27 2019, 3:57 PM

dougm removed a subscriber: alc.

dougm edited the test plan for this revision. (Show Details)Jul 28 2019, 6:11 PM

Get rid of the remaining uses of vm_map_simplify_entry.

Request retest from Peter Holm.

dougm edited the summary of this revision. (Show Details)Jul 31 2019, 5:17 AM

D20814.60315.diff

20190731 16:18:08 all (17/73): mmap10.sh
panic: Lock vm map (user) not exclusively locked @ ../../../vm/vm_map.c:3102
cpuid = 12
time = 1564582693
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe00c7c80800
vpanic() at vpanic+0x19d/frame 0xfffffe00c7c80850
panic() at panic+0x43/frame 0xfffffe00c7c808b0
_sx_xunlock() at _sx_xunlock+0xab/frame 0xfffffe00c7c808d0
_vm_map_unlock() at _vm_map_unlock+0x75/frame 0xfffffe00c7c80900
vm_map_wire() at vm_map_wire+0x58/frame 0xfffffe00c7c80930
kern_mlock() at kern_mlock+0x179/frame 0xfffffe00c7c80980
amd64_syscall() at amd64_syscall+0x2d6/frame 0xfffffe00c7c80ab0

https://people.freebsd.org/~pho/stress/log/dougm054.txt

Don't unlock on early exit from map_wire_locked.

With D20814.60322.diff I got:

20190731 19:27:23 all (1/1): mmap10.sh
panic: vm_map_unwire: lookup failed
cpuid = 12
time = 1564594061
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe00c96c5790
vpanic() at vpanic+0x19d/frame 0xfffffe00c96c57e0
panic() at panic+0x43/frame 0xfffffe00c96c5840
vm_map_wire_locked() at vm_map_wire_locked+0x69d/frame 0xfffffe00c96c5900
vm_map_wire() at vm_map_wire+0x40/frame 0xfffffe00c96c5930
kern_mlock() at kern_mlock+0x179/frame 0xfffffe00c96c5980
amd64_syscall() at amd64_syscall+0x2d6/frame 0xfffffe00c96c5ab0
fast_syscall_common() at fast_syscall_common+0x101/frame 0xfffffe00c96c5ab0

https://people.freebsd.org/~pho/stress/log/dougm055.txt

Fix function naming mismatches in KASSERTS. In wire/unwire, make sure to clip after first entry found in transition.

I ran tests for 15 hours on D20814.60344.diff without seeing any problems.

Ready for review....

kib added inline comments.Aug 17 2019, 11:31 AM

sys/vm/vm_map.c
2090	Assert that the map is exclusively locked.
2468	extra ()
2989	This pattern repeats more than once. Might be it makes sense to create a helper, e.g. vm_map_lookup_entry_and_prev() ?

Apply reviewer suggestions.

My major concern with this change is that it is doing two things at once, and I would ask that these two things be done separately, so that they can be individually evaluated. As the title says, this change is making simplification one way. However, most of this change is actually about decommissioning the "prev" pointer in the map entry.

A change that simply makes simplification one way doesn't require changing the loops over the map to maintain a local "prev" pointer. Internally, the one-way simplification function could simply use the map entry's "prev" pointer rather than having to modify every caller to pass it in. Moreover, making the callers maintain a local "prev" pointer rather than fetching it in the one-way simplification function isn't necessarily an optimization. In complex callers, especially where helper functions are being called, the "prev" pointer is probably winding up on the stack rather than being held in a register.

In summary, a change to make simplification one-way can be a lot simpler than what is presented here, and the change to eliminate the "prev" pointer really needs to be "atomic", i.e., it needs to be separate and complete, to properly evaluate it.

Simplify the patch.

In D20814#463451, @dougm wrote:

Simplify the patch.

Thank you.

sys/security/mac/mac_process.c
267–269	This case didn't get simplified.
353	(This comment is unrelated to the patch.) This looks like a bug to me, albeit an arguably innocuous one. If mac_mmap_revocation_via_cow is false, we should not be making the map entry COW.
sys/vm/vm_map.c
2081–2084	The clause beginning with "and" seems wrong. Specifically, it has the "direction" of the merge backwards.

Apply reviewer directives.

This looks good. Can you please repeat the performance test with and without this patch.

A different variation on your performance test that exercises minherit() in a similar way would probably show an even greater speedup.

This revision is now accepted and ready to land.Aug 22 2019, 9:52 PM

dougm edited the test plan for this revision. (Show Details)Aug 23 2019, 3:14 AM

dougm edited the summary of this revision. (Show Details)Aug 23 2019, 3:20 AM

I should be able to free up some test hardware later today.

I ran the full stress2 test with D20814.61081.diff on amd64. LGTM.

Closed by commit rS351476: vm_map_simplify_entry considers merging an entry with its two (authored by dougm). · Explain WhyAug 25 2019, 7:07 AM

This revision was automatically updated to reflect the committed changes.

dougm added a commit: rS351476: vm_map_simplify_entry considers merging an entry with its two.

Herald added a subscriber: imp. · View Herald TranscriptAug 25 2019, 7:07 AM

Revision Contents
Changeset List

Path

Size

sys/

security/

mac/

mac_process.c

7 lines

vm/

vm_map.h

3 lines

vm_map.c

198 lines

Diff 60344

View Options

sys/security/mac/mac_process.c

This file was added.

				/*-
				* SPDX-License-Identifier: BSD-2-Clause
				*
				* Copyright (c) 2023 Dave Cottlehuber <dch@FreeBSD.org>
				*
				* Redistribution and use in source and binary forms, with or without
				* modification, are permitted provided that the following conditions
				* are met:
				* 1. Redistributions of source code must retain the above copyright
				* notice, this list of conditions and the following disclaimer.
				* 2. Redistributions in binary form must reproduce the above copyright
				* notice, this list of conditions and the following disclaimer in the
				* documentation and/or other materials provided with the distribution.
				*
				* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
				* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
				* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
				* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
				* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
				* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
				* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
				* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
				* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
				* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
				* SUCH DAMAGE.
				*
				*/

				#include <sys/cdefs.h>
				__FBSDID("$FreeBSD$");

				#include <errno.h>
				#include <sys/param.h>
				#include <stdio.h>
				#include <string.h>
				#include <fetch.h>

				#include <lua.h>
				#include "lauxlib.h"
				#include "lfetch.h"

				/*
				* Minimal implementation of libfetch
				*/

				static int
				lfetch_parse_url(lua_State *L)
				{
				const char *url = luaL_checkstring(L, 1);
				struct url *u;

				u = fetchParseURL(url);
				if (u == NULL) {
				lua_pushnil(L);
				lua_pushstring(L, "Failed to parse URL");
				return (2);
				}

				lua_newtable(L);
				lua_pushstring(L, u->scheme);
				lua_setfield(L, -2, "scheme");
				lua_pushstring(L, u->user);
				lua_setfield(L, -2, "user");
				lua_pushstring(L, u->pwd);
				lua_setfield(L, -2, "password");
				lua_pushstring(L, u->host);
				lua_setfield(L, -2, "host");
				/* if port is not explicitly set, it defaults to 0, not scheme */
				kevansUnsubmitted Done Inline Actions My gut reaction to this is that we probably shouldn't expose the port at all if it's not explicitly set, just leave it nil in this table to allow for more idiomatic usage where you can simply test the port as truthy before using it.f kevans: My gut reaction to this is that we probably shouldn't expose the port at all if it's not…
				lua_pushinteger(L, u->port);
				lua_setfield(L, -2, "port");
				/* doc is also known as the query string for http(s) */
				lua_pushstring(L, u->doc);
				lua_setfield(L, -2, "doc");
				fetchFreeURL(u);
				return (1);
				}

				static int
				lfetch_get_url(lua_State *L) {
				kevansUnsubmitted Done Inline Actions Brace on its own line kevans: Brace on its own line
				const char *url = luaL_checkstring(L, 1);
				const char *out = luaL_checkstring(L, 2);

				struct url *u;
				kevansUnsubmitted Done Inline Actions This should move up above the blank line kevans: This should move up above the blank line
				u = fetchParseURL(url);
				if (u == NULL) {
				lua_pushnil(L);
				lua_pushstring(L, "Failed to parse URL");
				return (2);
				}

				FILE *file = fopen(out, "wb");
				kevansUnsubmitted Done Inline Actions IIRC style(9) still doesn't allow declarations mid-block, it would need to be at the top of this function. kevans: IIRC style(9) still doesn't allow declarations mid-block, it would need to be at the top of…
				if (file == NULL) {
				lua_pushnil(L);
				lua_pushfstring(L, "Failed to open output file: %s", strerror(errno));
				return 2;
				}

				FILE *fetch = fetchGet(u, "");
				if (fetch == NULL) {
				lua_pushnil(L);
				lua_pushfstring(L, "Failed to read from URL: %s", strerror(errno));
				return 2;
				kevansUnsubmitted Done Inline Actions Leaks `file` kevans: Leaks `file`
				}

				int chunk_size = 4096;
				kevansUnsubmitted Done Inline Actions Ditto for these declarations; chunk_size I'd probably just make a `#define` since we're not going to be altering it kevans: Ditto for these declarations; chunk_size I'd probably just make a `#define` since we're not…
				char buf[chunk_size];
				size_t bytes;

				while ((bytes = fread(buf, 1, chunk_size, fetch)) > 0) {
				if (fwrite(buf, 1, bytes, file) != bytes) {
				lua_pushnil(L);
				lua_pushfstring(L, "Failed to write to file: %s", strerror(errno));
				fclose(file);
				kevansUnsubmitted Done Inline Actions Leaks `fetch` kevans: Leaks `fetch`
				dchAuthorUnsubmitted Done Inline Actions If I `fclose(fetch);` here, this segfaults: Lua 5.4.4 Copyright (C) 1994-2022 Lua.org, PUC-Rio f.get_url("https://invalid.site/", "/tmp/i") fish: Job 1, '/usr/libexec/flua $argv' terminated by signal SIGSEGV (Address boundary error) (lldb) thr backtrace all * thread #1, name = 'flua', stop reason = signal SIGSEGV * frame #0: 0x00001252697ad0f9 frame #1: 0xffadb0abacff9a89 dch: If I `fclose(fetch);` here, this segfaults: Lua 5.4.4 Copyright (C) 1994-2022 Lua.org, PUC…
				kevansUnsubmitted Done Inline Actions The context for this one has changed, the comment was originally down in the error path in the loop. This branch looks fine kevans: The context for this one has changed, the comment was originally down in the error path in the…
				return 2;
				}
				}

				fclose(fetch);
				fclose(file);
				fetchFreeURL(u);
				lua_pushboolean(L, 1);
				return 1;
				}

				static const struct
				luaL_Reg fetchlib[] = {
				{"parse_url", lfetch_parse_url},
				{"get_url", lfetch_get_url},
				{NULL, NULL}
				};

				int
				luaopen_fetch(lua_State *L)
				{
				luaL_newlib(L, fetchlib);
				return (1);
				}

View Options

sys/vm/vm_map.h

This file was added.

				/*-
				* SPDX-License-Identifier: BSD-2-Clause
				*
				* Copyright (c) 2023 Dave Cottlehuber <dch@FreeBSD.org>
				*
				* Redistribution and use in source and binary forms, with or without
				* modification, are permitted provided that the following conditions
				* are met:
				* 1. Redistributions of source code must retain the above copyright
				* notice, this list of conditions and the following disclaimer.
				* 2. Redistributions in binary form must reproduce the above copyright
				* notice, this list of conditions and the following disclaimer in the
				* documentation and/or other materials provided with the distribution.
				*
				* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
				* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
				* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
				* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
				* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
				* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
				* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
				* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
				* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
				* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
				* SUCH DAMAGE.
				*
				*/

				#include <sys/cdefs.h>
				__FBSDID("$FreeBSD$");

				#include <errno.h>
				#include <sys/param.h>
				#include <stdio.h>
				#include <string.h>
				#include <fetch.h>

				#include <lua.h>
				#include "lauxlib.h"
				#include "lfetch.h"

				/*
				* Minimal implementation of libfetch
				*/

				static int
				lfetch_parse_url(lua_State *L)
				{
				const char *url = luaL_checkstring(L, 1);
				struct url *u;

				u = fetchParseURL(url);
				if (u == NULL) {
				lua_pushnil(L);
				lua_pushstring(L, "Failed to parse URL");
				return (2);
				}

				lua_newtable(L);
				lua_pushstring(L, u->scheme);
				lua_setfield(L, -2, "scheme");
				lua_pushstring(L, u->user);
				lua_setfield(L, -2, "user");
				lua_pushstring(L, u->pwd);
				lua_setfield(L, -2, "password");
				lua_pushstring(L, u->host);
				lua_setfield(L, -2, "host");
				/* if port is not explicitly set, it defaults to 0, not scheme */
				kevansUnsubmitted Done Inline Actions My gut reaction to this is that we probably shouldn't expose the port at all if it's not explicitly set, just leave it nil in this table to allow for more idiomatic usage where you can simply test the port as truthy before using it.f kevans: My gut reaction to this is that we probably shouldn't expose the port at all if it's not…
				lua_pushinteger(L, u->port);
				lua_setfield(L, -2, "port");
				/* doc is also known as the query string for http(s) */
				lua_pushstring(L, u->doc);
				lua_setfield(L, -2, "doc");
				fetchFreeURL(u);
				return (1);
				}

				static int
				lfetch_get_url(lua_State *L) {
				kevansUnsubmitted Done Inline Actions Brace on its own line kevans: Brace on its own line
				const char *url = luaL_checkstring(L, 1);
				const char *out = luaL_checkstring(L, 2);

				struct url *u;
				kevansUnsubmitted Done Inline Actions This should move up above the blank line kevans: This should move up above the blank line
				u = fetchParseURL(url);
				if (u == NULL) {
				lua_pushnil(L);
				lua_pushstring(L, "Failed to parse URL");
				return (2);
				}

				FILE *file = fopen(out, "wb");
				kevansUnsubmitted Done Inline Actions IIRC style(9) still doesn't allow declarations mid-block, it would need to be at the top of this function. kevans: IIRC style(9) still doesn't allow declarations mid-block, it would need to be at the top of…
				if (file == NULL) {
				lua_pushnil(L);
				lua_pushfstring(L, "Failed to open output file: %s", strerror(errno));
				return 2;
				}

				FILE *fetch = fetchGet(u, "");
				if (fetch == NULL) {
				lua_pushnil(L);
				lua_pushfstring(L, "Failed to read from URL: %s", strerror(errno));
				return 2;
				kevansUnsubmitted Done Inline Actions Leaks `file` kevans: Leaks `file`
				}

				int chunk_size = 4096;
				kevansUnsubmitted Done Inline Actions Ditto for these declarations; chunk_size I'd probably just make a `#define` since we're not going to be altering it kevans: Ditto for these declarations; chunk_size I'd probably just make a `#define` since we're not…
				char buf[chunk_size];
				size_t bytes;

				while ((bytes = fread(buf, 1, chunk_size, fetch)) > 0) {
				if (fwrite(buf, 1, bytes, file) != bytes) {
				lua_pushnil(L);
				lua_pushfstring(L, "Failed to write to file: %s", strerror(errno));
				fclose(file);
				kevansUnsubmitted Done Inline Actions Leaks `fetch` kevans: Leaks `fetch`
				dchAuthorUnsubmitted Done Inline Actions If I `fclose(fetch);` here, this segfaults: Lua 5.4.4 Copyright (C) 1994-2022 Lua.org, PUC-Rio f.get_url("https://invalid.site/", "/tmp/i") fish: Job 1, '/usr/libexec/flua $argv' terminated by signal SIGSEGV (Address boundary error) (lldb) thr backtrace all * thread #1, name = 'flua', stop reason = signal SIGSEGV * frame #0: 0x00001252697ad0f9 frame #1: 0xffadb0abacff9a89 dch: If I `fclose(fetch);` here, this segfaults: Lua 5.4.4 Copyright (C) 1994-2022 Lua.org, PUC…
				kevansUnsubmitted Done Inline Actions The context for this one has changed, the comment was originally down in the error path in the loop. This branch looks fine kevans: The context for this one has changed, the comment was originally down in the error path in the…
				return 2;
				}
				}

				fclose(fetch);
				fclose(file);
				fetchFreeURL(u);
				lua_pushboolean(L, 1);
				return 1;
				}

				static const struct
				luaL_Reg fetchlib[] = {
				{"parse_url", lfetch_parse_url},
				{"get_url", lfetch_get_url},
				{NULL, NULL}
				};

				int
				luaopen_fetch(lua_State *L)
				{
				luaL_newlib(L, fetchlib);
				return (1);
				}

View Options

sys/vm/vm_map.c

This file was added.

				/*-
				* SPDX-License-Identifier: BSD-2-Clause
				*
				* Copyright (c) 2023 Dave Cottlehuber <dch@FreeBSD.org>
				*
				* Redistribution and use in source and binary forms, with or without
				* modification, are permitted provided that the following conditions
				* are met:
				* 1. Redistributions of source code must retain the above copyright
				* notice, this list of conditions and the following disclaimer.
				* 2. Redistributions in binary form must reproduce the above copyright
				* notice, this list of conditions and the following disclaimer in the
				* documentation and/or other materials provided with the distribution.
				*
				* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
				* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
				* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
				* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
				* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
				* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
				* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
				* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
				* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
				* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
				* SUCH DAMAGE.
				*
				*/

				#include <sys/cdefs.h>
				__FBSDID("$FreeBSD$");

				#include <errno.h>
				#include <sys/param.h>
				#include <stdio.h>
				#include <string.h>
				#include <fetch.h>

				#include <lua.h>
				#include "lauxlib.h"
				#include "lfetch.h"

				/*
				* Minimal implementation of libfetch
				*/

				static int
				lfetch_parse_url(lua_State *L)
				{
				const char *url = luaL_checkstring(L, 1);
				struct url *u;

				u = fetchParseURL(url);
				if (u == NULL) {
				lua_pushnil(L);
				lua_pushstring(L, "Failed to parse URL");
				return (2);
				}

				lua_newtable(L);
				lua_pushstring(L, u->scheme);
				lua_setfield(L, -2, "scheme");
				lua_pushstring(L, u->user);
				lua_setfield(L, -2, "user");
				lua_pushstring(L, u->pwd);
				lua_setfield(L, -2, "password");
				lua_pushstring(L, u->host);
				lua_setfield(L, -2, "host");
				/* if port is not explicitly set, it defaults to 0, not scheme */
				kevansUnsubmitted Done Inline Actions My gut reaction to this is that we probably shouldn't expose the port at all if it's not explicitly set, just leave it nil in this table to allow for more idiomatic usage where you can simply test the port as truthy before using it.f kevans: My gut reaction to this is that we probably shouldn't expose the port at all if it's not…
				lua_pushinteger(L, u->port);
				lua_setfield(L, -2, "port");
				/* doc is also known as the query string for http(s) */
				lua_pushstring(L, u->doc);
				lua_setfield(L, -2, "doc");
				fetchFreeURL(u);
				return (1);
				}

				static int
				lfetch_get_url(lua_State *L) {
				kevansUnsubmitted Done Inline Actions Brace on its own line kevans: Brace on its own line
				const char *url = luaL_checkstring(L, 1);
				const char *out = luaL_checkstring(L, 2);

				struct url *u;
				kevansUnsubmitted Done Inline Actions This should move up above the blank line kevans: This should move up above the blank line
				u = fetchParseURL(url);
				if (u == NULL) {
				lua_pushnil(L);
				lua_pushstring(L, "Failed to parse URL");
				return (2);
				}

				FILE *file = fopen(out, "wb");
				kevansUnsubmitted Done Inline Actions IIRC style(9) still doesn't allow declarations mid-block, it would need to be at the top of this function. kevans: IIRC style(9) still doesn't allow declarations mid-block, it would need to be at the top of…
				if (file == NULL) {
				lua_pushnil(L);
				lua_pushfstring(L, "Failed to open output file: %s", strerror(errno));
				return 2;
				}

				FILE *fetch = fetchGet(u, "");
				if (fetch == NULL) {
				lua_pushnil(L);
				lua_pushfstring(L, "Failed to read from URL: %s", strerror(errno));
				return 2;
				kevansUnsubmitted Done Inline Actions Leaks `file` kevans: Leaks `file`
				}

				int chunk_size = 4096;
				kevansUnsubmitted Done Inline Actions Ditto for these declarations; chunk_size I'd probably just make a `#define` since we're not going to be altering it kevans: Ditto for these declarations; chunk_size I'd probably just make a `#define` since we're not…
				char buf[chunk_size];
				size_t bytes;

				while ((bytes = fread(buf, 1, chunk_size, fetch)) > 0) {
				if (fwrite(buf, 1, bytes, file) != bytes) {
				lua_pushnil(L);
				lua_pushfstring(L, "Failed to write to file: %s", strerror(errno));
				fclose(file);
				kevansUnsubmitted Done Inline Actions Leaks `fetch` kevans: Leaks `fetch`
				dchAuthorUnsubmitted Done Inline Actions If I `fclose(fetch);` here, this segfaults: Lua 5.4.4 Copyright (C) 1994-2022 Lua.org, PUC-Rio f.get_url("https://invalid.site/", "/tmp/i") fish: Job 1, '/usr/libexec/flua $argv' terminated by signal SIGSEGV (Address boundary error) (lldb) thr backtrace all * thread #1, name = 'flua', stop reason = signal SIGSEGV * frame #0: 0x00001252697ad0f9 frame #1: 0xffadb0abacff9a89 dch: If I `fclose(fetch);` here, this segfaults: Lua 5.4.4 Copyright (C) 1994-2022 Lua.org, PUC…
				kevansUnsubmitted Done Inline Actions The context for this one has changed, the comment was originally down in the error path in the loop. This branch looks fine kevans: The context for this one has changed, the comment was originally down in the error path in the…
				return 2;
				}
				}

				fclose(fetch);
				fclose(file);
				fetchFreeURL(u);
				lua_pushboolean(L, 1);
				return 1;
				}

				static const struct
				luaL_Reg fetchlib[] = {
				{"parse_url", lfetch_parse_url},
				{"get_url", lfetch_get_url},
				{NULL, NULL}
				};

				int
				luaopen_fetch(lua_State *L)
				{
				luaL_newlib(L, fetchlib);
				return (1);
				}

Make entry simplification one-wayClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 60344

sys/security/mac/mac_process.c

sys/vm/vm_map.h

sys/vm/vm_map.c

Make entry simplification one-way
ClosedPublic
Actions

Revision Contents
Changeset List