Skip to content

Commit

Permalink
net_irc, door_irc: Fix various IRC bugs.
Browse files Browse the repository at this point in the history 
There were several compounding bugs causing
various issues with IRC, in particular with
using door_irc to connect to IRC, both to
an external network and directly to the local BBS.

* menu.c: Reusing a small buffer truncated arguments
  to the menu handler. We now use a significantly
  large buffer.
* door_irc: Abort client loop if irc_read returns <= 0,
  fixing infinite loop on disconnect.
* door_irc: Fix passing wrong variable name into
  bbs_readline_append.
* door_irc: If nickname is already in use, automatically
  append underscores to try to construct a unique nickname,
  so that this works even if the user is already logged in
  on another node.
* net_irc: Allow nicknames to be either the actual username
  as before or also the username followed by an underscore
  and other characters, allowing for the same user to log in
  from multiple nodes.
* net_irc: Properly respond with an error message for unhandled
  messages.
  • Loading branch information
@InterLinked1
InterLinked1 committed Dec 14, 2023
1 parent 8189015 commit fba4de6
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 32 deletions.
21 changes: 14 additions & 7 deletions bbs/menu.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -470,14 +470,16 @@ static int menu_set_title(struct bbs_node *node, const char *name)
return bbs_node_set_term_title(node, stripped);
}

#define SCRATCH_BUF_SIZE 256

/*!
* \brief Run the BBS on a node
* \param node node
* \param menuname Name of menu to run
* \param stack Current stack count
* \param optreq Option request string
*/
static int bbs_menu_run(struct bbs_node *node, const char *menuname, const char *menuitemname, int stack, const char *optreq)
static int bbs_menu_run(struct bbs_node *node, const char *menuname, const char *menuitemname, int stack, const char *optreq, char *restrict scratchbuf)
{
int res;
char opt = 0;
Expand Down Expand Up @@ -653,13 +655,15 @@ static int bbs_menu_run(struct bbs_node *node, const char *menuname, const char
* we need to be able to recurse efficiently with arguments that other handlers don't (and shouldn't) get. */
if (STARTS_WITH(menuitem->action, "menu:")) {
/* Execute another menu, recursively */
res = bbs_menu_run(node, menuitem->action + 5, menuitem->name, stack, optreq ? optreq + 1 : NULL);
res = bbs_menu_run(node, menuitem->action + 5, menuitem->name, stack, optreq ? optreq + 1 : NULL, scratchbuf);
} else {
char *handler, *args;
/* At this point in the function, we're done with the options buffer, and if we need it again, we'll fill it again.
* So, reuse that buffer since we're in a recursive function, and we really want to avoid stack allocations if possible. */
safe_strncpy(options, menuitem->action, sizeof(options));
handler = options;
/* We need a buffer that is sufficiently long enough to avoid truncation, so use the scratch buffer. */
safe_strncpy(scratchbuf, menuitem->action, SCRATCH_BUF_SIZE);
if (strlen(scratchbuf) >= SCRATCH_BUF_SIZE - 1) {
bbs_warning("Truncation occurred copying menu item arguments into buffer of size %d\n", SCRATCH_BUF_SIZE);
}
handler = scratchbuf;
args = strchr(handler, ':');
if (args) {
*args++ = '\0'; /* We just want the name of the menu handler to use. */
Expand Down Expand Up @@ -723,7 +727,10 @@ static int bbs_menu_run(struct bbs_node *node, const char *menuname, const char

int bbs_node_menuexec(struct bbs_node *node)
{
return bbs_menu_run(node, DEFAULT_MENU, NULL, 0, NULL);
/* Declare just once rather than on each stack frame, since bbs_menu_run recurses a lot.
* This allows us to save (BBS_MAX_MENUSTACK - 1) * SCRATCH_BUF_SIZE bytes of stack space. */
char scratchbuf[SCRATCH_BUF_SIZE];
return bbs_menu_run(node, DEFAULT_MENU, NULL, 0, NULL, scratchbuf);
}

static int load_config(int reload)
Expand Down
4 changes: 2 additions & 2 deletions bbs/variables.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ const char *bbs_node_var_get(struct bbs_node *node, const char *key)
return value;
}

int bbs_node_var_get_buf(struct bbs_node *node, const char *key, char *buf, size_t len)
int bbs_node_var_get_buf(struct bbs_node *node, const char *key, char *restrict buf, size_t len)
{
const char *s;

Expand All @@ -459,7 +459,7 @@ int bbs_node_var_get_buf(struct bbs_node *node, const char *key, char *buf, size
return s ? 0 : -1;
}

int bbs_node_substitute_vars(struct bbs_node *node, const char *sub, char *buf, size_t len)
int bbs_node_substitute_vars(struct bbs_node *node, const char *sub, char *restrict buf, size_t len)
{
char varname[64];
char *bufstart = buf;
Expand Down
34 changes: 29 additions & 5 deletions doors/door_irc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,15 +159,19 @@ static int __chat_send(struct client_relay *client, struct participant *sender,

/* If sender is set, it's safe to use even with no locks, because the sender is a calling function of this one */
if (sender) {
bbs_debug(7, "Broadcasting %s to %s,%s (except node %d): %s%.*s\n", dorelay ? "to IRC" : "from IRC", client->name, channel, sender->node->id, datestr, len, msg);
bbs_debug(7, "Broadcasting %s -> %s,%s (except node %d): %s%.*s\n", dorelay ? "to IRC" : "from IRC", client->name, channel, sender->node->id, datestr, len, msg);
} else {
bbs_debug(7, "Broadcasting %s to %s,%s: %s%.*s\n", dorelay ? "to IRC" : "from IRC", client->name, channel, datestr, len, msg);
bbs_debug(7, "Broadcasting %s -> %s,%s: %s%.*s\n", dorelay ? "to IRC" : "from IRC", client->name, channel, datestr, len, msg);
}

/* Relay the message to everyone */
RWLIST_RDLOCK(&client->participants);
if (dorelay) {
bbs_irc_client_send(client->name, channel, msg); /* Actually send to IRC */
char prefix[32] = "";
if (sender) { /* XXX Can this ever be NULL here? */
snprintf(prefix, sizeof(prefix), "%s@%u", bbs_username(sender->node->user), sender->node->id);
}
bbs_irc_client_msg(client->name, channel, prefix, msg); /* Actually send to IRC */
}
RWLIST_TRAVERSE(&client->participants, p, entry) {
ssize_t res;
Expand Down Expand Up @@ -415,6 +419,7 @@ static int irc_single_client(struct bbs_node *node, char *constring, const char
char usernamebuf[24];
char passwordbuf[64];
char buf[2048];
int underscores = 0;
char *username, *password, *hostname, *portstr;

/* Parse the arguments.
Expand Down Expand Up @@ -584,11 +589,14 @@ static int irc_single_client(struct bbs_node *node, char *constring, const char
* so just pass 0 as poll should always return > 0 anyways, immediately,
* since we haven't read any data yet to quell the poll. */

/* Another clunky thing. Need to get data using irc_read, but we want to buffer it using a bbs_node_readline struct.
/* Another clunky thing. Need to get data using irc_read, but we want to buffer it using a readline_data struct.
* So use bbs_readline_append.
*/
bres = irc_read(ircl, tmpbuf, sizeof(tmpbuf));
res = bbs_readline_append(&rldata, "\r\n", tmpbuf, (size_t) res, &ready);
if (bres <= 0) {
break;
}
res = bbs_readline_append(&rldata, "\r\n", tmpbuf, (size_t) bres, &ready);
if (!ready) {
continue;
}
Expand All @@ -609,6 +617,22 @@ static int irc_single_client(struct bbs_node *node, char *constring, const char
switch (irc_msg_type(msg)) {
case IRC_NUMERIC:
bbs_node_writef(node, "%s %d %s\n", NODE_IS_TDD(node) ? "" : S_IF(irc_msg_prefix(msg)), irc_msg_numeric(msg), irc_msg_body(msg));
switch (irc_msg_numeric(msg)) {
case 433: /* Nickname in use - very likely to happen if already logged in using a regular IRC client */
if (underscores++ < 5) {
char newnick[32];
#define UNDERSCORES "_____"
snprintf(newnick, sizeof(newnick), "%s%.*s", irc_client_username(ircl), underscores, UNDERSCORES);
bbs_debug(1, "Auto-changing nickname to '%s' to avoid conflict\n", newnick);
irc_client_change_nick(ircl, newnick);
/* Assuming this succeeds, try joining the channel we intended to,
* since that would have failed. */
irc_client_channel_join(ircl, channel);
}
break;
default:
break;
}
break;
case IRC_CMD_PRIVMSG:
case IRC_CMD_NOTICE:
Expand Down
4 changes: 2 additions & 2 deletions include/variables.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ const char *bbs_node_var_get(struct bbs_node *node, const char *key);
* \param len Size of buf.
* \retval 0 if found, -1 if not found
*/
int bbs_node_var_get_buf(struct bbs_node *node, const char *key, char *buf, size_t len);
int bbs_node_var_get_buf(struct bbs_node *node, const char *key, char *restrict buf, size_t len);

/*!
* \brief Substitute variables in a string
Expand All @@ -147,4 +147,4 @@ int bbs_node_var_get_buf(struct bbs_node *node, const char *key, char *buf, size
* \param len Size of buf.
* \retval 0 if found, -1 if not found
*/
int bbs_node_substitute_vars(struct bbs_node *node, const char *sub, char *buf, size_t len);
int bbs_node_substitute_vars(struct bbs_node *node, const char *sub, char *restrict buf, size_t len);
63 changes: 47 additions & 16 deletions nets/net_irc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2282,12 +2282,38 @@ static void broadcast_nick_change(struct irc_user *user, const char *oldnick)
RWLIST_UNLOCK(&channels);
}

static void do_identity_update(struct irc_user *user)
{
if (!user->registered) {
add_user(user);
} else {
hostmask(user);
}
}

static void handle_nick(struct irc_user *user, char *s)
{
if (user->node->user && strcasecmp(s, bbs_username(user->node->user))) {
/* Don't allow changing nick if already logged in, unless it's to our actual username. */
send_numeric(user, 902, "You must use a nick assigned to you\r\n");
} else if (bbs_user_exists(s)) {
const char *suffix;
size_t usernamelen = strlen(bbs_username(user->node->user));
/* Don't allow changing nick if already logged in, unless it's to our actual username,
* or something that starts with it, followed by an underscore. */
if (strncasecmp(s, bbs_username(user->node->user), usernamelen)) {
send_numeric(user, 902, "You must use a nick assigned to you\r\n");
return;
}
/* If username is jsmith, allow jsmith_, jsmith__, jsmith_m, etc.
* But it has to be jsmith or start with jsmith_ */
suffix = s + usernamelen;
/* If the first n characters match but not the whole string the suffix must be non-empty */
bbs_assert(!strlen_zero(suffix));
if (*suffix != '_') {
send_numeric(user, 902, "You must use a nick assigned to you\r\n");
return;
}
}

if (bbs_user_exists(s)) {
send_numeric(user, 433, "%s :Nickname is already in use.\r\n", s);
send_reply(user, "NOTICE AUTH :*** This nickname is registered. Please choose a different nickname, or identify using NickServ\r\n");
/* Client will need to send NS IDENTIFY <password> or PRIVMSG NickServ :IDENTIFY <password> */
Expand All @@ -2307,9 +2333,10 @@ static void handle_nick(struct irc_user *user, char *s)
}
user->nickname = newnick;
RWLIST_UNLOCK(&users);
do_identity_update(user);
if (!s_strlen_zero(oldnick)) {
send_reply(user, ":%s NICK %s\r\n", oldnick, user->nickname);
broadcast_nick_change(user, oldnick); /* XXX Won't actually traverse, if registered users aren't allowed to change nicks? */
broadcast_nick_change(user, oldnick);
}
}
}
Expand Down Expand Up @@ -2341,11 +2368,7 @@ static void handle_identify(struct irc_user *user, char *s)
if (strlen_zero(user->nickname) || strcasecmp(username, user->nickname)) {
REPLACE(user->nickname, username);
}
if (!user->registered) {
add_user(user);
} else {
hostmask(user);
}
do_identity_update(user);
send_numeric(user, 900, IDENT_PREFIX_FMT " %s You are now logged in as %s\r\n", IDENT_PREFIX_ARGS(user), user->username, user->username);
}
}
Expand Down Expand Up @@ -2945,9 +2968,9 @@ static int do_sasl_auth(struct irc_user *user, char *s)
send_numeric(user, 904, "SASL authentication failed\r\n");
return -1;
}
send_numeric(user, 903, "SASL authentication successful\r\n");
/* The prefix is nick!ident@host */
send_numeric(user, 900, IDENT_PREFIX_FMT " %s You are now logged in as %s\r\n", IDENT_PREFIX_ARGS(user), user->username, user->username);
send_numeric(user, 903, "SASL authentication successful\r\n");
return 0;
}

Expand Down Expand Up @@ -3028,15 +3051,14 @@ static void handle_client(struct irc_user *user)
if (!started) {
/* Users that aren't started, and more importantly, in the user list, (!started, !user->registered)
* can change their nickname arbitrarily, but can't use it without identifying. */
user->nickname = strdup(s);
REPLACE(user->nickname, s);
bbs_debug(5, "Nickname is %s\n", user->nickname);
}
} else if (!strcasecmp(command, "USER")) { /* Whole message is something like 'ambassador * * :New Now Know How' */
char *realname;
bbs_debug(5, "Username data is %s\n", s);
realname = strsep(&s, " ");
free_if(user->realname);
user->realname = strdup(realname);
REPLACE(user->realname, realname);
if (handle_user(user)) {
break;
}
Expand Down Expand Up @@ -3098,7 +3120,10 @@ static void handle_client(struct irc_user *user)
bbs_error("Client %p already started?\n", user);
}
} else {
char *command;
bbs_warning("Unhandled message: %s\n", s);
command = strsep(&s, " ");
send_numeric2(user, 421, "%s :Unknown command or invalid in current state\r\n", command);
}
} else {
bbs_warning("Unhandled message: %s\n", s);
Expand Down Expand Up @@ -3166,7 +3191,7 @@ static void handle_client(struct irc_user *user)
send_reply(user, "NOTICE AUTH :*** This server requires SASL for authentication. Please reconnect with SASL enabled.\r\n");
goto quit; /* Disconnect at this point, there's no point in lingering around further. */
/* We can't necessarily use %s (user->username) instead of %p (user), since if require_sasl == false, we might not have a username still. */
} else if (!user->node->user) {
} else if (!user->node->user || !user->registered) {
char *target;
/* Okay to message NickServ without being registered, but nobody else. */
/* Can be NS IDENTIFY <password> or a regular PRIVMSG */
Expand All @@ -3186,7 +3211,13 @@ static void handle_client(struct irc_user *user)
continue;
}
}
send_numeric(user, 451, "You have not registered\r\n");
if (!user->node->user) {
send_numeric(user, 451, "You have not registered\r\n");
} else {
send_numeric(user, 433, "Nickname is already in use\r\n");
}
} else if (!user->registered) {
send_numeric(user, 433, "Nickname is already in use\r\n");
} else if (!strcasecmp(command, "NS")) { /* NickServ alias */
nickserv(user, s);
} else if (!strcasecmp(command, "CS")) { /* ChanServ alias (much like NS ~ NickServ) */
Expand Down Expand Up @@ -3376,7 +3407,7 @@ static void handle_client(struct irc_user *user)
/* Ignore SQUIT for now, since this is a single-server network */
} else {
send_numeric2(user, 421, "%s :Unknown command\r\n", command);
bbs_warning("%p: Unhandled message: %s %s\n", user, command, s);
bbs_warning("%p: Unhandled message: %s %s\n", user, command, S_IF(s));
}
}
}
Expand Down

0 comments on commit fba4de6

Please sign in to comment.