1547193 - _environ ../../gfx/angle/targets/angle_common/system_utils_posix.o (symbol scope specifies local binding)

Status: RESOLVED FIXED

(Core :: Graphics, defect, P5)

Description

[Petr Sumbera]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

Solaris issue related to Bug 1546440. But now the issue is during linking.

/usr/bin/g++ -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wc++1z-compat -Wduplicated-cond -Wimplicit-fallthrough -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=coverage-mismatch -Wno-error=free-nonheap-object -Wformat -Wformat-overflow=2 -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -O -fno-omit-frame-pointer -funwind-tables -fPIC -shared -Wl,-h,libxul.so -o libxul.so @/builds/psumbera/FIREFOX/obj-sparc64-sun-solaris2.11/toolkit/library/libxul_so.list -lpthread -fstack-protector-strong -Wl,-z,text -L/builds/psumbera/FIREFOX/obj-sparc64-sun-solaris2.11/dist/bin ../../security/nss/lib/crmf/crmf_crmf/libcrmf.a ../../js/src/build/libjs_static.a /builds/psumbera/FIREFOX/obj-sparc64-sun-solaris2.11/sparcv9-sun-solaris/debug/libgkrust.a ../../config/external/nspr/pr/libnspr4.so ../../config/external/nspr/libc/libplc4.so ../../config/external/nspr/ds/libplds4.so ../../config/external/lgpllibs/liblgpllibs.so ../../security/nss/lib/nss/nss_nss3/libnss3.so ../../security/nss/lib/util/util_nssutil3/libnssutil3.so ../../security/nss/lib/smime/smime_smime3/libsmime3.so ../../config/external/sqlite/libmozsqlite3.so ../../security/nss/lib/ssl/ssl_ssl3/libssl3.so ../../widget/gtk/mozgtk/stub/libmozgtk_stub.so -lsocket -L/usr/lib/sparcv9 -licui18n -licuuc -licudata -lpthread -lffi -lz -lm -lposix4 -ldl -lnsl -lsocket -lfreetype -lfontconfig -lXrender -levent -ldbus-glib-1 -ldbus-1 -lgobject-2.0 -lglib-2.0 -lpangocairo-1.0 -lpango-1.0 -latk-1.0 -lcairo-gobject -lcairo -lgdk_pixbuf-2.0 -lgio-2.0 -lX11-xcb -lxcb-shm -lxcb -lX11 -lXext -lpangoft2-1.0 -lXt -lgthread-2.0 -lelf -ldemangle
Undefined first referenced
symbol in file
_environ ../../gfx/angle/targets/angle_common/system_utils_posix.o (symbol scope specifies local binding)
ld: fatal: symbol referencing errors

--

I wonder why execv() cannot be used instead of execve() (to avoid usage of environ at all)?

From Solaris man page exec(2):

int execve(const char *path, char *const argv[],
char *const envp[]);
..

The envp argument is an array of character pointers to null-terminated
strings. These strings constitute the environment for the new process
image. The envp array is terminated by a null pointer. For execl(),
execv(), execvp(), and execlp(), the C-language run-time start-off rou-
tine places a pointer to the environment of the calling process in the
global object extern char **environ, and it is used to pass the envi-
ronment of the calling process to the new process image.

..

The environ array should not be accessed directly by the application.

Comment 1

[Kelsey Gilbert [:jgilbert]]

I'm not sure! I don't have a system to test on, though. Can you propose a patch?

Comment 2

[Petr Sumbera]

Please see proposed change below. If I understand it right, execve() was used so that executed process has the same environment as parent did. But that is what already execv() does by default [1].

diff -r 103069dd9f0b -r 7b23d2b94796 gfx/angle/checkout/src/common/system_utils_posix.cpp
--- a/gfx/angle/checkout/src/common/system_utils_posix.cpp      Wed Apr 24 17:15:22 2019 +0200
+++ b/gfx/angle/checkout/src/common/system_utils_posix.cpp      Fri Apr 26 09:33:33 2019 +0200
@@ -15,10 +15,6 @@
 #include <sys/wait.h>
 #include <unistd.h>

-// On BSDs (including mac), environ is not declared anywhere:
-// https://stackoverflow.com/a/31347357/912144
-extern char **environ;
-
 namespace angle
 {

@@ -170,7 +166,7 @@
         // modify either the array of pointers or the characters to which the function points, but
         // this would disallow existing correct code. Instead, only the array of pointers is noted
         // as constant.
-        execve(args[0], const_cast<char *const *>(args.data()), environ);
+        execv(args[0], const_cast<char *const *>(args.data()));
         _exit(errno);
     }

[1]

$ cat test.c
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

char* arr[] = {"bash", "-c", "echo $MYXXX", NULL};

int main()
{
  setenv("MYXXX", "hello", 1);
  execv("/usr/bin/bash", arr);
  return 0;
}
$ gcc -o test test.c
$ ./test
hello
$ truss -afe ./test 2>&1 | ggrep -e exec -e MYXXX
2644:   execve("test", 0x7FFFBFFFF948, 0x7FFFBFFFF958)  argc = 1
2644:   execve("/usr/bin/bash", 0x00501440, 0x00502FB0)  argc = 3
2644:    argv: bash -c echo $MYXXX
2644:    envp: MYXXX=hello SHELL=/bin/bash LESS=-FRX LC_MONETARY=

Comment 3

[Jan Beich]

comment 2 on https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5864c74e84799182d858e6c5874c6ada12380e9

FreeBSD is also affected but not with LLD linker. LLD is default since 11.1 aarch64, 12.0 amd64 + armv7, 13.0 i386 but our oldest supported is 11.2 atm. However, I usually test mozilla-central only on the very latest version while mozilla-beta on the very oldest supported.

https://www.freebsd.org/security/
https://man.freebsd.org/src.conf/5 (see LLD_IS_LD)

The issue is #pragma GCC visibility (bug 273336). Linux is fine because it bootlegs <stdlib.h> which marks "environ" as public. And macOS is fine because it uses -fvisibility instead of #pragma.

/usr/bin/ld: libxul.so: hidden symbol `environ' isn't defined
/usr/bin/ld: final link failed: Nonrepresentable section on output
clang-8: error: linker command failed with exit code 1 (use -v to see invocation)

$ readelf -a objdir/gfx/angle/targets/angle_common/system_utils_posix.o | fgrep environ
0000000002f5 002b00000002 R_X86_64_PC32 0000000000000000 environ - 4
43: 0000000000000000 0 NOTYPE GLOBAL HIDDEN UND environ

https://searchfox.org/mozilla-central/rev/6c9f60f8cc06/config/system-headers.mozbuild#794
https://searchfox.org/mozilla-central/rev/6c9f60f8cc06/build/moz.configure/toolchain.configure#1339-1346

Comment 4

[Jan Beich]

Landry, do you also see the linking error with 68.0b3 or did OpenBSD switch to LLD on all architectures www/mozilla-firefox supports? Assuming aarch64 and amd64 are already on LLD I don't know about i386.

Comment 5

[Jan Beich]

Martin, does NetBSD use LLD on any architecture?

Comment 6

[Martin Husemann]

(In reply to Jan Beich from comment #5)
Not by default now (but that is likely to change).

Comment 7

[Landry Breuil (:gaston)]

firefox isnt built on aarch64 (i have no hw and i dont care, someone will have to do the work)

i386 uses lld since january (so that includes 6.5), cf http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/share/mk/bsd.own.mk.diff?r1=1.193&r2=1.194&f=h

Comment 8

[Landry Breuil (:gaston)]

I'm a bit confused now, as 68.0b3 built fine on i386 but failed on amd64 when linking libxul:

ld: error: undefined symbol: environ
>>> referenced by system_utils_posix.cpp:173 (/usr/obj/ports/firefox-68.0beta3/firefox-68.0/gfx/angle/checkout/src/common/system_utils_posi
x.cpp:173)
>>>               ../../gfx/angle/targets/angle_common/system_utils_posix.o:(angle::RunApp(std::__1::vector<char const*, std::__1::allocato
r<char const*> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*, std::__1::basic_string<cha
r, std::__1::char_traits<char>, std::__1::allocator<char> >*, int*))
c++: error: linker command failed with exit code 1 (use -v to see invocation)

Comment 9

[Paul Adenot (:padenot)]

Attachment: Bug 1547193 - Unbreak on platforms without GeckoProfiler after bug 1552530. r=padenot

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89e071495832
Unbreak on platforms without GeckoProfiler after bug 1552530.

Comment 11

[Jan Beich]

Sorry, comment 10 is for bug 1553900.

Comment 12

[Jan Beich]

Jeff, can you help landing comment 2 or https://hg.mozilla.org/try/rev/57aa50a6d7d5?

Alternatively, environ can be marked with __attribute__((visibility("default"))) but that would be too Mozilla-specific. ANGLE_EXPORT or ANGLE_PLATFORM_EXPORT cannot be used:

In file included from gfx/angle/checkout/src/common/system_utils_posix.cpp:10:
gfx/angle/checkout/include/platform/Platform.h:262:8: error: visibility does not match previous
      declaration
struct ANGLE_PLATFORM_EXPORT PlatformMethods
       ^
gfx/angle/checkout/include/platform/Platform.h:24:50: note: expanded from macro
      'ANGLE_PLATFORM_EXPORT'
#    define ANGLE_PLATFORM_EXPORT __attribute__((visibility("default")))
                                                 ^
config/gcc_hidden.h:6:13: note: previous attribute is here
#pragma GCC visibility push(hidden)
            ^
1 error generated.

Comment 13

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/89e071495832

Comment 14

[Jan Beich]

I can still reproduce: both on mozilla-central and mozilla-beta. Let's escalate priority because Firefox 68 is going to be the next ESR. Patching downstream is PITA as ESR is also used by Thunderbird, Tor Browser and sometimes other projects (e.g., SeaMonkey, Waterfox).

[Tracking Requested - why for this release]:
Broken build on many Tier3 platforms e.g., Solaris, FreeBSD 11, NetBSD. Can be worked around by changing to a less strict linker but has limitations: unusable on big-endian architectures (sparc64, powerpc64), requires recent LLD version on 32-bit x86. Also, --enable-release defaults to BFD linker even if LLD is available.

Comment 15

[Kelsey Gilbert [:jgilbert]]

If you want it fixed faster, you can always upstream it yourself and direct me at the cset.

Comment 16

[Jan Beich]

(In reply to Jeff Gilbert [:jgilbert] from comment #15)

If you want it fixed faster, you can always upstream it yourself and direct me at the cset.

I can't. Upstreaming to Google projects requires Google Account (to sign CLA, interact with Gerrit) which is impossible to sign up for via Tor.
https://lists.torproject.org/pipermail/tor-talk/2012-October/025923.html

Comment 17

[Kelsey Gilbert [:jgilbert]]

https://chromium-review.googlesource.com/c/angle/angle/+/1646887

Comment 18

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1547193 - ANGLE cherry-pick environ solaris fix.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64b54fb36568
ANGLE cherry-pick environ solaris fix. r=lsalzman

Comment 20

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/64b54fb36568

Comment 21

[Jan Beich]

Comment on attachment 9070407 [details]
Bug 1547193 - ANGLE cherry-pick environ solaris fix.

Beta/Release Uplift Approval Request

• User impact if declined: Broken build on Tier3 platforms e.g., Solaris, FreeBSD 11, NetBSD.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Can only break build on Linux and macOS. execv() behavior is standardized:

For those forms not containing an envp pointer (execl(), execv(), execlp(), and execvp()), the environment for the new process image shall be taken from the external variable environ in the calling process.
https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html

• String changes made/needed:

Comment 22

[Jan Beich]

Jeff, do you plan to merge bug 1536672 to Firefox 68? If not the patch here have to be rebased due to conflicts in gfx/angle/checkout/out/gen/angle/id/commit.h and gfx/angle/cherry_picks.txt

Comment 23

[Jan Beich]

Attachment: Firefox 68 (ESR) version

Comment 24

[Julien Cristau [:jcristau] (back April 22)]

bug 1536672 doesn't look like an uplift candidate.

Comment 25

[Jan Beich]

firefox-68 branch in https://github.com/mozilla/angle also needs to cherry-pick the fix. Otherwise, future backports (e.g., security) may accidentally drop it due to revendoring.

Comment 26

[Kelsey Gilbert [:jgilbert]]

Yeah, we actually land fixes first to github.com/mozilla/angle, then vendor those into the tree. We never take gecko-only patches directly into gfx/angle anymore.

Comment 27

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Bug-1547193-ANGLE-cherry-pick-environ-solaris-fix.-r.patch

Beta/Release Uplift Approval Request

• User impact if declined: Tier-3 build failures
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Build fix, patch from 69, cherry-picked from upstream.
• String changes made/needed: none

Comment 28

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9072036 [details] [diff] [review]
0001-Bug-1547193-ANGLE-cherry-pick-environ-solaris-fix.-r.patch

tier3 build fix, approved for 68.0b11

Comment 29

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6d6e29d75fbf