trasz retitled this revision from Fix Linux recvmsg(2) when msg_namelen returned is 0. Previously it would fail with EINVAL, breaking some of the Python regression tests. to Fix Linux recvmsg(2) when msg_namelen returned is 0.Jun 21 2020, 8:34 PM

trasz edited the summary of this revision. (Show Details)

trasz added a reviewer: Linux Emulation.

emaste added a subscriber: emaste.Jun 21 2020, 8:54 PM

emaste added inline comments.

sys/compat/linux/linux_socket.c
1202	what's msg_namelen's type?

trasz added inline comments.Jun 21 2020, 11:57 PM

sys/compat/linux/linux_socket.c
1202	It's socklen_t.

trasz added a reviewer: kib.Jun 22 2020, 1:10 PM

kib added inline comments.Jun 22 2020, 2:35 PM

sys/compat/linux/linux_socket.c
1188	Is it fine to not check msg_namelen there ?
1202	Fix the style while there, msg_name != NULL ?

emaste added inline comments.Jun 22 2020, 4:36 PM

sys/compat/linux/linux_socket.c
1202	should it be `> 0` not `!= 0`?

Fix various issues.

Harbormaster completed remote builds in B32018: Diff 73793.Jun 27 2020, 11:49 PM

trasz marked 2 inline comments as done.Jun 27 2020, 11:50 PM

trasz added inline comments.

sys/compat/linux/linux_socket.c
1202	Not sure if the check is needed, but better safe than sorry.

kib added inline comments.Jul 1 2020, 12:14 AM

sys/compat/linux/linux_socket.c
1189	Is msg_namelen user-controlled ? Does it allow for arbitrary-sized allocations controlled by user ?
1202	Isn't msg_namelen > 0 recheck is redundant ?

trasz added inline comments.Jul 1 2020, 11:04 AM

sys/compat/linux/linux_socket.c
1189	Hah, good catch!
1202	It isn't, that's the whole point of this change: kern_recvit() updates it. I guess I should add a comment about it.

Add comment explaining the second check, and cap the buffer size.

Harbormaster completed remote builds in B32127: Diff 74058.Jul 3 2020, 8:16 PM

(Tinderboxed.)

kib added inline comments.Jul 4 2020, 2:47 PM

sys/compat/linux/linux_socket.c
1205	What should the code do if msg->msg_name != NULL but msg->msg_namelen == 0 (after the kern_recvit()) ?

trasz added inline comments.Jul 4 2020, 5:57 PM

sys/compat/linux/linux_socket.c
1205	My guess is it should copyout the new length. But right now the code does not copyout the length in any case, and like previously, I prefer the small steps approach. I'll mention it in the commit message.

trasz edited the summary of this revision. (Show Details)Jul 4 2020, 5:58 PM

kib accepted this revision.Jul 4 2020, 6:38 PM

This revision is now accepted and ready to land.Jul 4 2020, 6:38 PM

Closed by commit rS362941: Fix Linux recvmsg(2) when msg_namelen returned is 0. Previously (authored by trasz). · Explain WhyJul 5 2020, 10:57 AM

This revision was automatically updated to reflect the committed changes.

trasz added a commit: rS362941: Fix Linux recvmsg(2) when msg_namelen returned is 0. Previously.

Revision Contents
Changeset List

Path

Size

sys/

compat/

linux/

linux_socket.c

2 lines

Diff 73445

View Options

sys/compat/linux/linux_socket.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);
				}

Fix Linux recvmsg(2) when msg_namelen returned is 0ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 73445

sys/compat/linux/linux_socket.c

Fix Linux recvmsg(2) when msg_namelen returned is 0
ClosedPublic
Actions

Revision Contents
Changeset List