Re: Coding system conversion error (was Re: abort in x_handle_selection_event when copying text)

Re: Coding system conversion error (was Re: abort in x_handle_selection_event when copying text)

[Jan D.]

Hi.

I think I found the reason for this, but I am not sure.  Can anybody 
with knowledge about multibyte and unibyte string representations 
comment on the patch below (in function allocate_string_data in alloc.c)?

Thanks,

diff -c alloc.c.~1.363.~ alloc.c
Index: alloc.c
*** alloc.c.~1.363.~    2005-01-20 21:20:32.000000000 +0100
--- alloc.c    2005-02-08 22:46:02.000000000 +0100
***************
*** 1977,1983 ****
    SDATA_NBYTES (data) = nbytes;
  #endif
    s->size = nchars;
!   s->size_byte = nbytes;
    s->data[nbytes] = '\0';
  #ifdef GC_CHECK_STRING_OVERRUN
    bcopy (string_overrun_cookie, (char *) data + needed,
--- 1977,1983 ----
    SDATA_NBYTES (data) = nbytes;
  #endif
    s->size = nchars;
!   s->size_byte = nchars != nbytes ? nbytes : -1;
    s->data[nbytes] = '\0';
  #ifdef GC_CHECK_STRING_OVERRUN
    bcopy (string_overrun_cookie, (char *) data + needed,



Jan D. wrote:

> Reiner Steib wrote:
>
>> When doing this, another problem appeared.  I was copying the content
>> of the *Messages* buffer using M-w (`kill-ring-save') in the current
>> CVS Emacs.  Then I yanked (`C-y') it in the mail buffer of this Emacs
>> session (Emacs 21.3).
>>
>> At this moment, CVS Emacs aborted, see this backtrace:
>>  
>>
>
> This is easy for me to reproduce, start emacs-21.3, and emacs-cvs.
> In emacs-cvs:
> C-h i C-x h M-w
> In emacs-21.3, right click.
>
> Apparently it is not enought with a small set of text, it must be 
> large enough to trigger trigger this error (1025 characters, 1024 is ok).
>
> Emacs cvs crashes in the xassert in xselect.c, 
> lisp_data_to_selection_data:
>  else if (STRINGP (obj))
>    {
>      xassert (! STRING_MULTIBYTE (obj));
>      if (NILP (type))
>    type = QSTRING;
>      *format_ret = 8;
>      *size_ret = SBYTES (obj);
>
>
> The problem seems to be that xselect-convert-to-string in select.el 
> returns a multibyte string when coding is 
> compount-text-with-extensions in this call:
>
>       ((eq type 'COMPOUND_TEXT)
>        (setq str (encode-coding-string str coding)))
>
> Most applications except Emacs apparently don't request COMPOUND_TEXT, 
> for example UTF8_STRING works ok.
>
> I don't know enough of these coding systems, can anybody with insight 
> here help?
>
> Thanks,
>
>    Jan D.
>
>
>
>
>
> _______________________________________________
> Emacs-pretest-bug mailing list
> Emacs-pretest-bug@gnu.org
> http://lists.gnu.org/mailman/listinfo/emacs-pretest-bug

Re: Coding system conversion error

[Stefan Monnier]

> I think I found the reason for this, but I am not sure.  Can anybody with
> knowledge about multibyte and unibyte string representations comment on the
> patch below (in function allocate_string_data in alloc.c)?

> diff -c alloc.c.~1.363.~ alloc.c
> Index: alloc.c
> *** alloc.c.~1.363.~    2005-01-20 21:20:32.000000000 +0100
> --- alloc.c    2005-02-08 22:46:02.000000000 +0100
> ***************
> *** 1977,1983 ****
>     SDATA_NBYTES (data) = nbytes;
>   #endif
s-> size = nchars;
> !   s->size_byte = nbytes;
s-> data[nbytes] = '\0';
>   #ifdef GC_CHECK_STRING_OVERRUN
>     bcopy (string_overrun_cookie, (char *) data + needed,
> --- 1977,1983 ----
>     SDATA_NBYTES (data) = nbytes;
>   #endif
s-> size = nchars;
> !   s->size_byte = nchars != nbytes ? nbytes : -1;
s-> data[nbytes] = '\0';
>   #ifdef GC_CHECK_STRING_OVERRUN
>     bcopy (string_overrun_cookie, (char *) data + needed,


This doesn't look like the right fix.
Normally, the caller would instead use `STRING_SET_UNIBYTE' after the call
(or rather calls one of make_foo_string which does it for him) if needed.


        Stefan

Re: Coding system conversion error (was Re: abort in x_handle_selection_event when copying text)

[Richard Stallman]

It is possible for a multibyte string to have NBYTES = NCHARS.
The clean fix is for the args to allocate_string_data to indicate
explicitly whether the string is unibyte or multibyte.

This could be done with another argument, or by specifying -1
for NBYTES when the string is unibyte.

Re: Coding system conversion error

[Jan D.]

>
> This doesn't look like the right fix.
> Normally, the caller would instead use `STRING_SET_UNIBYTE' after the 
> call
> (or rather calls one of make_foo_string which does it for him) if 
> needed.

You are right, the problem is finding where this should be done :-)

	Jan D.

Re: Coding system conversion error

[Stefan Monnier]

>> This doesn't look like the right fix.
>> Normally, the caller would instead use `STRING_SET_UNIBYTE' after the call
>> (or rather calls one of make_foo_string which does it for him) if needed.

> You are right, the problem is finding where this should be done :-)

Well, maybe we can help, if you tell us what you know, ;-)


        Stefan

Re: Coding system conversion error

[Jan D.]

Stefan Monnier wrote:

> Well, maybe we can help, if you tell us what you know, ;-)


The mail you replied to was all I knew at the time.  But here is a 
distilled description of the problem (I've omitted the 1025 character 
string):

ELISP> (setq str (string-to-multibyte <1025 ASCII character string>))
...
ELISP> (multibyte-string-p str)
t
ELISP> (multibyte-string-p (encode-coding-string str
                                              
'compound-text-with-extensions))
t     <---- BUG, should be nil
ELISP> (multibyte-string-p (encode-coding-string str 'utf-8))
nil

Most applications don't ask for 'compound-text, so most of the time the 
xassert doesn't abort.


The compound-text case exits in the second return in 
encode_coding_string in coding.c in this code fragment (in the if (from 
== to_byte) cas near the bottom):

  if (! CODING_REQUIRE_ENCODING (coding))
    {
      coding->consumed = SBYTES (str);
      coding->consumed_char = SCHARS (str);
      if (STRING_MULTIBYTE (str))
       {
         str = Fstring_as_unibyte (str);
         nocopy = 1;
       }
      coding->produced = SBYTES (str);
      coding->produced_char = SCHARS (str);
      return (nocopy ? str : Fcopy_sequence (str));
    }

  if (coding->composing != COMPOSITION_DISABLED)
    coding_save_composition (coding, from, to, str);

  /* Try to skip the heading and tailing ASCIIs.  We can't skip them
     if we must run CCL program or there are compositions to
     encode.  */
  if (coding->type != coding_type_ccl
      && (! coding->cmp_data || coding->cmp_data->used == 0))
    {
      SHRINK_CONVERSION_REGION (&from, &to_byte, coding, SDATA (str),
                                1);
      if (from == to_byte)
       {
         coding_free_composition_data (coding);
         return (nocopy ? str : Fcopy_sequence (str));
       }
      shrinked_bytes = from + (SBYTES (str) - to_byte);
    }

So if str is mulitbyte when it enters this function, the return value is 
multibyte.  I suspect it is here Fstring_as_unibyte should be called, as 
it is in the previous early return.

    Jan D.

Re: Coding system conversion error

[Kenichi Handa]

Sorry for the late response on this thread.

At first, I think xassert in lisp_data_to_selection_data
(xselect.c) is wrong.  Here, obj is generated by a Lisp code
that may generate a multibyte string by error (as in the
current case).  But, in general, an error in Lisp code
should not lead to abort.  So, I propose this change.  I
choose string_make_unibyte instead of string_as_unibyte to
avoid exporting Emacs' internal leading bytes.

*** xselect.c	12 Feb 2005 09:54:46 +0900	1.148
--- xselect.c	12 Feb 2005 10:39:47 +0900	
***************
*** 1908,1914 ****
      }
    else if (STRINGP (obj))
      {
!       xassert (! STRING_MULTIBYTE (obj));
        if (NILP (type))
  	type = QSTRING;
        *format_ret = 8;
--- 1908,1915 ----
      }
    else if (STRINGP (obj))
      {
!       if (STRING_MULTIBYTE (obj))
! 	obj = string_make_unibyte (obj);
        if (NILP (type))
  	type = QSTRING;
        *format_ret = 8;


In article <420BD292.6040607@swipnet.se>, "Jan D." <jan.h.d@swipnet.se> writes:

> ELISP>  (setq str (string-to-multibyte <1025 ASCII character string>))
> ...
> ELISP>  (multibyte-string-p str)
> t
> ELISP>  (multibyte-string-p (encode-coding-string str
>                                               
> 'compound-text-with-extensions))
> t     <---- BUG, should be nil
> ELISP>  (multibyte-string-p (encode-coding-string str 'utf-8))
> nil

Thank you for finding this bug.  As encode-coding-string
should be regarded as an interface between multibyte and
unibyte, it should always return an unibyte string.  But,
NOCOPY argument in encode-coding-string and
encode_coding_string is to avoid unnecessary string
allocation if STR is ASCII-only.  So, in such a case, I'm
going to change that function to modify STR to unibyte
directly (i.e. by calling STRING_SET_UNIBYTE) instead of
calling Fstring_as_unibyte.

What do you think?

---
Ken'ichi HANDA
handa@m17n.org

Re: Coding system conversion error

[Kenichi Handa]

In article <jwvu0omlj5o.fsf-monnier+emacs@gnu.org>, Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>  --- 1977,1983 ----
>>      SDATA_NBYTES (data) = nbytes;
>>    #endif
s->  size = nchars;
>>  !   s->size_byte = nchars != nbytes ? nbytes : -1;
s->  data[nbytes] = '\0';
>>    #ifdef GC_CHECK_STRING_OVERRUN
>>      bcopy (string_overrun_cookie, (char *) data + needed,

> This doesn't look like the right fix.
> Normally, the caller would instead use `STRING_SET_UNIBYTE' after the call
> (or rather calls one of make_foo_string which does it for him) if needed.

I agree.  And, I think we can solve the current problem by
the change I wrote in the previous mail.

---
Ken'ichi HANDA
handa@m17n.org

Re: Coding system conversion error

[Jan D.]

> At first, I think xassert in lisp_data_to_selection_data
> (xselect.c) is wrong.  Here, obj is generated by a Lisp code
> that may generate a multibyte string by error (as in the
> current case).  But, in general, an error in Lisp code
> should not lead to abort.  So, I propose this change.  I
> choose string_make_unibyte instead of string_as_unibyte to
> avoid exporting Emacs' internal leading bytes.
>
> *** xselect.c	12 Feb 2005 09:54:46 +0900	1.148
> --- xselect.c	12 Feb 2005 10:39:47 +0900	
> ***************
> *** 1908,1914 ****
>       }
>     else if (STRINGP (obj))
>       {
> !       xassert (! STRING_MULTIBYTE (obj));
>         if (NILP (type))
>   	type = QSTRING;
>         *format_ret = 8;
> --- 1908,1915 ----
>       }
>     else if (STRINGP (obj))
>       {
> !       if (STRING_MULTIBYTE (obj))
> ! 	obj = string_make_unibyte (obj);
>         if (NILP (type))
>   	type = QSTRING;
>         *format_ret = 8;

If the multibyte string is generated by an error and this is one of the 
places where we can detect the error, should we not keep the xassert?

	Jan D.

Re: Coding system conversion error

[Richard Stallman]

    > Normally, the caller would instead use `STRING_SET_UNIBYTE' after the 
    > call
    > (or rather calls one of make_foo_string which does it for him) if 
    > needed.

This is not the same as what I suggested, but this too is ok.
However, if we stick with this, we should document it better
in the comments before allocate_string_data.

Re: Coding system conversion error

[Kim F. Storm]

"Jan D." <jan.h.d@swipnet.se> writes:

>> At first, I think xassert in lisp_data_to_selection_data
>> (xselect.c) is wrong.  Here, obj is generated by a Lisp code
>> that may generate a multibyte string by error (as in the
>> current case).  But, in general, an error in Lisp code
>> should not lead to abort.  So, I propose this change.  I
>> choose string_make_unibyte instead of string_as_unibyte to
>> avoid exporting Emacs' internal leading bytes.
>>
>> *** xselect.c	12 Feb 2005 09:54:46 +0900	1.148
>> --- xselect.c	12 Feb 2005 10:39:47 +0900	
>> ***************
>> *** 1908,1914 ****
>>       }
>>     else if (STRINGP (obj))
>>       {
>> !       xassert (! STRING_MULTIBYTE (obj));
>>         if (NILP (type))
>>   	type = QSTRING;
>>         *format_ret = 8;
>> --- 1908,1915 ----
>>       }
>>     else if (STRINGP (obj))
>>       {
>> !       if (STRING_MULTIBYTE (obj))
>> ! 	obj = string_make_unibyte (obj);
>>         if (NILP (type))
>>   	type = QSTRING;
>>         *format_ret = 8;
>
> If the multibyte string is generated by an error and this is one of
> the places where we can detect the error, should we not keep the
> xassert?

I agree, but since the source of the error is in Lisp code, it would
be more helpful to signal an error rather than abort.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Coding system conversion error

[Kenichi Handa]

In article <30e47fd1ee5ebcc03c15fe8c905bc525@swipnet.se>, "Jan D." <jan.h.d@swipnet.se> writes:

> If the multibyte string is generated by an error and this is one of the 
> places where we can detect the error, should we not keep the xassert?

storm@cua.dk (Kim F. Storm) writes:
> I agree, but since the source of the error is in Lisp code, it would
> be more helpful to signal an error rather than abort.

I agree that signaling an error is better than xassert.

But, it seems that a function in selection-converter-alist
can return a multibyte string as long as we have a fixed
rule about how to handle it.  And "converting to a unibyte
string by string-make-unibyte" seems to be a good rule.

---
Ken'ichi HANDA
handa@m17n.org

Re: Coding system conversion error

[Kim F. Storm]

Kenichi Handa <handa@m17n.org> writes:

> In article <30e47fd1ee5ebcc03c15fe8c905bc525@swipnet.se>, "Jan D." <jan.h.d@swipnet.se> writes:
>
>> If the multibyte string is generated by an error and this is one of the 
>> places where we can detect the error, should we not keep the xassert?
>
> storm@cua.dk (Kim F. Storm) writes:
>> I agree, but since the source of the error is in Lisp code, it would
>> be more helpful to signal an error rather than abort.
>
> I agree that signaling an error is better than xassert.
>
> But, it seems that a function in selection-converter-alist
> can return a multibyte string as long as we have a fixed
> rule about how to handle it.  And "converting to a unibyte
> string by string-make-unibyte" seems to be a good rule.

I see.

Could you please install that change now, as I've been bitten by
this crash more than once.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Coding system conversion error

[Stefan Monnier]

> I agree that signaling an error is better than xassert.

> But, it seems that a function in selection-converter-alist
> can return a multibyte string as long as we have a fixed
> rule about how to handle it.  And "converting to a unibyte
> string by string-make-unibyte" seems to be a good rule.

String-make-unibyte might not do the right thing.  It's just a guess when we
don't have any alternative.  In this case we have an alternative which is to
signal an error.
After all, this did catch an error in the handling of encode-coding-string
with compound-text, so I think it's better to signal the error than to
silently try to correct it.


        Stefan

Re: Coding system conversion error

[Kenichi Handa]

In article <87y8ds4f6d.fsf-monnier+emacs@gnu.org>, Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>  I agree that signaling an error is better than xassert.
>>  But, it seems that a function in selection-converter-alist
>>  can return a multibyte string as long as we have a fixed
>>  rule about how to handle it.  And "converting to a unibyte
>>  string by string-make-unibyte" seems to be a good rule.

> String-make-unibyte might not do the right thing.  It's just a guess when we
> don't have any alternative.  In this case we have an alternative which is to
> signal an error.
> After all, this did catch an error in the handling of encode-coding-string
> with compound-text, so I think it's better to signal the error than to
> silently try to correct it.

I reconsidered this problem, and now I agree with you.  I
was at first negative on signaling an error in
lisp_data_to_selection_data because I was not sure it is
safe to do that.  But, I found that Fsignal is already use
in this function.  So, I've just installed these changes.

2005-02-14  Kenichi Handa  <handa@m17n.org>

	* coding.c (encode_coding_string): Always return a unibyte string.
	If NOCOPY is nonzero and there's no need of encoding, make STR
	unibyte directly.

	* xselect.c (lisp_data_to_selection_data): If OBJ is a non-ASCII
	multibyte string, signal an error instead of aborting.

---
Ken'ichi HANDA
handa@m17n.org

Re: Coding system conversion error

[Jan D.]

> I reconsidered this problem, and now I agree with you.  I
> was at first negative on signaling an error in
> lisp_data_to_selection_data because I was not sure it is
> safe to do that.  But, I found that Fsignal is already use
> in this function.  So, I've just installed these changes.
>
> 2005-02-14  Kenichi Handa  <handa@m17n.org>
>
> 	* coding.c (encode_coding_string): Always return a unibyte string.
> 	If NOCOPY is nonzero and there's no need of encoding, make STR
> 	unibyte directly.
>
> 	* xselect.c (lisp_data_to_selection_data): If OBJ is a non-ASCII
> 	multibyte string, signal an error instead of aborting.

The error message can be improved if you put it in 
x_handle_selection_request instead (lisp_data_to_selection_data is only 
called from there).  Then you can get the target_symbol in to the error 
text, which otherwise can be hard to know.

	Jan D.

Re: Coding system conversion error

[Kenichi Handa]

In article <50e2f24c825ce097d2da5c2208fd5a91@swipnet.se>, "Jan D." <jan.h.d@swipnet.se> writes:
>>  	* xselect.c (lisp_data_to_selection_data): If OBJ is a non-ASCII
>>  	multibyte string, signal an error instead of aborting.

> The error message can be improved if you put it in 
> x_handle_selection_request instead (lisp_data_to_selection_data is only 
> called from there).  Then you can get the target_symbol in to the error 
> text, which otherwise can be hard to know.

That's true.  But I think the error should be signaled just
before we use the data.  Otherwise, the other code will slip
into between the call of Fsignal and the place where the
data is used.

---
Ken'ichi HANDA
handa@m17n.org