On Tue, Apr 8, 2014 at 9:46 AM, Stephen J. Turnbull <stephen(a)xemacs.org> wrote:
Sorry it took so long to get back to this.
I wonder if this crash accounts for Henry Thompson's report, too?
Jerry James writes:
> We need to notice the Lstream_read failure and bail out of
> determine_real_coding_system.
> --- a/src/file-coding.c Thu Mar 27 08:59:03 2014 -0600
> +++ b/src/file-coding.c Thu Mar 27 09:32:53 2014 -0600
In encode_decode_coding_region:
> @@ -2294,7 +2294,7 @@
> Bytecount size_in_bytes =
> Lstream_read (istr, tempbuf, sizeof (tempbuf));
>
> - if (!size_in_bytes)
> + if (size_in_bytes <= 0)
> break;
> newpos = lisp_buffer_stream_startpos (istr);
> Lstream_write (ostr, tempbuf, size_in_bytes);
I don't think this is sufficient. The calling code is expecting the
Lstream_write to succeed. If it fails due to an error, that error
should be signaled, because the user's data may be corrupt. I don't
think that the buffer is corrupt in the sense that there are bytes
that aren't in Mule internal encoding, but I'm not entirely sure of
that, and don't have time to make sure right now.
> @@ -3863,24 +3863,32 @@
> make_opaque_ptr (st));
> UExtbyte buf[4096];
> Bytecount nread = Lstream_read (stream, buf, sizeof (buf));
> - Lisp_Object coding_system
> - = look_for_coding_system_magic_cookie (buf, nread, 1);
> -
> - if (NILP (coding_system))
> + Lisp_Object coding_system;
> +
> + if (nread > 0)
> {
> - while (1)
> + coding_system = look_for_coding_system_magic_cookie (buf, nread, 1);
> +
> + if (NILP (coding_system))
> {
> - if (detect_coding_type (st, buf, nread))
> - break;
> - nread = Lstream_read (stream, buf, sizeof (buf));
> - if (nread == 0)
> - break;
> + while (1)
> + {
> + if (detect_coding_type (st, buf, nread))
> + break;
> + nread = Lstream_read (stream, buf, sizeof (buf));
> + if (nread <= 0)
> + break;
I worry about this simple change from "nread == 0" to "nread <=
0".
If there is a read error, shouldn't the user be told about it?
I suspect that a read error here is an XEmacs bug elsewhere.
Specifically, I think that the coding system should catch and report
the error (again, I need to check that).
> + }
> +
> + coding_system = detected_coding_system (st);
> }
>
> - coding_system = detected_coding_system (st);
> + Lstream_rewind (stream);
> }
> -
> - Lstream_rewind (stream);
> + else
> + {
> + coding_system = Qnil;
> + }
>
> unbind_to (depth);
> return coding_system;
> @@ -4315,7 +4323,9 @@
> Lstream_delete (XLSTREAM (lstream));
> retry_close (fd);
>
> - return look_for_coding_system_magic_cookie (buf, nread, 0);
> + return (nread > 0)
> + ? look_for_coding_system_magic_cookie (buf, nread, 0)
> +: Qnil;
> }
>
I'm almost tempted to suggest putting the checking logic in
look_for_coding_system_magic_cookie.
Here's another attempt at addressing the bug (or at least giving us a
way to get more information about the bug), this time signaling an
error if something goes wrong with reading from an lstream. The error
message in detect_coding_type() does not say it is an internal error,
because we can wind up there if the very first read() on the file's
file descriptor results in, for example, EIO. (Control reaches that
point if read() returns -1 for any reason.) It would be nice if we
could get the value of errno from that read() attempt somehow.
Actually, we could. In filedesc_reader() in lstream.c, when either
read_allowing_quit() or retry_read() returns -1, errno is still set to
the value it got inside read(). So we could stash errno away
somewhere in the lines that say:
if (nread < 0)
return LSTREAM_ERROR;
Would anybody object to adding a saved_errno field to struct
filedesc_stream? That way we could report the errno back to the user
in these cases, which would help us understand what is going wrong.
diff -r 8ef8d5e7c920 src/ChangeLog
--- a/src/ChangeLog Fri Mar 28 12:48:12 2014 -0600
+++ b/src/ChangeLog Thu Apr 24 11:19:23 2014 -0600
@@ -1,3 +1,10 @@
+2014-03-27 Jerry James <james(a)xemacs.org>
+
+ * file-coding.c (encode_decode_coding_region): Signal an error if
+ Lstream_read encounters an error (returns -1).
+ (detect_coding_type): Ditto.
+ (look_for_coding_system_magic_cookie): Ditto.
+
2014-01-27 Michael Sperber <mike(a)xemacs.org>
* symbols.c (Fdefine_function): Allow optional `docstring'
diff -r 8ef8d5e7c920 src/file-coding.c
--- a/src/file-coding.c Fri Mar 28 12:48:12 2014 -0600
+++ b/src/file-coding.c Thu Apr 24 11:19:23 2014 -0600
@@ -2294,6 +2294,12 @@
Bytecount size_in_bytes =
Lstream_read (istr, tempbuf, sizeof (tempbuf));
+ if (size_in_bytes < 0)
+ signal_error (Qtext_conversion_error,
+ direction == CODING_DECODE
+ ? "Internal error while decoding"
+: "Internal error while encoding",
+ XCODING_SYSTEM_NAME (coding_system));
if (!size_in_bytes)
break;
newpos = lisp_buffer_stream_startpos (istr);
@@ -3569,6 +3575,10 @@
const UExtbyte *src2 = src;
int i;
+ if (n < 0)
+ signal_error (Qtext_conversion_error,
+ "Error reading file to determine coding system", Qnil);
+
#ifdef DEBUG_XEMACS
if (!NILP (Vdebug_coding_detection))
{
@@ -3790,6 +3800,10 @@
const UExtbyte *scan_end;
Bytecount cookie_len;
+ if (len < 0)
+ signal_error (Qtext_conversion_error,
+ "Internal error while looking for coding cookie", Qnil);
+
/* Look for initial "-*-"; mode line prefix */
for (p = data,
scan_end = data + len - LENGTH ("-*-coding:?-*-");
Regards,
--
Jerry James
http://www.jamezone.org/
_______________________________________________
XEmacs-Beta mailing list
XEmacs-Beta(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-beta