Skip to content

Fix: Load the os env once at startup. #1141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ghost wants to merge 2 commits into from
Closed

Fix: Load the os env once at startup. #1141

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6c5d04
Loads the env once at startup.
AlliBalliBaba Nov 6, 2024
9cf58c4
fmt
AlliBalliBaba Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 27 additions & 23 deletions frankenphp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ var (
requestChan chan *http.Request
done chan struct{}
shutdownWG sync.WaitGroup
cachedEnv map[string]string
envMu sync.RWMutex

loggerMu sync.RWMutex
logger *zap.Logger
Expand Down Expand Up @@ -331,6 +333,14 @@ func Init(options ...Option) error {
logger.Warn(`ZTS is not enabled, only 1 thread will be available, recompile PHP using the "--enable-zts" configuration option or performance will be degraded`)
}

// load the os environment once
// prevents potential segfaults if it's is accessed from anywhere else
cachedEnv = make(map[string]string, len(os.Environ()))
for _, envVar := range os.Environ() {
key, val, _ := strings.Cut(envVar, "=")
cachedEnv[key] = val
}

shutdownWG.Add(1)
done = make(chan struct{})
requestChan = make(chan *http.Request)
Expand Down Expand Up @@ -513,43 +523,37 @@ func go_putenv(str *C.char, length C.int) C.bool {
// Convert byte slice to string
envString := string(s)

envMu.Lock()
// Check if '=' is present in the string
if key, val, found := strings.Cut(envString, "="); found {
if os.Setenv(key, val) != nil {
return false // Failure
}
cachedEnv[key] = val
} else {
// No '=', unset the environment variable
if os.Unsetenv(envString) != nil {
return false // Failure
}
delete(cachedEnv, key)
Comment on lines -519 to +532
Copy link
Member

@dunglas dunglas Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep the calls to os.Setenv() and os.Unsetenv() (in addition to updating cachedEnv to also update the environment.

Copy link
Author

@ghost ghost Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try adding it back in and see if it still segfaults 👍

Copy link
Author

@ghost ghost Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah doing the actual os.Setenv() here will make the test segfault sporadically. If we keep the logic, we'll have to keep in mind that the test might fail sometimes.

Copy link
Member

@dunglas dunglas Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's weird. That shouldn't be the case with @withinboredom patch because we should use the Go version with is thread-safe. Could you copy the stack trace you get?

Copy link
Author

@ghost ghost Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to get a trace from the core dump

Copy link
Member

@dunglas dunglas Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll have to compile PHP with debug symbols :/

Copy link
Contributor

@AlliBalliBaba AlliBalliBaba Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Managed to get a dump with debug symbols, this crash seems to be related to opcache though:

#0  runtime.raise () at /usr/local/go/src/runtime/sys_linux_amd64.s:154
#1  0x000000000045d0fb in runtime.raisebadsignal (sig=11, c=0xc00078fa50) at /usr/local/go/src/runtime/signal_unix.go:987
#2  0x000000000045d547 in runtime.badsignal (sig=11, c=0xc00078fa50) at /usr/local/go/src/runtime/signal_unix.go:1096
#3  0x000000000045bda8 in runtime.sigtrampgo (sig=11, info=0xc00078fbf0, ctx=0xc00078fac0) at /usr/local/go/src/runtime/signal_unix.go:468
#4  0x0000000000481126 in runtime.sigtramp () at /usr/local/go/src/runtime/sys_linux_amd64.s:352
#5  <signal handler called>
#6  0x00007f38055478b6 in zend_mm_alloc_small (heap=0x7f376c400040, bin_num=16,
    __zend_filename=0x7f3805fcdbc0 "/usr/local/src/php/Zend/zend_opcode.c", __zend_lineno=1051, __zend_orig_filename=0x0, __zend_orig_lineno=0)     
    at /usr/local/src/php/Zend/zend_alloc.c:1312
#7  0x00007f3805548471 in zend_mm_realloc_heap (heap=0x7f376c400040, ptr=0x7f376c472000, size=272, use_copy_size=false, copy_size=240, 
    __zend_filename=0x7f3805fcdbc0 "/usr/local/src/php/Zend/zend_opcode.c", __zend_lineno=1051, __zend_orig_filename=0x0, __zend_orig_lineno=0)     
    at /usr/local/src/php/Zend/zend_alloc.c:1609
--Type <RET> for more, q to quit, c to continue without paging--
#8  0x00007f380554ac5f in _erealloc (ptr=0x7f376c472000, size=240, __zend_filename=0x7f3805fcdbc0 "/usr/local/src/php/Zend/zend_opcode.c",
    __zend_lineno=1051, __zend_orig_filename=0x0, __zend_orig_lineno=0) at /usr/local/src/php/Zend/zend_alloc.c:2634
#9  0x00007f380557d083 in pass_two (op_array=0x7f376c533ca8) at /usr/local/src/php/Zend/zend_opcode.c:1051
#10 0x00007f3805564c5f in zend_compile_func_decl (result=0x0, ast=0x7f376c519f00, toplevel=false) at /usr/local/src/php/Zend/zend_compile.c:7698    
#11 0x00007f380556d3b6 in zend_compile_stmt (ast=0x7f376c519f00) at /usr/local/src/php/Zend/zend_compile.c:10427
#12 0x00007f380556071a in zend_compile_stmt_list (ast=0x7f376c519dd0) at /usr/local/src/php/Zend/zend_compile.c:6405
#13 0x00007f380556d28b in zend_compile_stmt (ast=0x7f376c519dd0) at /usr/local/src/php/Zend/zend_compile.c:10374
#14 0x00007f380555de79 in zend_compile_if (ast=0x7f376c519f60) at /usr/local/src/php/Zend/zend_compile.c:5643
#15 0x00007f380556d368 in zend_compile_stmt (ast=0x7f376c519f60) at /usr/local/src/php/Zend/zend_compile.c:10414
#16 0x00007f380556d04a in zend_compile_top_stmt (ast=0x7f376c519f60) at /usr/local/src/php/Zend/zend_compile.c:10352
#17 0x00007f380556cf57 in zend_compile_top_stmt (ast=0x7f376c4ecb98) at /usr/local/src/php/Zend/zend_compile.c:10338
#18 0x00007f380550b718 in zend_compile (type=2) at Zend/zend_language_scanner.l:618
--Type <RET> for more, q to quit, c to continue without paging--
#19 0x00007f380550b8dc in compile_file (file_handle=0x7f3788df7360, type=8) at Zend/zend_language_scanner.l:653
#20 0x00007f38052ea915 in phar_compile_file (file_handle=0x7f3788df7360, type=8) at /usr/local/src/php/ext/phar/phar.c:3352
#21 0x00007f379a26f93c in opcache_compile_file (file_handle=0x7f3788df7360, type=8, op_array_p=0x7f3788df72f8)
    at /usr/local/src/php/ext/opcache/ZendAccelerator.c:1817
#22 0x00007f379a2713e7 in persistent_compile_file (file_handle=0x7f3788df7360, type=8) at /usr/local/src/php/ext/opcache/ZendAccelerator.c:2147
#23 0x00007f380550bb85 in compile_filename (type=8, filename=0x4048f030) at Zend/zend_language_scanner.l:704
#24 0x00007f38055d31c1 in zend_include_or_eval (inc_filename_zv=0x7f376c4143b0, type=8) at /usr/local/src/php/Zend/zend_execute.c:4946
#25 0x00007f380563842d in ZEND_INCLUDE_OR_EVAL_SPEC_CV_HANDLER () at /usr/local/src/php/Zend/zend_vm_execute.h:39836
#26 0x00007f380565df51 in execute_ex (ex=0x7f376c414020) at /usr/local/src/php/Zend/zend_vm_execute.h:60595
#27 0x00007f380565efc9 in zend_execute (op_array=0x7f376c46d000, return_value=0x0) at /usr/local/src/php/Zend/zend_vm_execute.h:61604
#28 0x00007f38055928f2 in zend_execute_scripts (type=8, retval=0x0, file_count=3) at /usr/local/src/php/Zend/zend.c:1895
#29 0x00007f38054ca91c in php_execute_script (primary_file=0x7f3788df8b00) at /usr/local/src/php/main/main.c:2528
--Type <RET> for more, q to quit, c to continue without paging--
#30 0x00000000019b1f44 in frankenphp_execute_script (file_name=0x0) at frankenphp.c:954
#31 0x00000000019afb47 in _cgo_b21416d01a02_Cfunc_frankenphp_execute_script (v=0xc00061fdd8) at /tmp/go-build/cgo-gcc-prolog:74
#32 0x000000000047f244 in runtime.asmcgocall () at /usr/local/go/src/runtime/asm_amd64.s:918
#33 0x000000c000444700 in ?? ()
#34 0x00007f3788df8c70 in ?? ()
#35 0x00007f380617e5c0 in ?? () from /usr/local/lib/libphp.so
#36 0x000000c0005d4808 in ?? ()
#37 0x00007f3788df8cd8 in ?? ()
#38 0x00000000005539e1 in crosscall2 () at /usr/local/go/src/runtime/cgo/asm_amd64.s:43
#39 0x00000000019af86a in go_frankenphp_on_thread_work (threadIndex=824640142696) at _cgo_export.c:299
#40 0x00000000019b1313 in php_thread (arg=<optimized out>) at frankenphp.c:826
#41 0x00007f3804e64144 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
--Type <RET> for more, q to quit, c to continue without paging--
#42 0x00007f3804ee47dc in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

Copy link
Contributor

@AlliBalliBaba AlliBalliBaba Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll probably forward this to php/src (see) could be related to some other issues.

Copy link
Member

@dunglas dunglas Nov 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe should we add how you got debug symbols for Docker images in this file: https://github.com/dunglas/frankenphp/blob/main/CONTRIBUTING.md#debugging-segmentation-faults-with-static-builds

This will be super useful to debug reports of crashes with Docker.

Copy link
Contributor

@AlliBalliBaba AlliBalliBaba Nov 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah probably a good idea 👍, the instructions are pretty much just copied from the dev.Dockerfile

}
envMu.Unlock()

return true // Success
}

//export go_getfullenv
func go_getfullenv(threadIndex C.uintptr_t) (*C.go_string, C.size_t) {
thread := phpThreads[threadIndex]
envMu.RLock()
defer envMu.RUnlock()
goStrings := make([]C.go_string, len(cachedEnv)*2)

env := os.Environ()
goStrings := make([]C.go_string, len(env)*2)

for i, envVar := range env {
key, val, _ := strings.Cut(envVar, "=")
k := unsafe.StringData(key)
v := unsafe.StringData(val)
thread.Pin(k)
thread.Pin(v)

goStrings[i*2] = C.go_string{C.size_t(len(key)), (*C.char)(unsafe.Pointer(k))}
goStrings[i*2+1] = C.go_string{C.size_t(len(val)), (*C.char)(unsafe.Pointer(v))}
i := 0
for key, val := range cachedEnv {
goStrings[i*2] = C.go_string{C.size_t(len(key)), thread.pinString(key)}
goStrings[i*2+1] = C.go_string{C.size_t(len(val)), thread.pinString(val)}
i++
}

value := unsafe.SliceData(goStrings)
thread.Pin(value)

return value, C.size_t(len(env))
return value, C.size_t(len(cachedEnv))
}

//export go_getenv
Expand All @@ -560,16 +564,16 @@ func go_getenv(threadIndex C.uintptr_t, name *C.go_string) (C.bool, *C.go_string
envName := C.GoStringN(name.data, C.int(name.len))

// Get the environment variable value
envValue, exists := os.LookupEnv(envName)
envMu.RLock()
envValue, exists := cachedEnv[envName]
envMu.RUnlock()
if !exists {
// Environment variable does not exist
return false, nil // Return 0 to indicate failure
}

// Convert Go string to C string
val := unsafe.StringData(envValue)
thread.Pin(val)
value := &C.go_string{C.size_t(len(envValue)), (*C.char)(unsafe.Pointer(val))}
value := &C.go_string{C.size_t(len(envValue)), thread.pinString(envValue)}
thread.Pin(value)

return true, value // Return 1 to indicate success
Expand Down
Loading