Re: emacs-26 c87d04e: Avoid aborts in 'md5'

Re: emacs-26 c87d04e: Avoid aborts in 'md5'

[Michael Albinus]

eliz@gnu.org (Eli Zaretskii) writes:

> diff --git a/src/fns.c b/src/fns.c
> index aba34fd..de1dad3 100644
> --- a/src/fns.c
> +++ b/src/fns.c
> @@ -4952,6 +4952,9 @@ extract_data_from_object (Lisp_Object spec,
>  #endif
>      }
>  
> +  if (!STRINGP (object))
> +    signal_error ("Invalid object argument",
> +		  NILP (object) ? build_string ("nil") : object);
>    return SSDATA (object);
>  }

Don't we call CHECK_STRING (object) in such cases, which raises the
standardized error Qwrong_type_argument?

Best regards, Michael.

Re: emacs-26 c87d04e: Avoid aborts in 'md5'

[Andreas Schwab]

On Feb 28 2018, Michael Albinus <michael.albinus@gmx.de> wrote:

> eliz@gnu.org (Eli Zaretskii) writes:
>
>> diff --git a/src/fns.c b/src/fns.c
>> index aba34fd..de1dad3 100644
>> --- a/src/fns.c
>> +++ b/src/fns.c
>> @@ -4952,6 +4952,9 @@ extract_data_from_object (Lisp_Object spec,
>>  #endif
>>      }
>>  
>> +  if (!STRINGP (object))
>> +    signal_error ("Invalid object argument",
>> +		  NILP (object) ? build_string ("nil") : object);
>>    return SSDATA (object);
>>  }
>
> Don't we call CHECK_STRING (object) in such cases, which raises the
> standardized error Qwrong_type_argument?

How can that ever happen anyway?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: emacs-26 c87d04e: Avoid aborts in 'md5'

[Michael Albinus]

Andreas Schwab <schwab@suse.de> writes:

> On Feb 28 2018, Michael Albinus <michael.albinus@gmx.de> wrote:
>
>> eliz@gnu.org (Eli Zaretskii) writes:
>>
>>> diff --git a/src/fns.c b/src/fns.c
>>> index aba34fd..de1dad3 100644
>>> --- a/src/fns.c
>>> +++ b/src/fns.c
>>> @@ -4952,6 +4952,9 @@ extract_data_from_object (Lisp_Object spec,
>>>  #endif
>>>      }
>>>  
>>> +  if (!STRINGP (object))
>>> +    signal_error ("Invalid object argument",
>>> +		  NILP (object) ? build_string ("nil") : object);
>>>    return SSDATA (object);
>>>  }
>>
>> Don't we call CHECK_STRING (object) in such cases, which raises the
>> standardized error Qwrong_type_argument?
>
> How can that ever happen anyway?

See bug#30627.

> Andreas.

Best regards, Michael.

Re: emacs-26 c87d04e: Avoid aborts in 'md5'

[Andreas Schwab]

On Feb 28 2018, Michael Albinus <michael.albinus@gmx.de> wrote:

> Andreas Schwab <schwab@suse.de> writes:
>
>> On Feb 28 2018, Michael Albinus <michael.albinus@gmx.de> wrote:
>>
>>> eliz@gnu.org (Eli Zaretskii) writes:
>>>
>>>> diff --git a/src/fns.c b/src/fns.c
>>>> index aba34fd..de1dad3 100644
>>>> --- a/src/fns.c
>>>> +++ b/src/fns.c
>>>> @@ -4952,6 +4952,9 @@ extract_data_from_object (Lisp_Object spec,
>>>>  #endif
>>>>      }
>>>>  
>>>> +  if (!STRINGP (object))
>>>> +    signal_error ("Invalid object argument",
>>>> +		  NILP (object) ? build_string ("nil") : object);
>>>>    return SSDATA (object);
>>>>  }
>>>
>>> Don't we call CHECK_STRING (object) in such cases, which raises the
>>> standardized error Qwrong_type_argument?
>>
>> How can that ever happen anyway?
>
> See bug#30627.

So the CHECK_STRING should be put in the else branch.  There is also a
useless CHECK_BUFFER in the BUFFERP branch.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: emacs-26 c87d04e: Avoid aborts in 'md5'

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Wed, 28 Feb 2018 09:30:59 +0100
> 
> > +  if (!STRINGP (object))
> > +    signal_error ("Invalid object argument",
> > +		  NILP (object) ? build_string ("nil") : object);
> >    return SSDATA (object);
> >  }
> 
> Don't we call CHECK_STRING (object) in such cases, which raises the
> standardized error Qwrong_type_argument?

Using CHECK_STRING would produce a misleading error message, because
the APIs that call extract_data_from_object accept objects other than
strings.

Re: emacs-26 c87d04e: Avoid aborts in 'md5'

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Cc: emacs-devel@gnu.org,  Eli Zaretskii <eliz@gnu.org>
> Date: Wed, 28 Feb 2018 10:15:04 +0100
> 
> > See bug#30627.
> 
> So the CHECK_STRING should be put in the else branch.

In general, safe code should check STRINGP before using SSDATA, so the
way I did it is safer (and also more future-proof).

> There is also a useless CHECK_BUFFER in the BUFFERP branch.

Indeed.

Re: emacs-26 c87d04e: Avoid aborts in 'md5'

[Andreas Schwab]

On Feb 28 2018, Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Andreas Schwab <schwab@suse.de>
>> Cc: emacs-devel@gnu.org,  Eli Zaretskii <eliz@gnu.org>
>> Date: Wed, 28 Feb 2018 10:15:04 +0100
>> 
>> > See bug#30627.
>> 
>> So the CHECK_STRING should be put in the else branch.
>
> In general, safe code should check STRINGP before using SSDATA, so the
> way I did it is safer (and also more future-proof).

All branches already use SSDATA or SBYTES before that, so nothing is
gained.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: emacs-26 c87d04e: Avoid aborts in 'md5'

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Cc: michael.albinus@gmx.de,  emacs-devel@gnu.org
> Date: Wed, 28 Feb 2018 17:32:30 +0100
> 
> > In general, safe code should check STRINGP before using SSDATA, so the
> > way I did it is safer (and also more future-proof).
> 
> All branches already use SSDATA or SBYTES before that, so nothing is
> gained.

With the current code, I agree.

Re: emacs-26 c87d04e: Avoid aborts in 'md5'

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 110 bytes --]

On 02/28/2018 09:24 AM, Eli Zaretskii wrote:
> With the current code, I agree.

OK, proposed patch attached.


[-- Attachment #2: 0001-Avoid-unnecessary-STRINGP-test-in-recent-fix.patch --]
[-- Type: text/x-patch, Size: 870 bytes --]

From 1858a2c261f61cff1c62b9238dfb29a3eba5ecfa Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 28 Feb 2018 11:13:31 -0800
Subject: [PATCH] Avoid unnecessary STRINGP test in recent fix

Problem noted by Andreas Schwab in:
https://lists.gnu.org/r/emacs-devel/2018-02/msg00854.html
* src/fns.c (Fsecure_hash_algorithms): Simplify previous change.
---
 src/fns.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/fns.c b/src/fns.c
index de1dad3736..0a458ff45a 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -4951,8 +4951,7 @@ extract_data_from_object (Lisp_Object spec,
       error ("GnuTLS is not available, so `iv-auto' can't be used");
 #endif
     }
-
-  if (!STRINGP (object))
+  else
     signal_error ("Invalid object argument",
 		  NILP (object) ? build_string ("nil") : object);
   return SSDATA (object);
-- 
2.14.3

Re: emacs-26 c87d04e: Avoid aborts in 'md5'

[Eli Zaretskii]

> Cc: michael.albinus@gmx.de, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 28 Feb 2018 11:14:36 -0800
> 
> > With the current code, I agree.
> 
> OK, proposed patch attached.

That's not what I meant.  I meant that as long as the code is frozen
in its current form, nothing is gained by omitting 'else'.  But the
change I made is more future-proof.

As I said, we always check STRINGP before using SBYTES or SSDATA, so I
see no reason why we shouldn't in this case.

Re: emacs-26 c87d04e: Avoid aborts in 'md5'

[Paul Eggert]

On 02/28/2018 12:35 PM, Eli Zaretskii wrote:
> That's not what I meant.  I meant that as long as the code is frozen
> in its current form, nothing is gained by omitting 'else'.

I'm puzzled, since my patch inserts 'else', and yet you appear to be 
objecting to it on the grounds that nothing is gained by omitting 
'else'. I must be misunderstanding.

> we always check STRINGP before using SBYTES or SSDATA

Only if there's some possibility that the object is not a string. In 
general, Emacs C functions very often use SBYTES or SDATA without first 
checking STRINGP, because they are in contexts where the object must be 
a string. For fun I just now looked at print.c, and none of the first 
ten occurrences of SDATA or SSDATA were protected by STRINGP. And that's 
OK, since none of these occurrences needed STRINGP.

For the case we're talking about, it's not possible for the object to be 
a string, so although it might be appropriate to have an eassert 
(STRINGP (...)) to verify that the impossible does not happen (to help 
future-proof the code, say), we shouldn't need a runtime check in 
production code.

Re: emacs-26 c87d04e: Avoid aborts in 'md5'

[Eli Zaretskii]

> Cc: schwab@suse.de, michael.albinus@gmx.de, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 28 Feb 2018 14:14:46 -0800
> 
> On 02/28/2018 12:35 PM, Eli Zaretskii wrote:
> > That's not what I meant.  I meant that as long as the code is frozen
> > in its current form, nothing is gained by omitting 'else'.
> 
> I'm puzzled, since my patch inserts 'else', and yet you appear to be 
> objecting to it on the grounds that nothing is gained by omitting 
> 'else'. I must be misunderstanding.

"Nothing is gained" was Andreas's argument, not mine.

> For the case we're talking about, it's not possible for the object to be 
> a string, so although it might be appropriate to have an eassert 
> (STRINGP (...)) to verify that the impossible does not happen (to help 
> future-proof the code, say), we shouldn't need a runtime check in 
> production code.

I don't want a function called from half a dozen places, which
generates a string from at least 3 different object types, to become a
cause for an assertion violation or worse.  It is a large and
not-so-simple function, where drawing such conclusions could yield
mistakes.

So let's please stop this particular bike-shedding and leave the code
alone.  There's nothing wrong with it.