- 2010.02.26 14:30 "Re: [Tiff] Patch for tif_unix.c", by Edward Lam
-
2010.02.26 17:05 "Re: [Tiff] Patch for tif_unix.c", by Bob Friesenhahn
- 2010.02.26 17:43 "Re: [Tiff] Patch for tif_unix.c", by Edward Lam
-
2010.03.02 08:14 "Re: [Tiff] Patch for tif_unix.c", by Eric Doenges
- 2010.03.02 14:11 "Re: [Tiff] Patch for tif_unix.c", by Toby Thain
- 2010.03.02 14:21 "Re: [Tiff] Patch for tif_unix.c", by Edward Lam
2010.03.02 08:14 "Re: [Tiff] Patch for tif_unix.c", by Eric Doenges
Hi all,
thanks for the feedback. I've reworked the patch to declare 'bytes' as a ssize_t instead of size_t (this is actually quite an embarrassing error, as the compiler helpfully warns that it's not going to work as a size_t).
There is some code which looks very much like this in GraphicsMagick
but with the primary intention to deal with system calls which are
interrupted by a signal. While the approach used in GraphicsMagick
would deal with the short read/write issue, I had never considered
that a system would have an issue with the size of the data passed.
Writing truely portable I/O code is a source of endless joy =8^(
The similar code in GraphicsMagick is in the FilePositionRead() and
FilePositionWrite() functions viewable on the web at "http://cvs.graphicsmagick.org/cgi-bin/cvsweb.cgi/GraphicsMagick/magick/pixel_cache.c?rev=1.67;content-type=text%2Fx-cvsweb-markup".
IMO that code isn't right either. Unless you can guarantee that 'length' will always be < SSIZE_MAX, you need to check for it. And if the call is interrupted before any data could be read or written, the function will return with -1 and errno=EINTR and not read or write all data as the comments claim.
Which leads back to two questions about my patch:
- Can I simply assume that SSIZE_MAX will be defined in <limits.h>, and that <limits.h> will be available for all platforms that matter to libtiff ?
- What is the correct response to receiving EINTR (i.e. being interrupted before any data is read/written) ? Should the IO operation simply be retried in _tiffReadProc/_tiffWriteProc, or should the caller handle it ?
It does not seem like your patch deals with the case where the system
call returns -1 to indicate an error. This is important to deal with
properly since otherwise the software could lock up. Can you fix this
and re-submit your patch? While it is useful to post to this list,
always remember to open a bug report on libtiff Bugzilla and post your
patch there as well.
Unfortunately, Bugzilla isn't working for me. Following the link given on the libtiff homepage (http://bugzilla.remotesensing.org/enter_bug.cgi?product=libtiff) gives me a 404:
--- SNIP ---
The requested URL /enter_bug.cgi was not found on this server.
Additionally, a 404 Not Found error was encountered while trying to use an ErrorDocument to handle the request.
--- SNIP ---
With kind regards,
Eric Dönges
--
Dr. Eric Dönges doenges@mvtec.com
MVTec Software GmbH | Neherstr. 1 | 81675 München | Germany
www.mvtec.com | Tel: +49 89 457695-0 | Fax: +49 89 457695-55
Geschäftsführer: Dr. Wolfgang Eckstein, Dr. Olaf Munkelt Amtsgericht München HRB 114695
Index: libtiff/tif_unix.c
=================================================================== RCS file: /cvs/maptools/cvsroot/libtiff/libtiff/tif_unix.c,v retrieving revision 1.21
diff -u -r1.21 tif_unix.c
--- libtiff/tif_unix.c 18 Jul 2007 13:46:41 -0000 1.21
+++ libtiff/tif_unix.c 2 Mar 2010 08:00:58 -0000
@@ -39,6 +39,7 @@
#include <stdarg.h>
#include <stdlib.h>
+#include <limits.h>
#include <sys/stat.h>
#ifdef HAVE_UNISTD_H
@@ -58,25 +59,53 @@
static tmsize_t
_tiffReadProc(thandle_t fd, void* buf, tmsize_t size)
{
- size_t size_io = (size_t) size;
- if ((tmsize_t) size_io != size)
- {
- errno=EINVAL;
- return (tmsize_t) -1;
- }
- return ((tmsize_t) read((int) fd, buf, size_io));
+ ssize_t bytes;
+ size_t total, request, size_io = (size_t) size;
+ if ((tmsize_t) size_io != size)
+ {
+ errno=EINVAL;
+ return (tmsize_t) -1;
+ }
+
+ for (total = 0; total < size_io; total += bytes)
+ {
+ request = size_io - total;
+ if (request > SSIZE_MAX)
+ request = SSIZE_MAX;
+
+ bytes = read((int) fd, (char *) buf + total, request);
+ if (0 > bytes)
+ return (tmsize_t) -1;
+ else if (0 == bytes)
+ break;
+ }
+ return (tmsize_t) total;
}
static tmsize_t
_tiffWriteProc(thandle_t fd, void* buf, tmsize_t size)
{
- size_t size_io = (size_t) size;
- if ((tmsize_t) size_io != size)
- {
- errno=EINVAL;
- return (tmsize_t) -1;
- }
- return ((tmsize_t) write((int) fd, buf, size_io));
+ ssize_t bytes;
+ size_t total, request, size_io = (size_t) size;
+ if ((tmsize_t) size_io != size)
+ {
+ errno=EINVAL;
+ return (tmsize_t) -1;
+ }
+
+ for (total = 0; total < size_io; total += bytes)
+ {
+ request = size_io - total;
+ if (request > SSIZE_MAX)
+ request = SSIZE_MAX;
+
+ bytes = write((int) fd, (char *) buf + total, request);
+ if (0 > bytes)
+ return (tmsize_t)-1;
+ else if (0 == bytes)
+ break;
+ }
+ return (tmsize_t) total;
}
static uint64