unicode build: "comparison is always true" warnings

unicode build: "comparison is always true" warnings

[Mark A. Hershberger]

I'm working on providing snapshots of the unicode branch and have been
getting a ton of these messages:

    warning: comparison is always true due to limited range of data type

Most are in syntax.c, but one shows up in search.c.

There is also a warning from movemail.c:

    warning: the use of `mktemp' is dangerous, better use `mkstemp' or
    `mkdtemp'

Are these significant?  Should I be worried?

Shortened link to launchpad.net build log: http://xrl.us/bbntv

Mark.

-- 
http://hexmode.com/
GPG Fingerprint: 7E15 362D A32C DFAB E4D2  B37A 735E F10A 2DFC BFF5

The most beautiful experience we can have is the mysterious.
    -- Albert Einstein, The World As I See it

Re: unicode build: "comparison is always true" warnings

[Stefan Monnier]

> I'm working on providing snapshots of the unicode branch and have been
> getting a ton of these messages:

>     warning: comparison is always true due to limited range of data type

> Most are in syntax.c, but one shows up in search.c.

These are difficult to eliminate without making the code uglier.
IIRC the issue is typically that there's are generic macros to handle
characters and they can be used on any character (not just on 8-bit
chars).  But when you use it on a "char", part of the macro becomes
trivially optimizable and gcc thinks this may in fact indicate the
presence of a bug.
Maybe we should just silence these warnings with the
appropriate -Wno-<foobar>.

> There is also a warning from movemail.c:

>     warning: the use of `mktemp' is dangerous, better use `mkstemp' or
>     `mkdtemp'

This one looks bad.


        Stefan

Re: unicode build: "comparison is always true" warnings

[Mark A. Hershberger]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> There is also a warning from movemail.c:
>>     warning: the use of `mktemp' is dangerous, better use `mkstemp' or
>>     `mkdtemp'
>
> This one looks bad.

I attempted to fix this.  It has been ages since I had to write C, but
this fix is almost identical to the one here: http://xrl.us/bbocw

I'll apply if this looks ok.

diff -u -b -r1.88 movemail.c
--- movemail.c	26 Jul 2007 05:26:12 -0000	1.88
+++ movemail.c	23 Nov 2007 17:38:40 -0000
@@ -325,14 +325,20 @@
 	p--;
       *p = 0;
       strcpy (p, "EXXXXXX");
+#ifndef HAVE_MKSTEMP
       mktemp (tempname);
       unlink (tempname);
+#endif
 
       while (1)
 	{
 	  /* Create the lock file, but not under the lock file name.  */
 	  /* Give up if cannot do that.  */
+#ifndef HAVE_MKSTEMP
 	  desc = open (tempname, O_WRONLY | O_CREAT | O_EXCL, 0666);
+#else
+          desc = mkstemp (tempname);
+#endif
 	  if (desc < 0)
 	    {
 	      char *message = (char *) xmalloc (strlen (tempname) + 50);




-- 
http://hexmode.com/
GPG Fingerprint: 7E15 362D A32C DFAB E4D2  B37A 735E F10A 2DFC BFF5

The most beautiful experience we can have is the mysterious.
    -- Albert Einstein, The World As I See it

Re: unicode build: "comparison is always true" warnings

[Andreas Schwab]

mah@everybody.org (Mark A. Hershberger) writes:

>        while (1)
>  	{
>  	  /* Create the lock file, but not under the lock file name.  */
>  	  /* Give up if cannot do that.  */
> +#ifndef HAVE_MKSTEMP
>  	  desc = open (tempname, O_WRONLY | O_CREAT | O_EXCL, 0666);
> +#else
> +          desc = mkstemp (tempname);
> +#endif

That won't work since mkstemp modifies tempname in place, but can be
called more than once here (the second call will return with EINVAL).

(The use of mktemp is not really bad here anyway.)

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: unicode build: "comparison is always true" warnings

[Stefan Monnier]

> (The use of mktemp is not really bad here anyway.)

Why not?  (don't hesitate to put the explanation in the source code, of
course).


        Stefan

Re: unicode build: "comparison is always true" warnings

[Mark A. Hershberger]

Andreas Schwab <schwab@suse.de> writes:

> mah@everybody.org (Mark A. Hershberger) writes:
>
>>        while (1)
>>  	{
>>  	  /* Create the lock file, but not under the lock file name.  */
>>  	  /* Give up if cannot do that.  */
>> +#ifndef HAVE_MKSTEMP
>>  	  desc = open (tempname, O_WRONLY | O_CREAT | O_EXCL, 0666);
>> +#else
>> +          desc = mkstemp (tempname);
>> +#endif
>
> That won't work since mkstemp modifies tempname in place, but can be
> called more than once here (the second call will return with EINVAL).

There is only one call.

If you look at the full patch, mktemp (not mkstemp) is called once if
HAVE_MKSTEMP is _not_ defined.  Here, mkstemp (not mktemp) is called if
HAVE_MKSTEMP _is_ defined.

Mark.

-- 
http://hexmode.com/
GPG Fingerprint: 7E15 362D A32C DFAB E4D2  B37A 735E F10A 2DFC BFF5

The most beautiful experience we can have is the mysterious.
    -- Albert Einstein, The World As I See it

Re: unicode build: "comparison is always true" warnings

[Andreas Schwab]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> (The use of mktemp is not really bad here anyway.)
>
> Why not?

Because it is used securely.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: unicode build: "comparison is always true" warnings

[Andreas Schwab]

mah@everybody.org (Mark A. Hershberger) writes:

> Andreas Schwab <schwab@suse.de> writes:
>
>> mah@everybody.org (Mark A. Hershberger) writes:
>>
>>>        while (1)
>>>  	{
>>>  	  /* Create the lock file, but not under the lock file name.  */
>>>  	  /* Give up if cannot do that.  */
>>> +#ifndef HAVE_MKSTEMP
>>>  	  desc = open (tempname, O_WRONLY | O_CREAT | O_EXCL, 0666);
>>> +#else
>>> +          desc = mkstemp (tempname);
>>> +#endif
>>
>> That won't work since mkstemp modifies tempname in place, but can be
>> called more than once here (the second call will return with EINVAL).
>
> There is only one call.

Inside a loop.

> If you look at the full patch, mktemp (not mkstemp) is called once if
> HAVE_MKSTEMP is _not_ defined.  Here, mkstemp (not mktemp) is called if
> HAVE_MKSTEMP _is_ defined.

Correct but irrelevant.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: unicode build: "comparison is always true" warnings

[Stefan Monnier]

>>> (The use of mktemp is not really bad here anyway.)
>> Why not?
> Because it is used securely.

I assumed that's what you meant.  The question is: why is it secure in
this case?


        Stefan

Re: unicode build: "comparison is always true" warnings

[Glenn Morris]

Stefan Monnier wrote:

>> Because it is used securely.
>
> I assumed that's what you meant.  The question is: why is it secure in
> this case?

I think Andreas must pay for email by the word... ;)

Re: unicode build: "comparison is always true" warnings

[Andreas Schwab]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>> (The use of mktemp is not really bad here anyway.)
>>> Why not?
>> Because it is used securely.
>
> I assumed that's what you meant.  The question is: why is it secure in
> this case?

Please tell me which part of the sources you have difficulties to
understand, then I can explain it to you.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: unicode build: "comparison is always true" warnings

[Mark A. Hershberger]

Andreas Schwab <schwab@suse.de> writes:

> Please tell me which part of the sources you have difficulties to
> understand, then I can explain it to you.

Other people will see the compilation warning and naively ask “Why is
emacs doing things insecurely?”

It would be helpful to have an explanation in the comments to explain
_why_ it is secure despite the warnings.

I'll readily admit I don't know enough about mailfile locking or mktemp
to understand why this is secure despite the warnings.  For those of us
who don't readily understand secure mailfile locking, could you explain
why mktemp is ok?

-- 
http://hexmode.com/
GPG Fingerprint: 7E15 362D A32C DFAB E4D2  B37A 735E F10A 2DFC BFF5

The most beautiful experience we can have is the mysterious.
    -- Albert Einstein, The World As I See it

Re: unicode build: "comparison is always true" warnings

[Andreas Schwab]

mah@everybody.org (Mark A. Hershberger) writes:

> I'll readily admit I don't know enough about mailfile locking or mktemp
> to understand why this is secure despite the warnings.  For those of us
> who don't readily understand secure mailfile locking, could you explain
> why mktemp is ok?

Please tell me what you think is unsecure here?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: unicode build: "comparison is always true" warnings

[Mark A. Hershberger]

Andreas Schwab <schwab@suse.de> writes:

> Please tell me what you think is unsecure here?

mktemp

-- 
http://hexmode.com/
GPG Fingerprint: 7E15 362D A32C DFAB E4D2  B37A 735E F10A 2DFC BFF5

The most beautiful experience we can have is the mysterious.
    -- Albert Einstein, The World As I See it

Re: unicode build: "comparison is always true" warnings

[Andreas Schwab]

mah@everybody.org (Mark A. Hershberger) writes:

> Andreas Schwab <schwab@suse.de> writes:
>
>> Please tell me what you think is unsecure here?
>
> mktemp

Please expand.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: unicode build: "comparison is always true" warnings

[Jan Djärv]

 From the man page of mktemp:

        Since on the one  hand  the  names
        are  easy to guess, and on the other hand there is a race between test-
        ing whether the name exists and opening the file, every use of mktemp()
        is a security risk.  The race is avoided by mkstemp(3).



Andreas Schwab skrev:
> mah@everybody.org (Mark A. Hershberger) writes:
> 
>> Andreas Schwab <schwab@suse.de> writes:
>>
>>> Please tell me what you think is unsecure here?
>> mktemp
> 
> Please expand.
> 

Re: unicode build: "comparison is always true" warnings

[Andreas Schwab]

Jan Djärv <jan.h.d@swipnet.se> writes:

> From the man page of mktemp:
>
>        Since on the one  hand  the  names
>        are  easy to guess, and on the other hand there is a race between test-
>        ing whether the name exists and opening the file, every use of mktemp()
>        is a security risk.  The race is avoided by mkstemp(3).

In which way it is different from what mkstemp is doing?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: unicode build: "comparison is always true" warnings

[Matthieu Lemerre]

Andreas Schwab <schwab@suse.de> writes:
>
> In which way it is different from what mkstemp is doing?
>

When you use mktemp, you have to use two operations:
1/Create a file name with mktemp
2/Open it

An attacker could create a symlink with this name file between these
two operations, thus the file would be erased.

mkstemp guarantees these two operations to be done atomically by the
kernel (and the kernel chooses a name corresponding to an unused file
name), so the race condition does not work in that case; the file
descriptor is guaranteed to always correspond to a new file.

Re: unicode build: "comparison is always true" warnings

[Jan Djärv]

Andreas Schwab skrev:
> Jan Djärv <jan.h.d@swipnet.se> writes:
> 
>> Ok.  But mkstemp has one advantage that mktemp doesn't have, it does not emit
>> a warning when compiling :-).  I think that is cause enough to use mkstemp
>> where available, even if they both are equivalent.
> 
> It also adds complexity to a already quite complex program for its
> task.  And as the first attempt shows, it is not trivial to use mkstemp
> without breaking the program.
> 

The patch just needs some small changes:


*** movemail.c.~1.88.~  2007-07-27 09:52:07.000000000 +0200
--- movemail.c  2007-11-24 14:07:13.000000000 +0100
***************
*** 324,338 ****
--- 324,345 ----
        while (p != tempname && !IS_DIRECTORY_SEP (p[-1]))
        p--;
        *p = 0;
+ #ifndef HAVE_MKSTEMP
        strcpy (p, "EXXXXXX");
        mktemp (tempname);
        unlink (tempname);
+ #endif

        while (1)
        {
          /* Create the lock file, but not under the lock file name.  */
          /* Give up if cannot do that.  */
+ #ifndef HAVE_MKSTEMP
          desc = open (tempname, O_WRONLY | O_CREAT | O_EXCL, 0666);
+ #else
+           strcpy (p, "EXXXXXX");
+           desc = mkstemp (tempname);
+ #endif
          if (desc < 0)
            {
              char *message = (char *) xmalloc (strlen (tempname) + 50);

	Jan D.

Re: unicode build: "comparison is always true" warnings

[Andreas Schwab]

Jan Djärv <jan.h.d@swipnet.se> writes:

> The patch just needs some small changes:
>
>
> *** movemail.c.~1.88.~  2007-07-27 09:52:07.000000000 +0200
> --- movemail.c  2007-11-24 14:07:13.000000000 +0100
> ***************
> *** 324,338 ****
> --- 324,345 ----
>         while (p != tempname && !IS_DIRECTORY_SEP (p[-1]))
>         p--;
>         *p = 0;
> + #ifndef HAVE_MKSTEMP
>         strcpy (p, "EXXXXXX");
>         mktemp (tempname);
>         unlink (tempname);
> + #endif
>
>         while (1)
>         {
>           /* Create the lock file, but not under the lock file name.  */
>           /* Give up if cannot do that.  */
> + #ifndef HAVE_MKSTEMP
>           desc = open (tempname, O_WRONLY | O_CREAT | O_EXCL, 0666);
> + #else
> +           strcpy (p, "EXXXXXX");
> +           desc = mkstemp (tempname);
> + #endif
>           if (desc < 0)
>             {
>               char *message = (char *) xmalloc (strlen (tempname) + 50);

I'd suggest moving the mktemp into the loop as well.  This will avoid
code duplication.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: unicode build: "comparison is always true" warnings

[Andreas Schwab]

Matthieu Lemerre <racin@free.fr> writes:

> Andreas Schwab <schwab@suse.de> writes:
>>
>> In which way it is different from what mkstemp is doing?
>>
>
> When you use mktemp, you have to use two operations:
> 1/Create a file name with mktemp
> 2/Open it

That's the same what mkstemp does.

> An attacker could create a symlink with this name file between these
> two operations, thus the file would be erased.

That does not happen, because the open will fail when the file already
exists.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: unicode build: "comparison is always true" warnings

[Matthieu Lemerre]

>> An attacker could create a symlink with this name file between these
>> two operations, thus the file would be erased.
>
> That does not happen, because the open will fail when the file already
> exists.
>
> Andreas.

You're right, this is the meaning of O_EXCL, that I had forgotten.

Then I agree with you; mkstemp is just a commodity.

Re: unicode build: "comparison is always true" warnings

[David Kastrup]

Andreas Schwab <schwab@suse.de> writes:

> Matthieu Lemerre <racin@free.fr> writes:
>
>> An attacker could create a symlink with this name file between these
>> two operations, thus the file would be erased.
>
> That does not happen, because the open will fail when the file already
> exists.

And I bet it was impossible for you to mention this in the last 6 mails
or so.  After all, the system call could possibly have been considered
unsafe because of a buffer overflow or something (and that's so
absolutely very likely what GCC tries warning about).  So before
accidently giving one pointer too many, better taunt the other to go out
on a limb before showing him wrong.

There is probably something educational hidden in there, but I am likely
too dull to discern it.

Sorry for the rant.  I just think that at times we could be more helpful
towards each other.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: unicode build: "comparison is always true" warnings

[Andreas Schwab]

David Kastrup <dak@gnu.org> writes:

> Andreas Schwab <schwab@suse.de> writes:
>
>> Matthieu Lemerre <racin@free.fr> writes:
>>
>>> An attacker could create a symlink with this name file between these
>>> two operations, thus the file would be erased.
>>
>> That does not happen, because the open will fail when the file already
>> exists.
>
> And I bet it was impossible for you to mention this in the last 6 mails
> or so.

I have asked several times what it is considered insecure, but that was
the first mail that gave a hint.  I cannot read anyone's mind.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: unicode build: "comparison is always true" warnings

[David Kastrup]

Andreas Schwab <schwab@suse.de> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> Andreas Schwab <schwab@suse.de> writes:
>>
>>> Matthieu Lemerre <racin@free.fr> writes:
>>>
>>>> An attacker could create a symlink with this name file between these
>>>> two operations, thus the file would be erased.
>>>
>>> That does not happen, because the open will fail when the file already
>>> exists.
>>
>> And I bet it was impossible for you to mention this in the last 6 mails
>> or so.
>
> I have asked several times what it is considered insecure, but that
> was the first mail that gave a hint.  I cannot read anyone's mind.

What a delicately selective quotation for the final cheap shot.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: unicode build: "comparison is always true" warnings

[Mark A. Hershberger]

Andreas Schwab <schwab@suse.de> writes:

> I'd suggest moving the mktemp into the loop as well.  This will avoid
> code duplication.

like this?

--- movemail.c	26 Jul 2007 05:26:12 -0000	1.88
+++ movemail.c	24 Nov 2007 18:23:43 -0000
@@ -324,15 +324,20 @@
       while (p != tempname && !IS_DIRECTORY_SEP (p[-1]))
 	p--;
       *p = 0;
-      strcpy (p, "EXXXXXX");
-      mktemp (tempname);
-      unlink (tempname);
 
       while (1)
 	{
 	  /* Create the lock file, but not under the lock file name.  */
 	  /* Give up if cannot do that.  */
+          strcpy (p, "EXXXXXX");
+#ifndef HAVE_MKSTEMP
+          mktemp (tempname);
+          unlink (tempname);
 	  desc = open (tempname, O_WRONLY | O_CREAT | O_EXCL, 0666);
+#else
+          desc = mkstemp (tempname);
+#endif
+
 	  if (desc < 0)
 	    {
 	      char *message = (char *) xmalloc (strlen (tempname) + 50);

-- 
http://hexmode.com/
GPG Fingerprint: 7E15 362D A32C DFAB E4D2  B37A 735E F10A 2DFC BFF5

The most beautiful experience we can have is the mysterious.
    -- Albert Einstein, The World As I See it