Re: quiz: Fix multi-line questions (trailing newline)
On Wed, 10 Mar 2021 22:49:13 -0500, Alex Karle wrote:
> > This would now be clearer as:
> >
> > if (qlen > 0 && qp->q_text[qlen - 1] == '\\') {
>
> I agree this is clearer, but I think we'll need a `qlen = 0`
> initialization up above the while loop if we go with this variation
> (otherwise an uninitialized qlen could be > 0, causing indexing a null
> q_text, right?). Apologies if this was implied by your comment, thought
> it couldn't hurt to flag!
You are absolutely correct.
- todd
Re: quiz: Fix multi-line questions (trailing newline)
On Wed, Mar 10, 2021 at 02:03:33PM -0700, Todd C. Miller wrote:
> On Wed, 10 Mar 2021 21:15:13 +0100, Christian Weisgerber wrote:
>
> > Right. Next iteration:
>
> Looks better, one minor nit inline.
>
> - todd
My ok doesn't weigh much, but I wanted to chime in to say thanks for
picking this up!
I've got one small addition inline. Otherwise, I've applied it and
tested that the multi-line bug is fixed--looks good to me.
Thanks,
Alex
>
> > Index: quiz.c
> > ===
> > RCS file: /cvs/src/games/quiz/quiz.c,v
> > retrieving revision 1.30
> > diff -u -p -r1.30 quiz.c
> > --- quiz.c 24 Aug 2018 11:14:49 - 1.30
> > +++ quiz.c 10 Mar 2021 20:04:37 -
> > @@ -48,7 +48,6 @@ static QE qlist;
> > static int catone, cattwo, tflag;
> > static u_int qsize;
> >
> > -char *appdstr(char *, const char *, size_t);
> > voiddowncase(char *);
> > voidget_cats(char *, char *);
> > voidget_file(const char *);
> > @@ -110,7 +109,8 @@ get_file(const char *file)
> > {
> > FILE *fp;
> > QE *qp;
> > - size_t len;
> > + ssize_t len;
> > + size_t qlen, size;
> > char *lp;
> >
> > if ((fp = fopen(file, "r")) == NULL)
> > @@ -123,26 +123,36 @@ get_file(const char *file)
> > */
> > qp = &qlist;
> > qsize = 0;
> > - while ((lp = fgetln(fp, &len)) != NULL) {
> > + lp = NULL;
> > + size = 0;
> > + while ((len = getline(&lp, &size, fp)) != -1) {
> > if (lp[len - 1] == '\n')
> > - --len;
> > + lp[--len] = '\0';
> > + if (qp->q_text)
> > + qlen = strlen(qp->q_text);
> > if (qp->q_text && qp->q_text[0] != '\0' &&
>
> This would now be clearer as:
>
> if (qlen > 0 && qp->q_text[qlen - 1] == '\\') {
I agree this is clearer, but I think we'll need a `qlen = 0`
initialization up above the while loop if we go with this variation
(otherwise an uninitialized qlen could be > 0, causing indexing a null
q_text, right?). Apologies if this was implied by your comment, thought
it couldn't hurt to flag!
>
> > - qp->q_text[strlen(qp->q_text) - 1] == '\\')
> > - qp->q_text = appdstr(qp->q_text, lp, len);
> > - else {
> > + qp->q_text[qlen - 1] == '\\') {
> > + qp->q_text[--qlen] = '\0';
> > + qlen += len;
> > + qp->q_text = realloc(qp->q_text, qlen + 1);
> > + if (qp->q_text == NULL)
> > + errx(1, "realloc");
> > + strlcat(qp->q_text, lp, qlen + 1);
> > + } else {
> > if ((qp->q_next = malloc(sizeof(QE))) == NULL)
> > errx(1, "malloc");
> > qp = qp->q_next;
> > - if ((qp->q_text = malloc(len + 1)) == NULL)
> > - errx(1, "malloc");
> > - /* lp may not be zero-terminated; cannot use strlcpy */
> > - strncpy(qp->q_text, lp, len);
> > - qp->q_text[len] = '\0';
> > + qp->q_text = strdup(lp);
> > + if (qp->q_text == NULL)
> > + errx(1, "strdup");
> > qp->q_asked = qp->q_answered = FALSE;
> > qp->q_next = NULL;
> > ++qsize;
> > }
> > }
> > + free(lp);
> > + if (ferror(fp))
> > + err(1, "getline");
> > (void)fclose(fp);
> > }
> >
> > @@ -316,32 +326,6 @@ next_cat(const char *s)
> > esc = 0;
> > break;
> > }
> > -}
> > -
> > -char *
> > -appdstr(char *s, const char *tp, size_t len)
> > -{
> > - char *mp;
> > - const char *sp;
> > - int ch;
> > - char *m;
> > -
> > - if ((m = malloc(strlen(s) + len + 1)) == NULL)
> > - errx(1, "malloc");
> > - for (mp = m, sp = s; (*mp++ = *sp++) != '\0'; )
> > - ;
> > - --mp;
> > - if (*(mp - 1) == '\\')
> > - --mp;
> > -
> > - while ((ch = *mp++ = *tp++) && ch != '\n')
> > - ;
> > - if (*(mp - 2) == '\\')
> > - mp--;
> > - *mp = '\0';
> > -
> > - free(s);
> > - return (m);
> > }
> >
> > void
> > --
> > Christian "naddy" Weisgerber na...@mips.inka.de
>
Re: quiz: Fix multi-line questions (trailing newline)
On Wed, 10 Mar 2021 21:15:13 +0100, Christian Weisgerber wrote:
> Right. Next iteration:
Looks better, one minor nit inline.
- todd
> Index: quiz.c
> ===
> RCS file: /cvs/src/games/quiz/quiz.c,v
> retrieving revision 1.30
> diff -u -p -r1.30 quiz.c
> --- quiz.c24 Aug 2018 11:14:49 - 1.30
> +++ quiz.c10 Mar 2021 20:04:37 -
> @@ -48,7 +48,6 @@ static QE qlist;
> static int catone, cattwo, tflag;
> static u_int qsize;
>
> -char *appdstr(char *, const char *, size_t);
> void downcase(char *);
> void get_cats(char *, char *);
> void get_file(const char *);
> @@ -110,7 +109,8 @@ get_file(const char *file)
> {
> FILE *fp;
> QE *qp;
> - size_t len;
> + ssize_t len;
> + size_t qlen, size;
> char *lp;
>
> if ((fp = fopen(file, "r")) == NULL)
> @@ -123,26 +123,36 @@ get_file(const char *file)
>*/
> qp = &qlist;
> qsize = 0;
> - while ((lp = fgetln(fp, &len)) != NULL) {
> + lp = NULL;
> + size = 0;
> + while ((len = getline(&lp, &size, fp)) != -1) {
> if (lp[len - 1] == '\n')
> - --len;
> + lp[--len] = '\0';
> + if (qp->q_text)
> + qlen = strlen(qp->q_text);
> if (qp->q_text && qp->q_text[0] != '\0' &&
This would now be clearer as:
if (qlen > 0 && qp->q_text[qlen - 1] == '\\') {
> - qp->q_text[strlen(qp->q_text) - 1] == '\\')
> - qp->q_text = appdstr(qp->q_text, lp, len);
> - else {
> + qp->q_text[qlen - 1] == '\\') {
> + qp->q_text[--qlen] = '\0';
> + qlen += len;
> + qp->q_text = realloc(qp->q_text, qlen + 1);
> + if (qp->q_text == NULL)
> + errx(1, "realloc");
> + strlcat(qp->q_text, lp, qlen + 1);
> + } else {
> if ((qp->q_next = malloc(sizeof(QE))) == NULL)
> errx(1, "malloc");
> qp = qp->q_next;
> - if ((qp->q_text = malloc(len + 1)) == NULL)
> - errx(1, "malloc");
> - /* lp may not be zero-terminated; cannot use strlcpy */
> - strncpy(qp->q_text, lp, len);
> - qp->q_text[len] = '\0';
> + qp->q_text = strdup(lp);
> + if (qp->q_text == NULL)
> + errx(1, "strdup");
> qp->q_asked = qp->q_answered = FALSE;
> qp->q_next = NULL;
> ++qsize;
> }
> }
> + free(lp);
> + if (ferror(fp))
> + err(1, "getline");
> (void)fclose(fp);
> }
>
> @@ -316,32 +326,6 @@ next_cat(const char *s)
> esc = 0;
> break;
> }
> -}
> -
> -char *
> -appdstr(char *s, const char *tp, size_t len)
> -{
> - char *mp;
> - const char *sp;
> - int ch;
> - char *m;
> -
> - if ((m = malloc(strlen(s) + len + 1)) == NULL)
> - errx(1, "malloc");
> - for (mp = m, sp = s; (*mp++ = *sp++) != '\0'; )
> - ;
> - --mp;
> - if (*(mp - 1) == '\\')
> - --mp;
> -
> - while ((ch = *mp++ = *tp++) && ch != '\n')
> - ;
> - if (*(mp - 2) == '\\')
> - mp--;
> - *mp = '\0';
> -
> - free(s);
> - return (m);
> }
>
> void
> --
> Christian "naddy" Weisgerber na...@mips.inka.de
Re: quiz: Fix multi-line questions (trailing newline)
Todd C. Miller:
> I don't think your use of qlen is safe since it is initialized
> to zero. Specifically, it looks like "qp->q_text[qlen - 1]"
> would be an out of bounds read. Should qlen be initialized
> to strlen(qp->q_text) if qp->q_text != NULL?
Right. Next iteration:
Index: quiz.c
===
RCS file: /cvs/src/games/quiz/quiz.c,v
retrieving revision 1.30
diff -u -p -r1.30 quiz.c
--- quiz.c 24 Aug 2018 11:14:49 - 1.30
+++ quiz.c 10 Mar 2021 20:04:37 -
@@ -48,7 +48,6 @@ static QE qlist;
static int catone, cattwo, tflag;
static u_int qsize;
-char *appdstr(char *, const char *, size_t);
voiddowncase(char *);
voidget_cats(char *, char *);
voidget_file(const char *);
@@ -110,7 +109,8 @@ get_file(const char *file)
{
FILE *fp;
QE *qp;
- size_t len;
+ ssize_t len;
+ size_t qlen, size;
char *lp;
if ((fp = fopen(file, "r")) == NULL)
@@ -123,26 +123,36 @@ get_file(const char *file)
*/
qp = &qlist;
qsize = 0;
- while ((lp = fgetln(fp, &len)) != NULL) {
+ lp = NULL;
+ size = 0;
+ while ((len = getline(&lp, &size, fp)) != -1) {
if (lp[len - 1] == '\n')
- --len;
+ lp[--len] = '\0';
+ if (qp->q_text)
+ qlen = strlen(qp->q_text);
if (qp->q_text && qp->q_text[0] != '\0' &&
- qp->q_text[strlen(qp->q_text) - 1] == '\\')
- qp->q_text = appdstr(qp->q_text, lp, len);
- else {
+ qp->q_text[qlen - 1] == '\\') {
+ qp->q_text[--qlen] = '\0';
+ qlen += len;
+ qp->q_text = realloc(qp->q_text, qlen + 1);
+ if (qp->q_text == NULL)
+ errx(1, "realloc");
+ strlcat(qp->q_text, lp, qlen + 1);
+ } else {
if ((qp->q_next = malloc(sizeof(QE))) == NULL)
errx(1, "malloc");
qp = qp->q_next;
- if ((qp->q_text = malloc(len + 1)) == NULL)
- errx(1, "malloc");
- /* lp may not be zero-terminated; cannot use strlcpy */
- strncpy(qp->q_text, lp, len);
- qp->q_text[len] = '\0';
+ qp->q_text = strdup(lp);
+ if (qp->q_text == NULL)
+ errx(1, "strdup");
qp->q_asked = qp->q_answered = FALSE;
qp->q_next = NULL;
++qsize;
}
}
+ free(lp);
+ if (ferror(fp))
+ err(1, "getline");
(void)fclose(fp);
}
@@ -316,32 +326,6 @@ next_cat(const char *s)
esc = 0;
break;
}
-}
-
-char *
-appdstr(char *s, const char *tp, size_t len)
-{
- char *mp;
- const char *sp;
- int ch;
- char *m;
-
- if ((m = malloc(strlen(s) + len + 1)) == NULL)
- errx(1, "malloc");
- for (mp = m, sp = s; (*mp++ = *sp++) != '\0'; )
- ;
- --mp;
- if (*(mp - 1) == '\\')
- --mp;
-
- while ((ch = *mp++ = *tp++) && ch != '\n')
- ;
- if (*(mp - 2) == '\\')
- mp--;
- *mp = '\0';
-
- free(s);
- return (m);
}
void
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: quiz: Fix multi-line questions (trailing newline)
On Tue, 09 Mar 2021 22:04:42 +0100, Christian Weisgerber wrote: > Thanks a lot for figuring this out! I finally got around to looking > at your patch. Once we have nul-terminated lines, appdstr() can > be replaced with realloc() and strlcat(). I don't think your use of qlen is safe since it is initialized to zero. Specifically, it looks like "qp->q_text[qlen - 1]" would be an out of bounds read. Should qlen be initialized to strlen(qp->q_text) if qp->q_text != NULL? - todd
Re: quiz: Fix multi-line questions (trailing newline)
Alex Karle:
> Looking deeper, there is a bug in quiz(6) for datfiles with multi-line
> answers.
>
> Specifically, the following quiz.db line:
>
> foo:\
> bar
>
> Is parsed into "foo:bar\n", which made it impossible to get right (since
> the comparison was expecting a newline). This is a result of a bug in
> quiz.c's appdstr(qp->q_text, lp, len), which copies the trailing newline
> after "bar" (in lp) into q_text.
[...]
> That said, it seemed cleaner to modify the code to use getline(3) over
> fgetln(3), which guarantees each iteration of the loop is a single line
> and null-terminated, enabling the newline truncation in the loop and
> allowing the use of strlcpy(3). This patch implements that fix.
>
> One can verify the bug(fix) by:
>
> $ cat < /tmp/qtest
> foo1:bar
> foo2:\\
> bar
> foo3:\\
> ba\\
> r
> EOF
> $ echo "/tmp/qtest:test:lines" > /tmp/qindex
>
> # answer 'bar' to each question
> $ quiz -i /tmp/qindex test lines # fails
> $ /usr/obj/games/quiz/quiz -i /tmp/qindex test lines # succeeds
Thanks a lot for figuring this out! I finally got around to looking
at your patch. Once we have nul-terminated lines, appdstr() can
be replaced with realloc() and strlcat().
This could use another pair of eyes.
OK?
Index: quiz.c
===
RCS file: /cvs/src/games/quiz/quiz.c,v
retrieving revision 1.30
diff -u -p -r1.30 quiz.c
--- quiz.c 24 Aug 2018 11:14:49 - 1.30
+++ quiz.c 9 Mar 2021 20:38:20 -
@@ -48,7 +48,6 @@ static QE qlist;
static int catone, cattwo, tflag;
static u_int qsize;
-char *appdstr(char *, const char *, size_t);
voiddowncase(char *);
voidget_cats(char *, char *);
voidget_file(const char *);
@@ -110,7 +109,8 @@ get_file(const char *file)
{
FILE *fp;
QE *qp;
- size_t len;
+ ssize_t len;
+ size_t qlen, size;
char *lp;
if ((fp = fopen(file, "r")) == NULL)
@@ -123,26 +123,35 @@ get_file(const char *file)
*/
qp = &qlist;
qsize = 0;
- while ((lp = fgetln(fp, &len)) != NULL) {
+ qlen = 0;
+ lp = NULL;
+ size = 0;
+ while ((len = getline(&lp, &size, fp)) != -1) {
if (lp[len - 1] == '\n')
- --len;
+ lp[--len] = '\0';
if (qp->q_text && qp->q_text[0] != '\0' &&
- qp->q_text[strlen(qp->q_text) - 1] == '\\')
- qp->q_text = appdstr(qp->q_text, lp, len);
- else {
+ qp->q_text[qlen - 1] == '\\') {
+ qp->q_text[--qlen] = '\0';
+ qp->q_text = realloc(qp->q_text, qlen + len + 1);
+ if (qp->q_text == NULL)
+ errx(1, "realloc");
+ qlen = strlcat(qp->q_text, lp, qlen + len + 1);
+ } else {
if ((qp->q_next = malloc(sizeof(QE))) == NULL)
errx(1, "malloc");
qp = qp->q_next;
- if ((qp->q_text = malloc(len + 1)) == NULL)
- errx(1, "malloc");
- /* lp may not be zero-terminated; cannot use strlcpy */
- strncpy(qp->q_text, lp, len);
- qp->q_text[len] = '\0';
+ qp->q_text = strdup(lp);
+ if (qp->q_text == NULL)
+ errx(1, "strdup");
+ qlen = strlen(qp->q_text);
qp->q_asked = qp->q_answered = FALSE;
qp->q_next = NULL;
++qsize;
}
}
+ free(lp);
+ if (ferror(fp))
+ err(1, "getline");
(void)fclose(fp);
}
@@ -316,32 +325,6 @@ next_cat(const char *s)
esc = 0;
break;
}
-}
-
-char *
-appdstr(char *s, const char *tp, size_t len)
-{
- char *mp;
- const char *sp;
- int ch;
- char *m;
-
- if ((m = malloc(strlen(s) + len + 1)) == NULL)
- errx(1, "malloc");
- for (mp = m, sp = s; (*mp++ = *sp++) != '\0'; )
- ;
- --mp;
- if (*(mp - 1) == '\\')
- --mp;
-
- while ((ch = *mp++ = *tp++) && ch != '\n')
- ;
- if (*(mp - 2) == '\\')
- mp--;
- *mp = '\0';
-
- free(s);
- return (m);
}
void
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: quiz: Fix multi-line questions (trailing newline)
Hi all,
v2 below following some feedback from Christian (naddy@, thanks!). Only
change is the malloc + strlcpy is replaced with a strdup.
Thanks,
Alex
diff --git games/quiz/quiz.c games/quiz/quiz.c
index 073c1700719..f6eac5027e8 100644
--- games/quiz/quiz.c
+++ games/quiz/quiz.c
@@ -110,7 +110,8 @@ get_file(const char *file)
{
FILE *fp;
QE *qp;
- size_t len;
+ ssize_t len;
+ size_t size;
char *lp;
if ((fp = fopen(file, "r")) == NULL)
@@ -123,9 +124,11 @@ get_file(const char *file)
*/
qp = &qlist;
qsize = 0;
- while ((lp = fgetln(fp, &len)) != NULL) {
+ lp = NULL;
+ size = 0;
+ while ((len = getline(&lp, &size, fp)) != -1) {
if (lp[len - 1] == '\n')
- --len;
+ lp[len - 1] = '\0';
if (qp->q_text && qp->q_text[0] != '\0' &&
qp->q_text[strlen(qp->q_text) - 1] == '\\')
qp->q_text = appdstr(qp->q_text, lp, len);
@@ -133,16 +136,17 @@ get_file(const char *file)
if ((qp->q_next = malloc(sizeof(QE))) == NULL)
errx(1, "malloc");
qp = qp->q_next;
- if ((qp->q_text = malloc(len + 1)) == NULL)
- errx(1, "malloc");
- /* lp may not be zero-terminated; cannot use strlcpy */
- strncpy(qp->q_text, lp, len);
- qp->q_text[len] = '\0';
+ qp->q_text = strdup(lp);
+ if (qp->q_text == NULL)
+ err(1, NULL);
qp->q_asked = qp->q_answered = FALSE;
qp->q_next = NULL;
++qsize;
}
}
+ free(lp);
+ if (ferror(fp))
+ err(1, "getline");
(void)fclose(fp);
}
@@ -334,11 +338,9 @@ appdstr(char *s, const char *tp, size_t len)
if (*(mp - 1) == '\\')
--mp;
- while ((ch = *mp++ = *tp++) && ch != '\n')
+ /* tp guaranteed null-terminated, copy in full */
+ while ((ch = *mp++ = *tp++) != '\0')
;
- if (*(mp - 2) == '\\')
- mp--;
- *mp = '\0';
free(s);
return (m);
quiz: Fix multi-line questions (trailing newline)
Hi all,
I was thrilled to find that the quiz for ed(1) commands, `function
ed-command` (as referenced in The UNIX Programming Enviroment), existed
in OpenBSD, but found that there was a bug where ~half of my correct
answers were not accepted.
Looking deeper, there is a bug in quiz(6) for datfiles with multi-line
answers.
Specifically, the following quiz.db line:
foo:\
bar
Is parsed into "foo:bar\n", which made it impossible to get right (since
the comparison was expecting a newline). This is a result of a bug in
quiz.c's appdstr(qp->q_text, lp, len), which copies the trailing newline
after "bar" (in lp) into q_text.
In appdstr, there was logic to strip the newline in the case of multiple
backslashes:
if (*(mp - 2) == '\\')
mp--;
*mp = '\0';
For example (foo:\\\nbar\\\nbaz\n) would have the second newline stripped
when appending bar to foo; since bar\\\n has a \\ before the newline, the
above code would roll back mp and truncate the \n. However, when baz is
appended, the check fails to find a \\ and the \n is kept, resulting in
"foo:barbaz\n".
The simplest fix for the bug is to modify this check to strip the newline
unconditionally:
if (*(mp - 1) == '\n')
mp--;
*mp = '\0';
That said, it seemed cleaner to modify the code to use getline(3) over
fgetln(3), which guarantees each iteration of the loop is a single line
and null-terminated, enabling the newline truncation in the loop and
allowing the use of strlcpy(3). This patch implements that fix.
One can verify the bug(fix) by:
$ cat < /tmp/qtest
foo1:bar
foo2:\\
bar
foo3:\\
ba\\
r
EOF
$ echo "/tmp/qtest:test:lines" > /tmp/qindex
# answer 'bar' to each question
$ quiz -i /tmp/qindex test lines # fails
$ /usr/obj/games/quiz/quiz -i /tmp/qindex test lines # succeeds
Any and all feedback welcome!
Thanks,
Alex
diff --git games/quiz/quiz.c games/quiz/quiz.c
index 073c1700719..c70dabe617a 100644
--- games/quiz/quiz.c
+++ games/quiz/quiz.c
@@ -110,7 +110,8 @@ get_file(const char *file)
{
FILE *fp;
QE *qp;
- size_t len;
+ ssize_t len;
+ size_t size;
char *lp;
if ((fp = fopen(file, "r")) == NULL)
@@ -123,9 +124,11 @@ get_file(const char *file)
*/
qp = &qlist;
qsize = 0;
- while ((lp = fgetln(fp, &len)) != NULL) {
+ lp = NULL;
+ size = 0;
+ while ((len = getline(&lp, &size, fp)) != -1) {
if (lp[len - 1] == '\n')
- --len;
+ lp[len - 1] = '\0';
if (qp->q_text && qp->q_text[0] != '\0' &&
qp->q_text[strlen(qp->q_text) - 1] == '\\')
qp->q_text = appdstr(qp->q_text, lp, len);
@@ -135,14 +138,15 @@ get_file(const char *file)
qp = qp->q_next;
if ((qp->q_text = malloc(len + 1)) == NULL)
errx(1, "malloc");
- /* lp may not be zero-terminated; cannot use strlcpy */
- strncpy(qp->q_text, lp, len);
- qp->q_text[len] = '\0';
+ strlcpy(qp->q_text, lp, len + 1);
qp->q_asked = qp->q_answered = FALSE;
qp->q_next = NULL;
++qsize;
}
}
+ free(lp);
+ if (ferror(fp))
+ err(1, "getline");
(void)fclose(fp);
}
@@ -334,11 +338,9 @@ appdstr(char *s, const char *tp, size_t len)
if (*(mp - 1) == '\\')
--mp;
- while ((ch = *mp++ = *tp++) && ch != '\n')
+ /* tp guaranteed null-terminated, copy in full */
+ while ((ch = *mp++ = *tp++) != '\0')
;
- if (*(mp - 2) == '\\')
- mp--;
- *mp = '\0';
free(s);
return (m);
