Allow message write buffer to overflow.

With extended protocol limits, overflowing MAX_MSGLEN with single message is possible in MVD and client demo code. Allow msg_write to overflow and add explicit overflow checks to avoid crashing with fatal error.
skullernet · Oct 14, 2024 · c0966a9 · c0966a9
1 parent 5dcc520
commit c0966a9
Show file tree

Hide file tree

Showing 10 changed files with 113 additions and 37 deletions.
diff --git a/src/client/demo.c b/src/client/demo.c
@@ -224,7 +224,9 @@ void CL_EmitDemoFrame(void)
     // emit and flush frame
     emit_delta_frame(oldframe, &cl.frame, lastframe, FRAME_CUR);
 
-    if (cls.demo.buffer.cursize + msg_write.cursize > cls.demo.buffer.maxsize) {
+    if (msg_write.overflowed) {
+        Com_WPrintf("%s: message buffer overflowed\n", __func__);
+    } else if (cls.demo.buffer.cursize + msg_write.cursize > cls.demo.buffer.maxsize) {
         Com_DPrintf("Demo frame overflowed (%u + %u > %u)\n",
                     cls.demo.buffer.cursize, msg_write.cursize, cls.demo.buffer.maxsize);
         cls.demo.frames_dropped++;
@@ -850,16 +852,20 @@ void CL_EmitDemoSnapshot(void)
     MSG_WriteByte(svc_layout);
     MSG_WriteString(cl.layout);
 
-    snap = Z_Malloc(sizeof(*snap) + msg_write.cursize - 1);
-    snap->framenum = cls.demo.frames_read;
-    snap->filepos = pos;
-    snap->msglen = msg_write.cursize;
-    memcpy(snap->data, msg_write.data, msg_write.cursize);
+    if (msg_write.overflowed) {
+        Com_WPrintf("%s: message buffer overflowed\n", __func__);
+    } else {
+        snap = Z_Malloc(sizeof(*snap) + msg_write.cursize - 1);
+        snap->framenum = cls.demo.frames_read;
+        snap->filepos = pos;
+        snap->msglen = msg_write.cursize;
+        memcpy(snap->data, msg_write.data, msg_write.cursize);
 
-    cls.demo.snapshots = Z_Realloc(cls.demo.snapshots, sizeof(cls.demo.snapshots[0]) * Q_ALIGN(cls.demo.numsnapshots + 1, MIN_SNAPSHOTS));
-    cls.demo.snapshots[cls.demo.numsnapshots++] = snap;
+        cls.demo.snapshots = Z_Realloc(cls.demo.snapshots, sizeof(cls.demo.snapshots[0]) * Q_ALIGN(cls.demo.numsnapshots + 1, MIN_SNAPSHOTS));
+        cls.demo.snapshots[cls.demo.numsnapshots++] = snap;
 
-    Com_DPrintf("[%d] snaplen %u\n", cls.demo.frames_read, msg_write.cursize);
+        Com_DPrintf("[%d] snaplen %u\n", cls.demo.frames_read, msg_write.cursize);
+    }
 
     SZ_Clear(&msg_write);
 

diff --git a/src/client/gtv.c b/src/client/gtv.c
@@ -29,6 +29,8 @@ static byte     gtv_send_buffer[MAX_GTS_MSGLEN*2];
 
 static byte     gtv_message_buffer[MAX_MSGLEN];
 
+static void drop_client(const char *reason);
+
 static void build_gamestate(void)
 {
     centity_t *ent;
@@ -177,6 +179,13 @@ void CL_GTV_EmitFrame(void)
 
     MSG_WriteShort(0);      // end of packetentities
 
+    // check for overflow
+    if (msg_write.overflowed) {
+        SZ_Clear(&msg_write);
+        drop_client("frame overflowed");
+        return;
+    }
+
     SZ_Write(&cls.gtv.message, msg_write.data, msg_write.cursize);
     SZ_Clear(&msg_write);
 }
@@ -264,6 +273,14 @@ void CL_GTV_Resume(void)
 
     build_gamestate();
     emit_gamestate();
+
+    // check for overflow
+    if (msg_write.overflowed) {
+        SZ_Clear(&msg_write);
+        drop_client("gamestate overflowed");
+        return;
+    }
+
     write_message(GTS_STREAM_DATA);
     SZ_Clear(&msg_write);
 }

diff --git a/src/common/common.c b/src/common/common.c
@@ -530,7 +530,7 @@ void Com_Error(error_type_t code, const char *fmt, ...)
     // overlap with one of the arguments!
     memcpy(com_errorMsg, msg, len + 1);
 
-    // fix up drity message buffers
+    // fix up dirty message buffers
     MSG_Init();
 
     // abort any console redirects

diff --git a/src/common/msg.c b/src/common/msg.c
@@ -46,19 +46,16 @@ const usercmd_t         nullUserCmd;
 =============
 MSG_Init
 
-Initialize default buffers, clearing allow overflow/underflow flags.
-
-This is the only place where writing buffer is initialized. Writing buffer is
-never allowed to overflow.
-
-Reading buffer is reinitialized in many other places. Reinitializing will set
-the allow underflow flag as appropriate.
+Initialize default buffers (also called from Com_Error).
+This is the only place where writing buffer is initialized.
 =============
 */
 void MSG_Init(void)
 {
     SZ_Init(&msg_read, msg_read_buffer, MAX_MSGLEN, "msg_read");
     SZ_Init(&msg_write, msg_write_buffer, MAX_MSGLEN, "msg_write");
+    msg_read.allowunderflow = true;
+    msg_write.allowoverflow = true;
 }
 
 

diff --git a/src/server/game.c b/src/server/game.c
@@ -96,6 +96,9 @@ static void PF_Unicast(edict_t *ent, qboolean reliable)
         goto clear;
     }
 
+    if (msg_write.overflowed)
+        Com_Error(ERR_DROP, "%s: message buffer overflowed", __func__);
+
     clientNum = NUM_FOR_EDICT(ent) - 1;
     if (clientNum < 0 || clientNum >= sv_maxclients->integer) {
         Com_WPrintf("%s to a non-client %d\n", __func__, clientNum);

diff --git a/src/server/main.c b/src/server/main.c
@@ -1809,6 +1809,9 @@ static void SV_RunGameFrame(void)
         time_after_game = Sys_Milliseconds();
 #endif
 
+    if (msg_write.overflowed)
+        Com_Error(ERR_DROP, "%s: message buffer overflowed", __func__);
+
     if (msg_write.cursize) {
         Com_WPrintf("Game left %u bytes "
                     "in multicast buffer, cleared.\n",

diff --git a/src/server/mvd.c b/src/server/mvd.c
@@ -831,6 +831,13 @@ static void resume_streams(void)
     build_gamestate();
     emit_gamestate();
 
+    // check for overflow
+    if (msg_write.overflowed) {
+        SZ_Clear(&msg_write);
+        mvd_error("gamestate overflowed");
+        return;
+    }
+
     FOR_EACH_ACTIVE_GTV(client) {
         // send gamestate
         write_message(client, GTS_STREAM_DATA);
@@ -917,9 +924,6 @@ static bool mvd_enable(void)
     // don't timeout
     mvd.clients_active = svs.realtime;
 
-    // check for activation
-    check_players_activity();
-
     return true;
 }
 
@@ -1040,7 +1044,7 @@ void SV_MvdEndFrame(void)
     emit_frame();
 
     // if reliable message and frame update don't fit, kick all clients
-    if (mvd.message.cursize + msg_write.cursize >= MAX_MSGLEN) {
+    if (msg_write.overflowed || mvd.message.cursize + msg_write.cursize >= MAX_MSGLEN) {
         SZ_Clear(&msg_write);
         mvd_error("frame overflowed");
         return;
@@ -1573,6 +1577,11 @@ static void parse_stream_start(gtv_client_t *client)
     // send gamestate if active
     if (mvd.active) {
         emit_gamestate();
+        if (msg_write.overflowed) {
+            SZ_Clear(&msg_write);
+            drop_client(client, "gamestate overflowed");
+            return;
+        }
         write_message(client, GTS_STREAM_DATA);
         SZ_Clear(&msg_write);
     } else {
@@ -1955,7 +1964,7 @@ static void mvd_drop(gtv_serverop_t op)
 // something bad happened, remove all clients
 static void mvd_error(const char *reason)
 {
-    Com_EPrintf("Fatal MVD error: %s\n", reason);
+    Com_EPrintf("Fatal MVD server error: %s\n", reason);
 
     // stop recording
     rec_stop();
@@ -2010,6 +2019,13 @@ void SV_MvdMapChanged(void)
         build_gamestate();
         emit_gamestate();
 
+        // check for overflow
+        if (msg_write.overflowed) {
+            SZ_Clear(&msg_write);
+            mvd_error("gamestate overflowed");
+            return;
+        }
+
         // send gamestate to all MVD clients
         FOR_EACH_ACTIVE_GTV(client) {
             write_message(client, GTS_STREAM_DATA);
@@ -2246,11 +2262,20 @@ static void rec_start(qhandle_t demofile)
     magic = MVD_MAGIC;
     FS_Write(&magic, 4, demofile);
 
-    if (mvd.active) {
-        emit_gamestate();
-        rec_write();
+    if (!mvd.active)
+        return;
+
+    emit_gamestate();
+
+    // check for overflow
+    if (msg_write.overflowed) {
         SZ_Clear(&msg_write);
+        mvd_error("gamestate overflowed");
+        return;
     }
+
+    rec_write();
+    SZ_Clear(&msg_write);
 }
 
 /*

diff --git a/src/server/mvd/client.c b/src/server/mvd/client.c
@@ -626,19 +626,23 @@ static void demo_emit_snapshot(mvd_t *mvd)
         MSG_WriteString(mvd->layout);
     }
 
-    snap = MVD_Malloc(sizeof(*snap) + msg_write.cursize - 1);
-    snap->framenum = mvd->framenum;
-    snap->filepos = pos;
-    snap->msglen = msg_write.cursize;
-    memcpy(snap->data, msg_write.data, msg_write.cursize);
-
-    if (!mvd->snapshots)
-        mvd->snapshots = MVD_Malloc(sizeof(mvd->snapshots[0]) * MIN_SNAPSHOTS);
-    else
-        mvd->snapshots = Z_Realloc(mvd->snapshots, sizeof(mvd->snapshots[0]) * Q_ALIGN(mvd->numsnapshots + 1, MIN_SNAPSHOTS));
-    mvd->snapshots[mvd->numsnapshots++] = snap;
+    if (msg_write.overflowed) {
+        Com_WPrintf("%s: message buffer overflowed\n", __func__);
+    } else {
+        snap = MVD_Malloc(sizeof(*snap) + msg_write.cursize - 1);
+        snap->framenum = mvd->framenum;
+        snap->filepos = pos;
+        snap->msglen = msg_write.cursize;
+        memcpy(snap->data, msg_write.data, msg_write.cursize);
+
+        if (!mvd->snapshots)
+            mvd->snapshots = MVD_Malloc(sizeof(mvd->snapshots[0]) * MIN_SNAPSHOTS);
+        else
+            mvd->snapshots = Z_Realloc(mvd->snapshots, sizeof(mvd->snapshots[0]) * Q_ALIGN(mvd->numsnapshots + 1, MIN_SNAPSHOTS));
+        mvd->snapshots[mvd->numsnapshots++] = snap;
 
-    Com_DPrintf("[%d] snaplen %u\n", mvd->framenum, msg_write.cursize);
+        Com_DPrintf("[%d] snaplen %u\n", mvd->framenum, msg_write.cursize);
+    }
 
     SZ_Clear(&msg_write);
 
@@ -1976,6 +1980,12 @@ void MVD_StreamedRecord_f(void)
 
     emit_gamestate(mvd);
 
+    // check for overflow
+    if (msg_write.overflowed) {
+        ret = Q_ERR(EMSGSIZE);
+        goto fail;
+    }
+
     // write magic
     magic = MVD_MAGIC;
     ret = FS_Write(&magic, 4, f);

diff --git a/src/server/save.c b/src/server/save.c
@@ -74,6 +74,12 @@ static int write_server_file(savetype_t autosave)
     }
     MSG_WriteString(NULL);
 
+    // check for overflow
+    if (msg_write.overflowed) {
+        SZ_Clear(&msg_write);
+        return -1;
+    }
+
     // write server state
     ret = FS_WriteFile("save/" SAVE_CURRENT "/server.ssv",
                        msg_write.data, msg_write.cursize);

diff --git a/src/server/send.c b/src/server/send.c
@@ -271,6 +271,9 @@ void SV_Multicast(const vec3_t origin, multicast_t to)
     if (to && !origin)
         Com_Error(ERR_DROP, "%s: NULL origin", __func__);
 
+    if (msg_write.overflowed)
+        Com_Error(ERR_DROP, "%s: message buffer overflowed", __func__);
+
     if (!msg_write.cursize) {
         Com_DPrintf("%s with empty data\n", __func__);
         return;
@@ -389,6 +392,8 @@ void SV_ClientAddMessage(client_t *client, int flags)
 {
     int len;
 
+    Q_assert(!msg_write.overflowed);
+
     if (!msg_write.cursize) {
         return;
     }
@@ -755,6 +760,8 @@ static void write_datagram_old(client_t *client)
     }
 #endif
 
+    Q_assert(!msg_write.overflowed);
+
     // send the datagram
     cursize = Netchan_Transmit(&client->netchan,
                                msg_write.cursize,
@@ -819,6 +826,8 @@ static void write_datagram_new(client_t *client)
     }
 #endif
 
+    Q_assert(!msg_write.overflowed);
+
     // send the datagram
     cursize = Netchan_Transmit(&client->netchan,
                                msg_write.cursize,