Opened 8 months ago

Last modified 8 months ago

#1465 new defect

configure: use -iquote for $ngx_module_incs

Reported by: foonlyboy@… Owned by:
Priority: minor Milestone:
Component: other Version: 1.13.x
Keywords: Cc:
uname -a: Darwin pro.local 16.7.0 Darwin Kernel Version 16.7.0: Mon Nov 13 21:56:25 PST 2017; root:xnu-3789.72.11~1/RELEASE_X86_64 x86_64
nginx -V: nginx-1.13.8

Description

  • nginx-1.13.8
  • Darwin pro.local 16.7.0 Darwin Kernel Version 16.7.0: Mon Nov 13 21:56:25 PST 2017; root:xnu-3789.72.11~1/RELEASE_X86_64 x86_64

I found a problem when trying to compile ngx_brotli on macOS/OSX
which might be rooted in nginx/configure (or at least needs some help there)

see also:

./configure --add-module=../ngx_brotli
make

---

in ngx_brotli/src/ngx_http_brotli_filter_module.c:

#include <brotli/encode.h>

pulls in </opt/local/include/brotli/encode.h>
instead of "../ngx_brotli/deps/brotli/include/brotli/encode.h"
(I have installed "brotli" from macports which installs an incompatible version of brotli/encode.h)

This clearly is a bug in ngx_brotli, which should read:

#include "brotli/encode.h"

But for this to have the desired efect, the CFLAGS have to be changed from:

-I ../ngx_brotli/deps/brotli/include

to

-iquote ../ngx_brotli/deps/brotli/include

as of my first guess the include path flag comes from
ngx_brotli/config:

ngx_module_incs="$brotli/include"

(please excuse, I'm not yet that dig deep in the nginx module config api)

I think this should read something like:

ngx_module_incs_quote="$brotli/include"

which should generate CFLAGS

-iquote ../ngx_brotli/deps/brotli/include

I patched ngx_brotli/src/ngx_http_brotli_filter_module.c

from:

#include <brotli/encode.h>

to (angle brackets replaced by quotes):

#include "brotli/encode.h"

I then manually compiled the file,
fisrt using the CFLAGS as given, aka:

 -I ../ngx_brotli/deps/brotli/include

then modifing CFLAGS to -iquote

-iquote ../ngx_brotli/deps/brotli/include

first compile fails:

nginx-1.13.8 $ sh -x c-I
+ /usr/bin/cc -c -pipe -O -Wall -Wextra -Wpointer-arith -Wconditional-uninitialized -Wno-unused-parameter -Wno-deprecated-declarations -Werror -g -Wno-error -Wno-deprecated-declarations -I src/core -I src/event -I src/event/modules -I src/os/unix -I /opt/local/include -I /usr/local/src/openssl-1.1.0f/.openssl/include -I objs -I src/http -I src/http/modules -I src/http/v2 -I ../ngx_brotli/deps/brotli/include -I /opt/local/lib/ruby2.5/gems/2.5.0/gems/passenger-5.1.12/src -o objs/addon/src/ngx_http_brotli_filter_module.o ../ngx_brotli/src/ngx_http_brotli_filter_module.c
../ngx_brotli/src/ngx_http_brotli_filter_module.c:272:28: warning: implicit
      declaration of function 'BrotliEncoderInputBlockSize' is invalid in C99
      [-Wimplicit-function-declaration]
        ctx->brotli_ring = BrotliEncoderInputBlockSize(ctx->encoder);

my manually modified compile works (replaced -I by -iquote)

nginx-1.13.8 $ sh -x c-iquote 
+ /usr/bin/cc -c -pipe -O -Wall -Wextra -Wpointer-arith -Wconditional-uninitialized -Wno-unused-parameter -Wno-deprecated-declarations -Werror -g -Wno-error -Wno-deprecated-declarations -I src/core -I src/event -I src/event/modules -I src/os/unix -I /opt/local/include -I /usr/local/src/openssl-1.1.0f/.openssl/include -I objs -I src/http -I src/http/modules -I src/http/v2 -iquote ../ngx_brotli/deps/brotli/include -I /opt/local/lib/ruby2.5/gems/2.5.0/gems/passenger-5.1.12/src -o objs/addon/src/ngx_http_brotli_filter_module.o ../ngx_brotli/src/ngx_http_brotli_filter_module.c

---

I did set this to major priority (you might even want to raise this to critical)
because pulling in wrong header files can lead to really bad and hard to diagnose bugs.

Change History (4)

comment:1 Changed 8 months ago by foonlyboy@…

workaround:
sudo port uninstall brotli

comment:3 Changed 8 months ago by mdounin

  • Priority changed from major to minor

In nginx, we intentionally do not use quoted includes, as this will result in inconsistent handling for files located in different parts of the source tree. Instead, we use the -I compiler flag and angular includes, which is defined by POSIX and behaves as follows:

... For headers whose names are enclosed in angle brackets ( "<>" ), the header shall be searched for only in directories named in -I options and then in the usual places. ...

The problem you are facing is due to the fact that there is -I /opt/local/include, likely for the PCRE library, before the ngx_brotli module include directories. This causes /opt/local/include to be searched before the include directories in ngx_brotli sources.

Unfortunately, it is not something trivial to resolve. Switching to using quoted includes and the non-standard -iquote option is certainly not something we are going to do, as this will cause problems on other platforms and compilers.

Most trivial solution is to avoid conflicts in your build environment. In particular, nginx itself uses the ngx_... prefix for files to avoid potential conflicts with various other files.

A potential improvement might be to move include directories pulled by various libraries to the end of the list of include directories. This might have other unexpected effects though, and also not really trival to do.

comment:4 Changed 8 months ago by foonlyboy@…

Thanks for your attention,
I fully agree with your reply.

We should really throw this on macports:brotli and maybe ngx_brotli
https://trac.macports.org/ticket/55729
https://github.com/google/ngx_brotli/issues/64

But there is still an issue that I did not get resolved, yet:

I could not yet find out, how that /opt/local/include got into my -I path of ./configure.
Because when running [abbreviated, see above]

env CC=/usr/bin/cc [...] ./configure

then that /opt/local paths should not creep into the ./configure.

---

Sorry for making so much fuzz about that,
but nginx is the front facing component on my web server,
and therefor I want to have fine control, of what get's in there.

So having a wrong include of brotli in there, raised my alarms.
The nignx compile should not be attackable by bad include paths.

Maybe we should teach autoconf about this.
This is out of the reach of the nignx compile team.

Thanks for your response,
~eike


Note: See TracTickets for help on using tickets.