http://www.freebsd.org/cgi/query-pr.cgi?pr=154407 To: FreeBSD-gnats-submit@@@freebsd.org From: "Julian H. Stacey" cc: "Julian H. Stacey" , "Tim Kientzle" , "Marc Recht (for NetBSD)" , "Gary Jennejohn (Home)" Reply-To: "Julian H. Stacey" Subject: usr.bin/tar/ ignores error codes from read() just silently pads nulls X-send-pr-version: 3.113 X-GNATS-Notify: >Submitter-Id: current-users >Originator: "Julian H. Stacey" >Organization: http://www.berklix.com BSD Linux Unix Consultancy, Munich/Muenchen. >Confidential: no >Synopsis: usr.bin/tar/ ignores error codes from read() just silently pads nulls >Severity: serious >Priority: high >Category: bin >Class: change-request >Release: FreeBSD 8.1-RELEASE amd64 >Environment: System: FreeBSD blak.js.berklix.net 8.1-RELEASE FreeBSD 8.1-RELEASE #2: Fri Aug 20 15:11:19 CEST 2010 jhs@@@blak.js.berklix.net:/usr/src/sys/amd64/compile/BLAK.small amd64 >Description: usr.bin/tar/ ignores error codes from read() silently pads with nulls, corrupting copied data without warning ! Log of how I discovered problem + diffs + copy of send-pr: http://www.berklix.com/~jhs/src/bsd/fixes/FreeBSD/src/gen/usr.bin/tar/ (various files have .no_customise extension to avoid my customise script). Comparison with gtar: gtar also delivers false nulls unfortunately, gtar warns users it is getting device errors. Comparison with cp: cp warns of errors, cp delivers no false nulls, truncates at read fail. >How-To-Repeat: - Get a normal data CD or DVD you have created ) (not commercial recorded hollywood deliberately sector crippled media), - Damage the media so it will have some read errors, mount /dev/acd0 /cdrom cd /tmp mkdir -p cp tar/cdrom cp -R /cdrom cp (cd /cdrom ; tar cf - . ) | ( cd tar/cdrom ; tar xf - ) cd tar/cdrom # http://www.berklix.com/~jhs/src/bsd/jhs/bin/public/cmpd/ find . -type f -exex cmpd -d {} ../../cp/cdrom \; # http://www.berklix.com/~jhs/src/bsd/jhs/bin/public/8f/ foreach i ( `find . -type f | xargs 8f -n 2048 -b 0 -l` ) echo $i xargs od -c $i | yail end >Fix: My patches fix code to avoid unwarned damaged data. As it's Tim K.'s recently written code, I presume he will want to fix it himself, so my patch lines as well as fixing, are also appended with edit mark // JHS for easy searching. write.c & util.c were bad in 8.1-RELEASE (& the critical bit in ./-current/src/usr/bin/write.c ^write_file_data() the final bytes_read = read(fd, bsdtar->buff, FILEDATABUFLEN); is also bad) Various return values silently ignored. All invocations of [f]read() & [f]write() & all invocations of *_read_* & *_write_* etc should be checked whether return value is properly used. Later fixes if any will be in http://www.berklix.com/~jhs/src/bsd/fixes/FreeBSD/src/gen/usr.bin/tar/ A copy appended. *** 8.1-RELEASE-generic/src/usr.bin/tar/write.c Mon Jun 14 04:09:06 2010 --- 8.1-RELEASE+jhs-error-detect/src/usr.bin/tar/write.c Sat Jan 29 18:34:55 2011 *************** *** 217,222 **** --- 217,224 ---- if (ARCHIVE_OK != archive_write_open_file(a, bsdtar->filename)) bsdtar_errc(bsdtar, 1, 0, archive_error_string(a)); write_archive(a, bsdtar); + // JHS write_archive is declared void, + // JHS is there a static extern ret val to check ? } /* *************** *** 301,312 **** archive_write_set_format(a, format); } lseek(bsdtar->fd, end_offset, SEEK_SET); /* XXX check return val XXX */ if (ARCHIVE_OK != archive_write_set_options(a, bsdtar->option_options)) bsdtar_errc(bsdtar, 1, 0, archive_error_string(a)); if (ARCHIVE_OK != archive_write_open_fd(a, bsdtar->fd)) bsdtar_errc(bsdtar, 1, 0, archive_error_string(a)); ! write_archive(a, bsdtar); /* XXX check return val XXX */ close(bsdtar->fd); bsdtar->fd = -1; --- 303,316 ---- archive_write_set_format(a, format); } lseek(bsdtar->fd, end_offset, SEEK_SET); /* XXX check return val XXX */ + // JHS if (ARCHIVE_OK != archive_write_set_options(a, bsdtar->option_options)) bsdtar_errc(bsdtar, 1, 0, archive_error_string(a)); if (ARCHIVE_OK != archive_write_open_fd(a, bsdtar->fd)) bsdtar_errc(bsdtar, 1, 0, archive_error_string(a)); ! write_archive(a, bsdtar); /* XXX check return val XXX */ ! // JHS what return val ? write_archive is declared void close(bsdtar->fd); bsdtar->fd = -1; *************** *** 390,395 **** --- 394,401 ---- bsdtar_errc(bsdtar, 1, 0, archive_error_string(a)); write_archive(a, bsdtar); + // JHS write_archive is declared void, + // JHS is there a static extern ret val to check ? close(bsdtar->fd); bsdtar->fd = -1; *************** *** 926,931 **** --- 932,945 ---- off_t progress = 0; bytes_read = read(fd, bsdtar->buff, FILEDATABUFLEN); + if (bytes_read < 0) { // JHS + perror(NULL) ; // JHS + fprintf(stderr,"File: %s, Line %d, Ret %d\n", // JHS + __FILE__, __LINE__, (int)bytes_read ); // JHS + return(-1) ; // JHS + // I've not checked to see if caller // JHS + // appropriately detects & deals with -1 // JHS + } // JHS while (bytes_read > 0) { siginfo_printinfo(bsdtar, progress); *************** *** 945,951 **** } progress += bytes_written; bytes_read = read(fd, bsdtar->buff, FILEDATABUFLEN); ! } return 0; } --- 959,995 ---- } progress += bytes_written; bytes_read = read(fd, bsdtar->buff, FILEDATABUFLEN); ! if (bytes_read < 0) // JHS ! { // JHS ! perror(NULL) ; // JHS ! fprintf(stderr,"Read error.\n"); // JHS ! // fprintf(stderr,"Read error on %s.\n", // JHS ! // Please pass in name as parameter to print. // JHS ! // "" ); // JHS ! fprintf(stderr, // JHS ! "Output will contain false trailing nulls.\n"); // JHS ! // This prints when ! fprintf(stderr, // JHS ! "File: %s, Line %d, Ret %d\n", // JHS ! __FILE__, __LINE__, (int)bytes_read ); // JHS ! return(-1) ; // JHS ! // I've not read code to see if caller // JHS ! // appropriately detects & deals with -1 // JHS ! // But Ive tested it, with // JHS ! // tar cvf junk.tar aa bb, // JHS ! // & if aa has dev errors, // JHS ! // bb does not get archived. // JHS ! } // JHS ! } ! if (bytes_read < 0) // JHS ! { // JHS ! perror(NULL) ; // JHS ! fprintf(stderr,"File: %s, Line %d\n", // JHS ! __FILE__, __LINE__ ); // JHS ! return(-1) ; // JHS ! // I've not read code to see if caller // JHS ! // appropriately detects & deals with -1 // JHS ! } // JHS return 0; } *** 8.1-RELEASE-generic/src/usr.bin/tar/util.c Mon Jun 14 04:09:06 2010 --- 8.1-RELEASE+jhs-error-detect/src/usr.bin/tar/util.c Sat Jan 29 16:36:45 2011 *************** *** 234,239 **** --- 234,241 ---- exit(eval); } + /* Get confirmation from user to do something, eg add/ copy // JHS + Return: 1=yes, 0=no */ // JHS int yes(const char *fmt, ...) { *************** *** 249,254 **** --- 251,263 ---- fflush(stderr); l = read(2, buff, sizeof(buff) - 1); + if (l < 0) // JHS + { // JHS + perror(NULL) ; // JHS + fprintf(stderr,"Failed to read yes/no\n" ); // JHS + fprintf(stderr,"File: %s, Line %d, Ret %d\n", // JHS + __FILE__, __LINE__, (int)l ); // JHS + } // JHS if (l <= 0) return (0); buff[l] = 0; *************** *** 307,312 **** --- 316,333 ---- /* Get some more data into the buffer. */ bytes_wanted = buff + buff_length - buff_end; bytes_read = fread(buff_end, 1, bytes_wanted, f); + if ( // JHS + ( bytes_read < bytes_wanted ) // JHS + || // JHS + (bytes_read = 0) // JHS + ) // JHS + if ((bytes_read = 0) || ( bytes_read < bytes_wanted )) // JHS + { // JHS + perror(NULL) ; // JHS + fprintf(stderr, // JHS + "File: %s, Line %d, Ret %d\n", // JHS + __FILE__, __LINE__, (int)bytes_read ); // JHS + } // JHS buff_end += bytes_read; /* Process all complete lines in the buffer. */ while (line_end < buff_end) {