[PATCH] http: cast result to FILE *

Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR",
2019-01-10) introduced an implicit assumption that rewind, fileno, and
fflush are functions. At least on FreeBSD fileno is not, and as such
passing a void * failed.

Explicitly cast result to a FILE * when using standard functions that
may ultimately be macros.

Signed-off-by: Dan McGregor <redacted>
---
 http.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/http.c b/http.c
index 954bebf684..8b9476b151 100644
--- a/http.c
+++ b/http.c

@@ -1996,12 +1996,12 @@ static int http_request_reauth(const char *url,
 		strbuf_reset(result);
 		break;
 	case HTTP_REQUEST_FILE:
-		if (fflush(result)) {
+		if (fflush((FILE *)result)) {
 			error_errno("unable to flush a file");
 			return HTTP_START_FAILED;
 		}
-		rewind(result);
-		if (ftruncate(fileno(result), 0) < 0) {
+		rewind((FILE *)result);
+		if (ftruncate(fileno((FILE *)result), 0) < 0) {
 			error_errno("unable to truncate a file");
 			return HTTP_START_FAILED;
 		}

-- 
2.20.1

Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR",
2019-01-10) introduced an implicit assumption that rewind, fileno, and
fflush are functions. At least on FreeBSD fileno is not, and as such
passing a void * failed.

All systems tested (FreeBSD and NetBSD) that define fineo as a macro
also have a function defined. Undefine the macro on these systems so
that the function is used.

Signed-off-by: Dan McGregor <redacted>
---
 git-compat-util.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 29a19902aa..b5489bbcf2 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -764,6 +764,15 @@ char *gitstrdup(const char *s);
 extern FILE *git_fopen(const char*, const char*);
 #endif
 
+/* Some systems (the various BSDs for eaxmple) define
+ * fileno as a macro as an optimization. All systems I
+ * know about also define it as a real funcion, so use
+ * the real function to allow passing void *s to fileno.
+ */
+#ifdef fileno
+# undef fileno
+#endif
+
 #ifdef SNPRINTF_RETURNS_BOGUS
 #ifdef snprintf
 #undef snprintf

-- 
2.20.1

On Fri, Feb 08, 2019 at 08:36:21PM -0600, Dan McGregor wrote:

Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR",
2019-01-10) introduced an implicit assumption that rewind, fileno, and
fflush are functions. At least on FreeBSD fileno is not, and as such
passing a void * failed.

All systems tested (FreeBSD and NetBSD) that define fineo as a macro

OpenBSD or NetBSD? From this [1], it looks like OpenBSD fails while
NetBSD compiles ok (and fails to run some tests)

[1] https://gitlab.com/git-vcs/git-ci/pipelines/47139741/failures

also have a function defined. Undefine the macro on these systems so
that the function is used.

I don't think this is enough. At least fbsd defines this

#define    fileno(p)    (!__isthreaded ? __sfileno(p) : (fileno)(p))

so one of the two functions will be used depending on __isthreaded
flag. Your forcing to use fileno, ignoring __sfileno, is technically
not correct.

For the record, at least fbsd also defines feof, ferror, clearerr,
getc and putc in the same way. But at least I don't see how something
like feof(fp++) could cause bad side effects.

So, how about something like this? A teeny bit longer than your
version, but I think it's easier to control long term.

-- 8< --

diff --git a/Makefile b/Makefile
index 0e13a5b469..44cfc63fc4 100644
--- a/Makefile
+++ b/Makefile

@@ -433,6 +433,8 @@ all::
 #
 # Define HAVE_GETDELIM if your system has the getdelim() function.
 #
+# Define FILENO_IS_A_MACRO is fileno() is a macro, not a real function.
+#
 # Define PAGER_ENV to a SP separated VAR=VAL pairs to define
 # default environment variables to be passed when a pager is spawned, e.g.
 #

@@ -1800,6 +1802,11 @@ ifdef HAVE_WPGMPTR
 	BASIC_CFLAGS += -DHAVE_WPGMPTR
 endif
 
+ifdef FILENO_IS_A_MACRO
+	COMPAT_CFLAGS += -DFILENO_IS_A_MACRO
+	COMPAT_OBJS += compat/fileno.o
+endif
+
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK = NoThanks
 endif

diff --git a/compat/fileno.c b/compat/fileno.c
new file mode 100644
index 0000000000..7b105f4cd7
--- /dev/null
+++ b/compat/fileno.c

@@ -0,0 +1,7 @@
+#define COMPAT_CODE
+#include "../git-compat-util.h"
+
+int git_fileno(FILE *stream)
+{
+	return fileno(stream);
+}

diff --git a/config.mak.uname b/config.mak.uname
index 786bb2f913..01f62674a4 100644
--- a/config.mak.uname
+++ b/config.mak.uname

@@ -221,6 +221,7 @@ ifeq ($(uname_S),FreeBSD)
 	HAVE_BSD_KERN_PROC_SYSCTL = YesPlease
 	PAGER_ENV = LESS=FRX LV=-c MORE=FRX
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
+	FILENO_IS_A_MACRO = UnfortunatelyYes
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease

@@ -234,6 +235,7 @@ ifeq ($(uname_S),OpenBSD)
 	HAVE_BSD_KERN_PROC_SYSCTL = YesPlease
 	PROCFS_EXECUTABLE_PATH = /proc/curproc/file
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
+	FILENO_IS_A_MACRO = UnfortunatelyYes
 endif
 ifeq ($(uname_S),MirBSD)
 	NO_STRCASESTR = YesPlease

diff --git a/git-compat-util.h b/git-compat-util.h
index 29a19902aa..6573808ebd 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -1234,6 +1234,14 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
 #define getc_unlocked(fh) getc(fh)
 #endif
 
+#ifdef FILENO_IS_A_MACRO
+int git_fileno(FILE *stream);
+# ifndef COMPAT_CODE
+#  undef fileno
+#  define fileno(p) git_fileno(p)
+# endif
+#endif
+
 /*
  * Our code often opens a path to an optional file, to work on its
  * contents when we can successfully open it.  We can ignore a failure
-- 8< --

--

Duy

On Tue, Feb 12, 2019 at 8:45 PM Duy Nguyen [off-list ref] wrote:

On Fri, Feb 08, 2019 at 08:36:21PM -0600, Dan McGregor wrote:

quoted

Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR",
2019-01-10) introduced an implicit assumption that rewind, fileno, and
fflush are functions. At least on FreeBSD fileno is not, and as such
passing a void * failed.

All systems tested (FreeBSD and NetBSD) that define fineo as a macro

OpenBSD or NetBSD? From this [1], it looks like OpenBSD fails while
NetBSD compiles ok (and fails to run some tests)

According to this [2] looks like NetBSD is affected too, but only in
no-thread mode. With pthreads, fileno is not redefined as a macro.
That probably explains why it's ok from git-ci.

#define fileno(p) __sfileno(p)

#define __sfileno(p) \
    ((p)->_file == -1 ? -1 : (int)(unsigned short)(p)->_file)

[2] http://cvsweb.netbsd.org/bsdweb.cgi/src/include/stdio.h?rev=1.97&content-type=text/x-cvsweb-markup

For the record, at least fbsd also defines feof, ferror, clearerr,
getc and putc in the same way. But at least I don't see how something
like feof(fp++) could cause bad side effects.

... and __sfileno (and other friends) can cause bad side effects,
sigh. It's probably best to avoid fancy function calls.
-- 
Duy

On February 12, 2019 7:45:37 a.m. CST, Duy Nguyen [off-list ref] wrote:

On Fri, Feb 08, 2019 at 08:36:21PM -0600, Dan McGregor wrote:

quoted

Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR",
2019-01-10) introduced an implicit assumption that rewind, fileno,

and

quoted

fflush are functions. At least on FreeBSD fileno is not, and as such
passing a void * failed.

All systems tested (FreeBSD and NetBSD) that define fineo as a macro

OpenBSD or NetBSD? From this [1], it looks like OpenBSD fails while
NetBSD compiles ok (and fails to run some tests)

[1] https://gitlab.com/git-vcs/git-ci/pipelines/47139741/failures

quoted

also have a function defined. Undefine the macro on these systems so
that the function is used.

I don't think this is enough. At least fbsd defines this

#define    fileno(p)    (!__isthreaded ? __sfileno(p) : (fileno)(p))

so one of the two functions will be used depending on __isthreaded
flag. Your forcing to use fileno, ignoring __sfileno, is technically
not correct.

For the record, at least fbsd also defines feof, ferror, clearerr,
getc and putc in the same way. But at least I don't see how something
like feof(fp++) could cause bad side effects.

So, how about something like this? A teeny bit longer than your
version, but I think it's easier to control long term.

Yes, this looks pretty reasonable to me too.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.