From 43e95dc4dbdd6949d69f1eaabedebd6b3c26d94a Mon Sep 17 00:00:00 2001 From: Alex Young Date: Thu, 21 Jun 2012 15:31:28 +0100 Subject: [PATCH] Make sure all the lines we read get freed (including the trailing blank) --- Rakefile | 20 +++---- src/ioutil.c | 29 +++++++--- tests/check_ioutil.c | 134 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 165 insertions(+), 18 deletions(-) create mode 100644 tests/check_ioutil.c diff --git a/Rakefile b/Rakefile index 41054f1..40ca0ba 100644 --- a/Rakefile +++ b/Rakefile @@ -14,9 +14,9 @@ TEST_OBJECTS = TEST_SOURCES.pathmap( "%{^tests,build/tests}X.o" ) LIBS = %w( pthread ) CCFLAGS = %w( -D_GNU_SOURCE=1 - -Wall - -Wextra - -Werror-implicit-function-declaration + -Wall + -Wextra + -Werror-implicit-function-declaration -Wstrict-prototypes -Wno-missing-field-initializers ) # Added -Wno-missing-field-initializers to shut GCC up over {0} struct initialisers @@ -93,7 +93,7 @@ rule 'build/flexnbd' => OBJECTS do |t| end -file check("client") => +file check("client") => %w{build/tests/check_client.o build/self_pipe.o build/nbdtypes.o @@ -101,9 +101,9 @@ file check("client") => build/readwrite.o build/parse.o build/client.o - build/serve.o + build/serve.o build/acl.o - build/ioutil.o + build/ioutil.o build/util.o} do |t| gcc_link t.name, t.prerequisites + [LIBCHECK] end @@ -112,7 +112,7 @@ file check("acl") => %w{build/tests/check_acl.o build/parse.o build/acl.o - build/util.o} do |t| + build/util.o} do |t| gcc_link t.name, t.prerequisites + [LIBCHECK] end @@ -124,9 +124,9 @@ file check("serve") => build/readwrite.o build/parse.o build/client.o - build/serve.o + build/serve.o build/acl.o - build/ioutil.o + build/ioutil.o build/util.o} do |t| gcc_link t.name, t.prerequisites + [LIBCHECK] end @@ -149,8 +149,8 @@ end (TEST_MODULES- %w{acl client serve readwrite}).each do |m| tgt = "build/tests/check_#{m}.o" - deps = ["build/ioutil.o", "build/util.o"] maybe_obj_name = "build/#{m}.o" + deps = ["build/ioutil.o", "build/util.o"] - [maybe_obj_name] deps << maybe_obj_name if OBJECTS.include?( maybe_obj_name ) diff --git a/src/ioutil.c b/src/ioutil.c index 89ad47c..75c944f 100644 --- a/src/ioutil.c +++ b/src/ioutil.c @@ -197,19 +197,27 @@ int splice_via_pipe_loop(int fd_in, int fd_out, size_t len) return spliced < len ? -1 : 0; } +/* Reads single bytes from fd until either an EOF or a newline appears. + * If an EOF occurs before a newline, returns -1. The line is lost. + * Inserts the read bytes (without the newline) into buf, followed by a + * trailing NULL. + * Returns the number of read bytes: the length of the line without the + * newline, plus the trailing null. + */ int read_until_newline(int fd, char* buf, int bufsize) { int cur; - bufsize -=1; for (cur=0; cur < bufsize; cur++) { int result = read(fd, buf+cur, 1); - if (result < 0) { return -1; } - if (buf[cur] == 10) { break; } + if (result <= 0) { return -1; } + if (buf[cur] == 10) { + buf[cur] = '\0'; + break; + } } - buf[cur++] = 0; - return cur; + return cur+1; } int read_lines_until_blankline(int fd, int max_line_length, char ***lines) @@ -221,9 +229,14 @@ int read_lines_until_blankline(int fd, int max_line_length, char ***lines) memset(line, 0, max_line_length+1); while (1) { - if (read_until_newline(fd, line, max_line_length) < 0) { - return lines_count; - } + int readden = read_until_newline(fd, line, max_line_length); + /* readden will be: + * 1 for an empty line + * -1 for an eof + * -1 for a read error + */ + if (readden <= 1) { return lines_count; } + fprintf(stderr, "Mallocing for %s\n", line ); *lines = xrealloc(*lines, (lines_count+1) * sizeof(char*)); (*lines)[lines_count] = strdup(line); if ((*lines)[lines_count][0] == 0) { diff --git a/tests/check_ioutil.c b/tests/check_ioutil.c new file mode 100644 index 0000000..b1f26a5 --- /dev/null +++ b/tests/check_ioutil.c @@ -0,0 +1,134 @@ +#include "ioutil.h" + +#include + +START_TEST( test_read_until_newline_returns_line_length_plus_null ) +{ + int fds[2]; + int nread; + char buf[5] = {0}; + pipe(fds); + + write( fds[1], "1234\n", 5 ); + + nread = read_until_newline( fds[0], buf, 5 ); + + ck_assert_int_eq( 5, nread ); +} +END_TEST + + +START_TEST( test_read_until_newline_inserts_null ) +{ + int fds[2]; + int nread; + char buf[5] = {0}; + pipe(fds); + + write( fds[1], "1234\n", 5 ); + + nread = read_until_newline( fds[0], buf, 5 ); + + ck_assert_int_eq( '\0', buf[4] ); + +} +END_TEST + + +START_TEST( test_read_empty_line_inserts_null ) +{ + int fds[2]; + int nread; + char buf[5] = {0}; + pipe(fds); + + write( fds[1], "\n", 1 ); + nread = read_until_newline( fds[0], buf, 1 ); + + ck_assert_int_eq( '\0', buf[0] ); + ck_assert_int_eq( 1, nread ); +} +END_TEST + + +START_TEST( test_read_eof_returns_err ) +{ + int fds[2]; + int nread; + char buf[5] = {0}; + pipe( fds ); + + close( fds[1] ); + nread = read_until_newline( fds[0], buf, 5 ); + + ck_assert_int_eq( -1, nread ); +} +END_TEST + + +START_TEST( test_read_eof_fills_line ) +{ + int fds[2]; + int nread; + char buf[5] = {0}; + pipe(fds); + + write( fds[1], "1234", 4 ); + close( fds[1] ); + nread = read_until_newline( fds[0], buf, 5 ); + + ck_assert_int_eq( -1, nread ); + ck_assert_int_eq( '4', buf[3] ); +} +END_TEST + + +START_TEST( test_read_lines_until_blankline ) +{ + char **lines = NULL; + int fds[2]; + int nlines; + pipe( fds ); + + write( fds[1], "a\nb\nc\n\n", 7 ); + + nlines = read_lines_until_blankline( fds[0], 256, &lines ); + + ck_assert_int_eq( 3, nlines ); +} +END_TEST + + +Suite *ioutil_suite(void) +{ + Suite *s = suite_create("ioutil"); + + TCase *tc_read_until_newline = tcase_create("read_until_newline"); + TCase *tc_read_lines_until_blankline = tcase_create("read_lines_until_blankline"); + + tcase_add_test(tc_read_until_newline, test_read_until_newline_returns_line_length_plus_null); + tcase_add_test(tc_read_until_newline, test_read_until_newline_inserts_null); + tcase_add_test(tc_read_until_newline, test_read_empty_line_inserts_null); + tcase_add_test(tc_read_until_newline, test_read_eof_returns_err); + tcase_add_test(tc_read_until_newline, test_read_eof_fills_line ); + + tcase_add_test(tc_read_lines_until_blankline, test_read_lines_until_blankline ); + + suite_add_tcase(s, tc_read_until_newline); + suite_add_tcase(s, tc_read_lines_until_blankline); + + return s; +} + +int main(void) +{ + int number_failed; + + Suite *s = ioutil_suite(); + SRunner *sr = srunner_create(s); + srunner_run_all(sr, CK_NORMAL); + number_failed = srunner_ntests_failed(sr); + srunner_free(sr); + return (number_failed == 0) ? 0 : 1; +} +