From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Elijah Newren <newren@gmail.com>,
Michael J Gruber <git@grubix.eu>,
git@vger.kernel.org, Calvin Wan <calvinwan@google.com>
Subject: Re: [PATCH/RFD] fix connection via git protocol
Date: Mon, 17 Apr 2023 23:39:04 -0400 [thread overview]
Message-ID: <20230418033904.GA60552@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqq1qkio4cq.fsf@gitster.g>
On Mon, Apr 17, 2023 at 09:33:57AM -0700, Junio C Hamano wrote:
> > diff --git a/connect.c b/connect.c
> > index fd3179e545..1eba71e34c 100644
> > --- a/connect.c
> > +++ b/connect.c
> > @@ -753,7 +753,7 @@ static char *host_end(char **hoststart, int removebrackets)
> > }
> >
> > #define STR_(s) # s
> > -#define STR(s) STR_(s)
> > +#define STR(s) (STR_(s) + BUILD_ASSERT_OR_ZERO(s))
>
> OOoooh. Clever. A pointer plus N indexes into an array, but if the
> offset is N then the pointer is left intact so the caller does not
> see the difference.
>
> > ... But the
> > BUILD_ASSERT doesn't seem too bad to me.
>
> Indeed.
So I started to write this up as a patch, but there's another subtle
thing going on.
The BUILD_ASSERT is actually checking two things: that the result
compiles (which is what we care about here), and that the expression it
evaluates is nonzero (which we don't).
So this would fail for example with:
#define ZERO 0
const char *x = STR(ZERO);
That is OK for our purposes here (a zero port does not make any sense).
But it feels a bit weird for a macro as generically named as STR(). At
least it's local to the one file. But maybe it should be PORT_TO_STR()
or something.
All of that makes me wonder if we wouldn't be just as happy with it as a
string in the first place. In three out of four locations that use it,
they want the string anyway (to feed to getaddrinfo). And in the final
one (git-daemon), we need to convert from the user's "--port" anyway, so
there's always some string-to-int parsing. And depending on the #ifdefs,
in most cases we turn it back into a string anyway to feed to...you
guessed it, getaddrinfo!
The exception is when NO_IPV6 is defined, in which we do want the
numeric value. But we could delay parsing until that point (and
otherwise let getaddrinfo handle, which seems more correct anyway).
Something like this (though I'd probably split it into a few patches to
reason about the motivation and implications of each):
diff --git a/cache.h b/cache.h
index 2f21704da9..2ece09a2b8 100644
--- a/cache.h
+++ b/cache.h
@@ -58,7 +58,7 @@
*
* See http://www.iana.org/assignments/port-numbers
*/
-#define DEFAULT_GIT_PORT 9418
+#define DEFAULT_GIT_PORT "9418"
/*
* Basic data structures for the directory cache
diff --git a/connect.c b/connect.c
index fd3179e545..189367604c 100644
--- a/connect.c
+++ b/connect.c
@@ -1,4 +1,5 @@
#include "git-compat-util.h"
+#include "cache.h" /* or protocol.h after Elijah's patch */
#include "config.h"
#include "environment.h"
#include "gettext.h"
@@ -752,9 +753,6 @@ static char *host_end(char **hoststart, int removebrackets)
return end;
}
-#define STR_(s) # s
-#define STR(s) STR_(s)
-
static void get_host_and_port(char **host, const char **port)
{
char *colon, *end;
@@ -798,7 +796,7 @@ static int git_tcp_connect_sock(char *host, int flags)
{
struct strbuf error_message = STRBUF_INIT;
int sockfd = -1;
- const char *port = STR(DEFAULT_GIT_PORT);
+ const char *port = DEFAULT_GIT_PORT;
struct addrinfo hints, *ai0, *ai;
int gai;
int cnt = 0;
@@ -868,7 +866,7 @@ static int git_tcp_connect_sock(char *host, int flags)
{
struct strbuf error_message = STRBUF_INIT;
int sockfd = -1;
- const char *port = STR(DEFAULT_GIT_PORT);
+ const char *port = DEFAULT_GIT_PORT;
char *ep;
struct hostent *he;
struct sockaddr_in sa;
@@ -1020,7 +1018,7 @@ static int git_use_proxy(const char *host)
static struct child_process *git_proxy_connect(int fd[2], char *host)
{
- const char *port = STR(DEFAULT_GIT_PORT);
+ const char *port = DEFAULT_GIT_PORT;
struct child_process *proxy;
get_host_and_port(&host, &port);
diff --git a/daemon.c b/daemon.c
index db8a31a6ea..ce692bad35 100644
--- a/daemon.c
+++ b/daemon.c
@@ -984,22 +984,20 @@ static const char *ip2str(int family, struct sockaddr *sin, socklen_t len)
#ifndef NO_IPV6
-static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist)
+static int setup_named_sock(char *listen_addr, const char *listen_port, struct socketlist *socklist)
{
int socknum = 0;
- char pbuf[NI_MAXSERV];
struct addrinfo hints, *ai0, *ai;
int gai;
long flags;
- xsnprintf(pbuf, sizeof(pbuf), "%d", listen_port);
memset(&hints, 0, sizeof(hints));
hints.ai_family = AF_UNSPEC;
hints.ai_socktype = SOCK_STREAM;
hints.ai_protocol = IPPROTO_TCP;
hints.ai_flags = AI_PASSIVE;
- gai = getaddrinfo(listen_addr, pbuf, &hints, &ai0);
+ gai = getaddrinfo(listen_addr, listen_port, &hints, &ai0);
if (gai) {
logerror("getaddrinfo() for %s failed: %s", listen_addr, gai_strerror(gai));
return 0;
@@ -1065,15 +1063,27 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
#else /* NO_IPV6 */
-static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist)
+static int parse_port(const char *s)
+{
+ unsigned long ret;
+ char *end;
+
+ ret = strtoul(s, &end, 0);
+ if (!ret || ret > 65535 || *end)
+ die(_("invalid listen port: %s"), s);
+
+ return ret;
+}
+
+static int setup_named_sock(char *listen_addr, const char *listen_port, struct socketlist *socklist)
{
struct sockaddr_in sin;
int sockfd;
long flags;
memset(&sin, 0, sizeof sin);
sin.sin_family = AF_INET;
- sin.sin_port = htons(listen_port);
+ sin.sin_port = parse_port(listen_port);
if (listen_addr) {
/* Well, host better be an IP address here. */
@@ -1122,7 +1132,7 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
#endif
-static void socksetup(struct string_list *listen_addr, int listen_port, struct socketlist *socklist)
+static void socksetup(struct string_list *listen_addr, const char *listen_port, struct socketlist *socklist)
{
if (!listen_addr->nr)
setup_named_sock(NULL, listen_port, socklist);
@@ -1133,7 +1143,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
listen_port, socklist);
if (socknum == 0)
- logerror("unable to allocate any listen sockets for host %s on port %u",
+ logerror("unable to allocate any listen sockets for host %s on port %s",
listen_addr->items[i].string, listen_port);
}
}
@@ -1246,14 +1256,14 @@ static struct credentials *prepare_credentials(const char *user_name,
}
#endif
-static int serve(struct string_list *listen_addr, int listen_port,
+static int serve(struct string_list *listen_addr, const char *listen_port,
struct credentials *cred)
{
struct socketlist socklist = { NULL, 0, 0 };
socksetup(listen_addr, listen_port, &socklist);
if (socklist.nr == 0)
- die("unable to allocate any listen sockets on port %u",
+ die("unable to allocate any listen sockets on port %s",
listen_port);
drop_privileges(cred);
@@ -1265,7 +1275,7 @@ static int serve(struct string_list *listen_addr, int listen_port,
int cmd_main(int argc, const char **argv)
{
- int listen_port = 0;
+ const char *listen_port = NULL;
struct string_list listen_addr = STRING_LIST_INIT_NODUP;
int serve_mode = 0, inetd_mode = 0;
const char *pid_file = NULL, *user_name = NULL, *group_name = NULL;
@@ -1282,13 +1292,8 @@ int cmd_main(int argc, const char **argv)
continue;
}
if (skip_prefix(arg, "--port=", &v)) {
- char *end;
- unsigned long n;
- n = strtoul(v, &end, 0);
- if (*v && !*end) {
- listen_port = n;
- continue;
- }
+ listen_port = v;
+ continue;
}
if (!strcmp(arg, "--serve")) {
serve_mode = 1;
@@ -1439,7 +1444,7 @@ int cmd_main(int argc, const char **argv)
if (inetd_mode && (listen_port || (listen_addr.nr > 0)))
die("--listen= and --port= are incompatible with --inetd");
- else if (listen_port == 0)
+ else if (!listen_port)
listen_port = DEFAULT_GIT_PORT;
if (group_name && !user_name)
next prev parent reply other threads:[~2023-04-18 3:39 UTC|newest]
Thread overview: 101+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-01 15:10 [PATCH 00/24] Header cleanups (splitting up cache.h) Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 01/24] treewide: be explicit about dependence on trace.h & trace2.h Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 02/24] treewide: be explicit about dependence on advice.h Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 03/24] treewide: be explicit about dependence on convert.h Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 04/24] treewide: be explicit about dependence on pack-revindex.h Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 05/24] treewide: be explicit about dependence on oid-array.h Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 06/24] treewide: be explicit about dependence on mem-pool.h Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 07/24] treewide: remove unnecessary cache.h inclusion Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 08/24] object-name.h: move declarations for object-name.c functions from cache.h Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 09/24] treewide: remove cache.h inclusion due to object-name.h changes Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 10/24] git-zlib: move declarations for git-zlib functions from cache.h Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 11/24] treewide: remove cache.h inclusion due to git-zlib changes Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 12/24] object-file.h: move declarations for object-file.c functions from cache.h Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 13/24] treewide: remove cache.h inclusion due to object-file.h changes Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 14/24] object.h: move an inline function and some defines from cache.h Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 15/24] editor: move editor-related functions and declarations into common file Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 16/24] treewide: remove cache.h inclusion due to editor.h changes Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 17/24] pager.h: move declarations for pager.c functions from cache.h Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 18/24] treewide: remove cache.h inclusion due to pager.h changes Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 19/24] cache.h: remove unnecessary includes Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 20/24] strbuf: move forward declarations to beginning of file Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 21/24] treewide: remove double forward declaration of read_in_full Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 22/24] treewide: reduce includes of cache.h in other headers Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 23/24] chdir-notify, quote: replace cache.h include with path.h Elijah Newren via GitGitGadget
2023-04-01 15:10 ` [PATCH 24/24] mailmap, quote: move declarations of global vars to correct unit Elijah Newren via GitGitGadget
2023-04-03 16:23 ` [PATCH 00/24] Header cleanups (splitting up cache.h) Elijah Newren
2023-04-04 1:22 ` [PATCH v2 " Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 01/24] treewide: be explicit about dependence on trace.h & trace2.h Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 02/24] treewide: be explicit about dependence on advice.h Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 03/24] treewide: be explicit about dependence on convert.h Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 04/24] treewide: be explicit about dependence on pack-revindex.h Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 05/24] treewide: be explicit about dependence on oid-array.h Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 06/24] treewide: be explicit about dependence on mem-pool.h Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 07/24] treewide: remove unnecessary cache.h inclusion Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 08/24] object-name.h: move declarations for object-name.c functions from cache.h Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 09/24] treewide: remove cache.h inclusion due to object-name.h changes Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 10/24] git-zlib: move declarations for git-zlib functions from cache.h Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 11/24] treewide: remove cache.h inclusion due to git-zlib changes Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 12/24] object-file.h: move declarations for object-file.c functions from cache.h Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 13/24] treewide: remove cache.h inclusion due to object-file.h changes Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 14/24] object.h: move some inline functions and defines from cache.h Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 15/24] treewide: remove cache.h inclusion due to object.h changes Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 16/24] editor: move editor-related functions and declarations into common file Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 17/24] treewide: remove cache.h inclusion due to editor.h changes Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 18/24] pager.h: move declarations for pager.c functions from cache.h Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 19/24] treewide: remove cache.h inclusion due to pager.h changes Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 20/24] cache.h: remove unnecessary includes Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 21/24] strbuf: move forward declarations to beginning of file Elijah Newren via GitGitGadget
2023-04-05 17:28 ` Calvin Wan
2023-04-07 6:07 ` Elijah Newren
2023-04-10 15:52 ` Calvin Wan
2023-04-10 16:47 ` Elijah Newren
2023-04-04 1:22 ` [PATCH v2 22/24] treewide: remove double forward declaration of read_in_full Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 23/24] treewide: reduce includes of cache.h in other headers Elijah Newren via GitGitGadget
2023-04-04 1:22 ` [PATCH v2 24/24] mailmap, quote: move declarations of global vars to correct unit Elijah Newren via GitGitGadget
2023-04-05 17:30 ` [PATCH v2 00/24] Header cleanups (splitting up cache.h) Calvin Wan
2023-04-07 7:08 ` Elijah Newren
2023-04-10 15:54 ` Calvin Wan
2023-04-11 3:00 ` [PATCH v3 00/23] " Elijah Newren via GitGitGadget
2023-04-11 3:00 ` [PATCH v3 01/23] treewide: be explicit about dependence on trace.h & trace2.h Elijah Newren via GitGitGadget
2023-04-11 3:00 ` [PATCH v3 02/23] treewide: be explicit about dependence on advice.h Elijah Newren via GitGitGadget
2023-04-11 3:00 ` [PATCH v3 03/23] treewide: be explicit about dependence on convert.h Elijah Newren via GitGitGadget
2023-04-11 3:00 ` [PATCH v3 04/23] treewide: be explicit about dependence on pack-revindex.h Elijah Newren via GitGitGadget
2023-04-11 3:00 ` [PATCH v3 05/23] treewide: be explicit about dependence on oid-array.h Elijah Newren via GitGitGadget
2023-04-11 7:41 ` [PATCH v3 06/23] treewide: be explicit about dependence on mem-pool.h Elijah Newren
2023-04-11 7:41 ` [PATCH v3 07/23] treewide: remove unnecessary cache.h inclusion Elijah Newren
2023-04-15 11:06 ` [PATCH/RFD] fix connection via git protocol Michael J Gruber
2023-04-16 5:47 ` Elijah Newren
2023-04-16 5:51 ` Elijah Newren
2023-04-16 11:21 ` Michael J Gruber
2023-04-17 7:38 ` Jeff King
2023-04-17 16:33 ` Junio C Hamano
2023-04-17 18:31 ` Junio C Hamano
2023-04-18 3:39 ` Jeff King [this message]
2023-04-18 20:47 ` Junio C Hamano
2023-04-18 1:56 ` Elijah Newren
2023-04-18 3:40 ` Jeff King
2023-04-17 16:29 ` Junio C Hamano
2023-04-18 1:55 ` Elijah Newren
2023-04-18 21:00 ` Junio C Hamano
2023-04-18 21:24 ` Eric Sunshine
2023-04-19 3:16 ` Elijah Newren
2023-04-24 20:57 ` Junio C Hamano
2023-04-19 3:15 ` Elijah Newren
2023-04-11 7:41 ` [PATCH v3 08/23] object-name.h: move declarations for object-name.c functions from cache.h Elijah Newren
2023-04-11 7:41 ` [PATCH v3 09/23] treewide: remove cache.h inclusion due to object-name.h changes Elijah Newren
2023-04-11 7:41 ` [PATCH v3 10/23] git-zlib: move declarations for git-zlib functions from cache.h Elijah Newren
2023-04-11 7:41 ` [PATCH v3 11/23] treewide: remove cache.h inclusion due to git-zlib changes Elijah Newren
2023-04-11 7:41 ` [PATCH v3 12/23] object-file.h: move declarations for object-file.c functions from cache.h Elijah Newren
2023-04-11 7:41 ` [PATCH v3 13/23] treewide: remove cache.h inclusion due to object-file.h changes Elijah Newren
2023-04-11 7:41 ` [PATCH v3 14/23] object.h: move some inline functions and defines from cache.h Elijah Newren
2023-04-11 7:41 ` [PATCH v3 15/23] treewide: remove cache.h inclusion due to object.h changes Elijah Newren
2023-04-11 7:41 ` [PATCH v3 16/23] editor: move editor-related functions and declarations into common file Elijah Newren
2023-04-11 7:41 ` [PATCH v3 17/23] treewide: remove cache.h inclusion due to editor.h changes Elijah Newren
2023-04-11 7:41 ` [PATCH v3 18/23] pager.h: move declarations for pager.c functions from cache.h Elijah Newren
2023-04-11 7:42 ` [PATCH v3 19/23] treewide: remove cache.h inclusion due to pager.h changes Elijah Newren
2023-04-11 7:42 ` [PATCH v3 20/23] cache.h: remove unnecessary includes Elijah Newren
2023-04-11 7:42 ` [PATCH v3 21/23] treewide: remove double forward declaration of read_in_full Elijah Newren
2023-04-11 7:42 ` [PATCH v3 22/23] treewide: reduce includes of cache.h in other headers Elijah Newren
2023-04-11 7:42 ` [PATCH v3 23/23] mailmap, quote: move declarations of global vars to correct unit Elijah Newren
2023-04-11 7:47 ` [PATCH v3 00/23] Header cleanups (splitting up cache.h) Elijah Newren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230418033904.GA60552@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=calvinwan@google.com \
--cc=git@grubix.eu \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=newren@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).