[PATCH] Coverity: don't try using O_BINARY and O_TEXT in non-Windows platforms #13771

p5pRT · 2014-04-26T19:01:06Z

Migrated from rt.perl.org#121739 (status was 'resolved')

Searchable as RT121739$

p5pRT · 2014-04-26T19:01:06Z

From @jhi

In non-Windows platforms the O_BINARY and O_TEXT are defined, but with
both probably as zeros, which is not very useful if they are being
tested with bit-and. Leads into unreachable code.

Attached.

p5pRT · 2014-04-26T19:01:06Z

From @jhi

0001-Fix-for-Coverity-perl5-CIDs-28945-28948.patch

From f95ebb5c5a633e80b4ca0994e86906225688bb8b Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Wed, 23 Apr 2014 13:57:21 -0400
Subject: [PATCH] Fix for Coverity perl5 CIDs 28945, 28948: Logically dead code
 (DEADCODE)

The O_BINARY and O_TEXT are defined but both zero in non-Windows platforms.
Bit-anding with zero is not that useful. The PERLIO_USING_CRLF while not
fully equivalent to O_BINARY and O_TEXT being non-zero, seems to be
the idiom used elsewhere in the code.  (One case this misses is at
least e.g. O_BINARY and 0 and O_TEXT of 1, or vice versa.  But I have
no idea how to fairly handle this case since one could not still bit-and
sensibly with the zero case.)
---
 op.c     | 4 ++++
 perlio.c | 9 +++++----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/op.c b/op.c
index 716c684..3440b10 100644
--- a/op.c
+++ b/op.c
@@ -8565,6 +8565,7 @@ Perl_ck_anoncode(pTHX_ OP *o)
 static void
 S_io_hints(pTHX_ OP *o)
 {
+#ifdef PERLIO_USING_CRLF
     HV * const table =
 	PL_hints & HINT_LOCALIZE_HH ? GvHV(PL_hintgv) : NULL;;
     if (table) {
@@ -8590,6 +8591,9 @@ S_io_hints(pTHX_ OP *o)
 		o->op_private |= OPpOPEN_OUT_CRLF;
 	}
     }
+#else
+    PERL_UNUSED_ARG(o);
+#endif
 }
 
 OP *
diff --git a/perlio.c b/perlio.c
index 0ae0a43..d4a7637 100644
--- a/perlio.c
+++ b/perlio.c
@@ -199,8 +199,10 @@ PerlIO_intmode2str(int rawmode, char *mode, int *writing)
 	    mode[ix++] = '+';
 	}
     }
+#ifdef PERLIO_USING_CRLF
     if (rawmode & O_BINARY)
 	mode[ix++] = 'b';
+#endif
     mode[ix] = '\0';
     return ptype;
 }
@@ -2529,25 +2531,24 @@ PerlIOUnix_oflags(const char *mode)
 	    oflags |= O_WRONLY;
 	break;
     }
+#ifdef PERLIO_USING_CRLF
     if (*mode == 'b') {
 	oflags |= O_BINARY;
 	oflags &= ~O_TEXT;
 	mode++;
-    }
-    else if (*mode == 't') {
+    } else if (*mode == 't') {
 	oflags |= O_TEXT;
 	oflags &= ~O_BINARY;
 	mode++;
     }
     else {
-#ifdef PERLIO_USING_CRLF
 	/*
 	 * If neither "t" nor "b" was specified, open the file
 	 * in O_BINARY mode.
 	 */
 	oflags |= O_BINARY;
-#endif
     }
+#endif
     if (*mode || oflags == -1) {
 	SETERRNO(EINVAL, LIB_INVARG);
 	oflags = -1;
-- 
1.9.2

p5pRT · 2014-04-28T00:53:12Z

From @tonycoz

On Sat Apr 26 12:01:06 2014, jhi wrote:

In non-Windows platforms the O_BINARY and O_TEXT are defined, but with
both probably as zeros, which is not very useful if they are being
tested with bit-and. Leads into unreachable code.

Attached.

I suspect this patch is incorrect, at least for Cygwin.

O_TEXT and O_BINARY on Cygwin do have a meaning, but PERLIO_USING_CRLF is explicitly undefined.

I think a better check would be that O_BINARY or O_TEXT is non-zero.

I can't think of a perl level check to demonstrate the incorrectness.

Tony

p5pRT · 2014-04-28T00:53:12Z

The RT System itself - Status changed from 'new' to 'open'

p5pRT · 2014-04-28T00:56:07Z

From @jhi

On Sunday-201404-27, 20:53, Tony Cook via RT wrote:

I think a better check would be that O_BINARY or O_TEXT is non-zero.

Hmmm. I see. Well. But. If O_BINARY or O_TEXT are zero how they can
be tested with bit-and?

#if O_BINARY && O_TEXT && O_BINARY != O_TEXT

maybe?

p5pRT · 2014-04-28T01:20:43Z

From @jhi

On Sunday-201404-27, 20:53, Tony Cook via RT wrote:

On Sat Apr 26 12:01:06 2014, jhi wrote:

In non-Windows platforms the O_BINARY and O_TEXT are defined, but with
both probably as zeros, which is not very useful if they are being
tested with bit-and. Leads into unreachable code.

Attached.

I suspect this patch is incorrect, at least for Cygwin.

O_TEXT and O_BINARY on Cygwin do have a meaning, but PERLIO_USING_CRLF is explicitly undefined.

I think a better check would be that O_BINARY or O_TEXT is non-zero.

I can't think of a perl level check to demonstrate the incorrectness.

I think honestly this mess should be a Configure level test. Though, of
course, in exotic places where there's no Configure, we are back to
square one.

But a new patch attached: I now introduced a new PERLIO_ define.

Tony

p5pRT · 2014-04-28T01:20:43Z

From @jhi

0001-Fix-for-Coverity-perl5-CIDs-28945-28948.patch

From 07377388a1022492e60b0d7cb78a103076dcb0da Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Wed, 23 Apr 2014 13:57:21 -0400
Subject: [PATCH] Fix for Coverity perl5 CIDs 28945, 28948: Logically dead code
 (DEADCODE)

The O_BINARY and O_TEXT are defined but both zero in non-Windows platforms
(most common case, hybrid cases exists).  But bit-anding with zero is not
that useful.  Introduce a new define:
PERLIO_BINARY_AND_TEXT_DIFFERENT_AND_EFFECTIVE
which is defined if the O_BINARY and O_TEXT are different from each
other *and* non-zero (and they have an effect).
---
 op.c     |  4 ++++
 perl.h   | 10 +++++++++-
 perlio.c |  6 ++++--
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/op.c b/op.c
index 716c684..36f0b39 100644
--- a/op.c
+++ b/op.c
@@ -8565,6 +8565,7 @@ Perl_ck_anoncode(pTHX_ OP *o)
 static void
 S_io_hints(pTHX_ OP *o)
 {
+#ifdef PERLIO_BINARY_AND_TEXT_DIFFERENT_AND_EFFECTIVE
     HV * const table =
 	PL_hints & HINT_LOCALIZE_HH ? GvHV(PL_hintgv) : NULL;;
     if (table) {
@@ -8590,6 +8591,9 @@ S_io_hints(pTHX_ OP *o)
 		o->op_private |= OPpOPEN_OUT_CRLF;
 	}
     }
+#else
+    PERL_UNUSED_ARG(o);
+#endif
 }
 
 OP *
diff --git a/perl.h b/perl.h
index 6da39f3..6a43b12 100644
--- a/perl.h
+++ b/perl.h
@@ -5639,7 +5639,7 @@ int flock(int fd, int op);
 #endif
 
 #if O_TEXT != O_BINARY
-    /* If you have different O_TEXT and O_BINARY and you are a CLRF shop,
+    /* If you have different O_TEXT and O_BINARY and you are a CRLF shop,
      * that is, you are somehow DOSish. */
 #   if defined(__HAIKU__) || defined(__VOS__) || defined(__CYGWIN__)
     /* Haiku has O_TEXT != O_BINARY but O_TEXT and O_BINARY have no effect;
@@ -5653,6 +5653,14 @@ int flock(int fd, int op);
     /* If you really are DOSish. */
 #      define PERLIO_USING_CRLF 1
 #   endif
+#   if O_TEXT != 0 && O_BINARY != 0
+#     if !defined(__HAIKU__)
+        /* If you have O_TEXT different from your O_BINARY and
+         * they are both not zero (being zero would make testing
+         * with bit-and interesting) and they have an effect. */
+#       define PERLIO_BINARY_AND_TEXT_DIFFERENT_AND_EFFECTIVE
+#     endif
+#   endif
 #endif
 
 #ifdef I_LIBUTIL
diff --git a/perlio.c b/perlio.c
index 0ae0a43..1fab644 100644
--- a/perlio.c
+++ b/perlio.c
@@ -199,8 +199,10 @@ PerlIO_intmode2str(int rawmode, char *mode, int *writing)
 	    mode[ix++] = '+';
 	}
     }
+#ifdef PERLIO_BINARY_AND_TEXT_DIFFERENT_AND_EFFECTIVE
     if (rawmode & O_BINARY)
 	mode[ix++] = 'b';
+#endif
     mode[ix] = '\0';
     return ptype;
 }
@@ -2529,6 +2531,7 @@ PerlIOUnix_oflags(const char *mode)
 	    oflags |= O_WRONLY;
 	break;
     }
+#ifdef PERLIO_BINARY_AND_TEXT_DIFFERENT_AND_EFFECTIVE
     if (*mode == 'b') {
 	oflags |= O_BINARY;
 	oflags &= ~O_TEXT;
@@ -2540,14 +2543,13 @@ PerlIOUnix_oflags(const char *mode)
 	mode++;
     }
     else {
-#ifdef PERLIO_USING_CRLF
 	/*
 	 * If neither "t" nor "b" was specified, open the file
 	 * in O_BINARY mode.
 	 */
 	oflags |= O_BINARY;
-#endif
     }
+#endif
     if (*mode || oflags == -1) {
 	SETERRNO(EINVAL, LIB_INVARG);
 	oflags = -1;
-- 
1.9.2

p5pRT · 2014-04-28T01:21:57Z

From @tonycoz

On Sun, Apr 27, 2014 at 08:55:33PM -0400, Jarkko Hietaniemi wrote:

On Sunday-201404-27, 20:53, Tony Cook via RT wrote:

I think a better check would be that O_BINARY or O_TEXT is non-zero.

Hmmm. I see. Well. But. If O_BINARY or O_TEXT are zero how they
can be tested with bit-and?

#if O_BINARY && O_TEXT && O_BINARY != O_TEXT

Depending on which test, I'd just check that O_TEXT or O_BINARY is
non-zero:

#if O_BINARY
if (rawmode & O_BINARY)
mode[ix++] = 'b';
#endif

The other hunk could be simply:

/* or maybe && */
#if O_TEXT || O_BINARY
if (*mode == 'b') {
oflags |= O_BINARY;
oflags &= ~O_TEXT;
mode++;
} else if (*mode == 't') {
oflags |= O_TEXT;
oflags &= ~O_BINARY;
mode++;
}
else {
/*
* If neither "t" nor "b" was specified, open the file
* in O_BINARY mode.
*/
oflags |= O_BINARY;
}
#endif

or it could be:

#if O_TEXT || O_BINARY
if (*mode == 'b') {
#if O_BINARY
oflags |= O_BINARY;
#endif
#if O_TEXT
oflags &= ~O_TEXT;
#endif
mode++;
} else if (*mode == 't') {
#if O_TEXT
oflags |= O_TEXT;
#endif
#if O_BINARY
oflags &= ~O_BINARY;
#endif
mode++;
}
else {
#if O_BINARY
/*
* If neither "t" nor "b" was specified, open the file
* in O_BINARY mode.
*/
oflags |= O_BINARY;
#endif
}
#endif

but I think that's going too far to satisfy a tool.

Tony

p5pRT · 2014-04-28T01:24:14Z

From @tonycoz

On Sun Apr 27 18:20:43 2014, jhi wrote:

On Sunday-201404-27, 20:53, Tony Cook via RT wrote:

On Sat Apr 26 12:01:06 2014, jhi wrote:

In non-Windows platforms the O_BINARY and O_TEXT are defined, but
with
both probably as zeros, which is not very useful if they are being
tested with bit-and. Leads into unreachable code.

Attached.

I suspect this patch is incorrect, at least for Cygwin.

O_TEXT and O_BINARY on Cygwin do have a meaning, but
PERLIO_USING_CRLF is explicitly undefined.

I think a better check would be that O_BINARY or O_TEXT is non-zero.

I can't think of a perl level check to demonstrate the incorrectness.

I think honestly this mess should be a Configure level test. Though,
of
course, in exotic places where there's no Configure, we are back to
square one.

But a new patch attached: I now introduced a new PERLIO_ define.

That's sane, added as a 5.21.1 blocker.

Tony

p5pRT · 2014-05-06T12:43:43Z

From @jhi

But a new patch attached: I now introduced a new PERLIO_ define.

Just a patch regen to have a pithy summary line.

p5pRT · 2014-05-06T12:43:43Z

From @jhi

0001-O_BINARY-versus-O_TEXT.patch

From 2e7f8495afbe8698f07ebbfeb21e9a1962bb50bb Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Tue, 6 May 2014 08:35:02 -0400
Subject: [PATCH] O_BINARY versus O_TEXT.

O_BINARY and O_TEXT can be either different (mostly in Windowsy platforms,
and in some hybrids, and then they can be either effective or no-ops);
or equal (in UNIXy platforms, no-ops).  If they are no-ops, and especially
if one or both of them are zeros, one cannot even test for them (bit-and),
without introducing dead/non-sensical code.
---
 op.c     |  4 ++++
 perl.h   | 10 +++++++++-
 perlio.c |  6 ++++--
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/op.c b/op.c
index 716c684..36f0b39 100644
--- a/op.c
+++ b/op.c
@@ -8565,6 +8565,7 @@ Perl_ck_anoncode(pTHX_ OP *o)
 static void
 S_io_hints(pTHX_ OP *o)
 {
+#ifdef PERLIO_BINARY_AND_TEXT_DIFFERENT_AND_EFFECTIVE
     HV * const table =
 	PL_hints & HINT_LOCALIZE_HH ? GvHV(PL_hintgv) : NULL;;
     if (table) {
@@ -8590,6 +8591,9 @@ S_io_hints(pTHX_ OP *o)
 		o->op_private |= OPpOPEN_OUT_CRLF;
 	}
     }
+#else
+    PERL_UNUSED_ARG(o);
+#endif
 }
 
 OP *
diff --git a/perl.h b/perl.h
index 6da39f3..6a43b12 100644
--- a/perl.h
+++ b/perl.h
@@ -5639,7 +5639,7 @@ int flock(int fd, int op);
 #endif
 
 #if O_TEXT != O_BINARY
-    /* If you have different O_TEXT and O_BINARY and you are a CLRF shop,
+    /* If you have different O_TEXT and O_BINARY and you are a CRLF shop,
      * that is, you are somehow DOSish. */
 #   if defined(__HAIKU__) || defined(__VOS__) || defined(__CYGWIN__)
     /* Haiku has O_TEXT != O_BINARY but O_TEXT and O_BINARY have no effect;
@@ -5653,6 +5653,14 @@ int flock(int fd, int op);
     /* If you really are DOSish. */
 #      define PERLIO_USING_CRLF 1
 #   endif
+#   if O_TEXT != 0 && O_BINARY != 0
+#     if !defined(__HAIKU__)
+        /* If you have O_TEXT different from your O_BINARY and
+         * they are both not zero (being zero would make testing
+         * with bit-and interesting) and they have an effect. */
+#       define PERLIO_BINARY_AND_TEXT_DIFFERENT_AND_EFFECTIVE
+#     endif
+#   endif
 #endif
 
 #ifdef I_LIBUTIL
diff --git a/perlio.c b/perlio.c
index 653c7c4..437fb78 100644
--- a/perlio.c
+++ b/perlio.c
@@ -199,8 +199,10 @@ PerlIO_intmode2str(int rawmode, char *mode, int *writing)
 	    mode[ix++] = '+';
 	}
     }
+#ifdef PERLIO_BINARY_AND_TEXT_DIFFERENT_AND_EFFECTIVE
     if (rawmode & O_BINARY)
 	mode[ix++] = 'b';
+#endif
     mode[ix] = '\0';
     return ptype;
 }
@@ -2529,6 +2531,7 @@ PerlIOUnix_oflags(const char *mode)
 	    oflags |= O_WRONLY;
 	break;
     }
+#ifdef PERLIO_BINARY_AND_TEXT_DIFFERENT_AND_EFFECTIVE
     if (*mode == 'b') {
 	oflags |= O_BINARY;
 	oflags &= ~O_TEXT;
@@ -2540,14 +2543,13 @@ PerlIOUnix_oflags(const char *mode)
 	mode++;
     }
     else {
-#ifdef PERLIO_USING_CRLF
 	/*
 	 * If neither "t" nor "b" was specified, open the file
 	 * in O_BINARY mode.
 	 */
 	oflags |= O_BINARY;
-#endif
     }
+#endif
     if (*mode || oflags == -1) {
 	SETERRNO(EINVAL, LIB_INVARG);
 	oflags = -1;
-- 
1.9.2

p5pRT · 2014-06-04T05:24:32Z

From @tonycoz

On Tue May 06 05:43:43 2014, jhi wrote:

But a new patch attached: I now introduced a new PERLIO_ define.

Just a patch regen to have a pithy summary line.

Applied as 38d9694 by Jarkko on the 29th.

Tony

p5pRT · 2014-06-04T05:24:33Z

@tonycoz - Status changed from 'open' to 'resolved'

p5pRT closed this as completed Jun 4, 2014

p5pRT added Severity Low hasPatch labels Oct 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PATCH] Coverity: don't try using O_BINARY and O_TEXT in non-Windows platforms #13771

[PATCH] Coverity: don't try using O_BINARY and O_TEXT in non-Windows platforms #13771

p5pRT commented Apr 26, 2014

p5pRT commented Apr 26, 2014

p5pRT commented Apr 26, 2014

p5pRT commented Apr 28, 2014

p5pRT commented Apr 28, 2014

p5pRT commented Apr 28, 2014

p5pRT commented Apr 28, 2014

p5pRT commented Apr 28, 2014

p5pRT commented Apr 28, 2014

p5pRT commented Apr 28, 2014

p5pRT commented May 6, 2014

p5pRT commented May 6, 2014

p5pRT commented Jun 4, 2014

p5pRT commented Jun 4, 2014

[PATCH] Coverity: don't try using O_BINARY and O_TEXT in non-Windows platforms #13771

[PATCH] Coverity: don't try using O_BINARY and O_TEXT in non-Windows platforms #13771

Comments

p5pRT commented Apr 26, 2014

p5pRT commented Apr 26, 2014

From @jhi

p5pRT commented Apr 26, 2014

From @jhi

p5pRT commented Apr 28, 2014

From @tonycoz

p5pRT commented Apr 28, 2014

p5pRT commented Apr 28, 2014

From @jhi

p5pRT commented Apr 28, 2014

From @jhi

p5pRT commented Apr 28, 2014

From @jhi

p5pRT commented Apr 28, 2014

From @tonycoz

p5pRT commented Apr 28, 2014

From @tonycoz

p5pRT commented May 6, 2014

From @jhi

p5pRT commented May 6, 2014

From @jhi

p5pRT commented Jun 4, 2014

From @tonycoz

p5pRT commented Jun 4, 2014