quiz: Fix multi-line questions (trailing newline)

[Alex Karle-2]

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 <<EOF > /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);

[Alex Karle-2]

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);