[hackers] Re: [sent][PATCH] Avoid out-of-bounds access when a slide input line begins with \0

[Chris Down]

Chris Down writes:
and maybe fgets can simply be rewritten using getline() at:
        http://git.suckless.org/sent/file/sent.c.html#l417

Hmm, unless I'm missing something, getline() actually would hinder 
here since we need to use separate heap storage for each line, but 
getline() optimises for the case where `char **lineptr` can be reused 
for each line. As such we'd still have to strdup(), or entries in 
s->lines would always end up being the same.

Oh, I think I see what you mean now. :-)

Patch attached, not sure it's simpler though since it requires all this futzing 
around with buf and n.
>From bbcfb1b22441b9b8b55834f66280d160c24b6560 Mon Sep 17 00:00:00 2001
From: Chris Down <ch...@chrisdown.name>
Date: Wed, 13 May 2020 01:21:02 +0100
Subject: [sent][PATCH] Allocate line up front with getline() instead of
 strdup()ing later
To: hackers@suckless.org
Cc: Markus Teich <tei...@fs.tum.de>,
    Laslo Hunhold <d...@frign.de>,
    Hiltjo Posthuma <hil...@codemadness.org>

---
 config.mk |  2 +-
 sent.c    | 22 ++++++++++++++--------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/config.mk b/config.mk
index d61c554..a7ebd21 100644
--- a/config.mk
+++ b/config.mk
@@ -20,7 +20,7 @@ LIBS = -L/usr/lib -lc -lm -L${X11LIB} -lXft -lfontconfig -lX11
 #LIBS = -L/usr/local/lib -lc -lm -L${X11LIB} -lXft -lfontconfig -lX11
 
 # flags
-CPPFLAGS = -DVERSION=\"${VERSION}\" -D_XOPEN_SOURCE=600
+CPPFLAGS = -DVERSION=\"${VERSION}\" -D_XOPEN_SOURCE=600 
-D_POSIX_C_SOURCE=200809L
 CFLAGS += -g -std=c99 -pedantic -Wall ${INCS} ${CPPFLAGS}
 LDFLAGS += -g ${LIBS}
 #CFLAGS += -std=c99 -pedantic -Wall -Os ${INCS} ${CPPFLAGS}
diff --git a/sent.c b/sent.c
index 9534fca..4bf6973 100644
--- a/sent.c
+++ b/sent.c
@@ -407,17 +407,18 @@ void
 load(FILE *fp)
 {
        static size_t size = 0;
-       size_t blen, maxlines;
-       char buf[BUFSIZ], *p;
+       size_t blen, maxlines, bufsiz = 0;
+       char *buf = NULL;
+       ssize_t n;
        Slide *s;
 
        /* read each line from fp and add it to the item list */
        while (1) {
                /* eat consecutive empty lines */
-               while ((p = fgets(buf, sizeof(buf), fp)))
+               while ((n = getline(&buf, &bufsiz, fp)) > 0)
                        if (strcmp(buf, "\n") != 0 && buf[0] != '#')
                                break;
-               if (!p)
+               if (n < 0)
                        break;
 
                if ((slidecount+1) * sizeof(*slides) >= size)
@@ -443,8 +444,8 @@ load(FILE *fp)
                        }
 
                        blen = strlen(buf);
-                       if (!(s->lines[s->linecount] = strdup(buf)))
-                               die("sent: Unable to strdup:");
+                       s->lines[s->linecount] = buf;
+
                        if (s->lines[s->linecount][blen-1] == '\n')
                                s->lines[s->linecount][blen-1] = '\0';
 
@@ -454,11 +455,16 @@ load(FILE *fp)
 
                        if (s->lines[s->linecount][0] == '\\')
                                memmove(s->lines[s->linecount], 
&s->lines[s->linecount][1], blen);
+
+                       /* make getline() uses a new allocation next time */
+                       buf = NULL;
+                       n = 0;
+
                        s->linecount++;
-               } while ((p = fgets(buf, sizeof(buf), fp)) && strcmp(buf, "\n") 
!= 0);
+               } while ((n = getline(&buf, &bufsiz, fp)) > 0 && strcmp(buf, 
"\n") != 0);
 
                slidecount++;
-               if (!p)
+               if (n < 0)
                        break;
        }
 
-- 
2.26.2