bin/154407: usr.bin/tar/ ignores error codes from read() just
silently pads nulls
Julian H. Stacey
jhs at berklix.com
Mon Jan 31 04:00:24 UTC 2011
>Number: 154407
>Category: bin
>Synopsis: usr.bin/tar/ ignores error codes from read() just silently pads nulls
>Confidential: no
>Severity: serious
>Priority: high
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: change-request
>Submitter-Id: current-users
>Arrival-Date: Mon Jan 31 04:00:21 UTC 2011
>Closed-Date:
>Last-Modified:
>Originator: "Julian H. Stacey" <jhs at berklix.com>
>Release: FreeBSD 8.1-RELEASE amd64
>Organization:
http://berklix.com BSD Linux Unix Consultancy, Munich/Muenchen.
>Environment:
System: FreeBSD blak.js.berklix.net 8.1-RELEASE FreeBSD 8.1-RELEASE #2: Fri Aug 20 15:11:19 CEST 2010 jhs at 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://berklix.com/~jhs/src/bsd/fixes/FreeBSD/src/gen/usr.bin/tar/
(various files have .ignore 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://berklix.com/~jhs/src/bsd/jhs/bin/public/cmpd/
find . -type f -exex cmpd -d {} ../../cp/cdrom \;
# http://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://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) {
>Release-Note:
>Audit-Trail:
>Unformatted:
More information about the freebsd-bugs
mailing list