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); |
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); |
Free forum by Nabble | Edit this page |