smbclient_state_free leads to segfault

[legsak1mbo]

I'd been working on context support but noticed that it's been added anyway which is great. However freeing the context seems to occasionally cause a segfault the next time that smbclient_opendir is called.

Example script:-

$state = smbclient_state_new();
smbclient_state_init($state, 'WORKGROUP', 'username', 'password');
$dir = smbclient_opendir($state, $url);
smbclient_closedir($state, $dir);
smbclient_state_free($state);

The first three or four requests are fine, after that it begins seemingly randomly segfaulting. Removing any mention of smbc_free_context from libsmbclient.c prevents the issue from happening but obviously isn't ideal because connections are then left open.

I'm testing this with PHP 5.5 on Apache Prefork.

Let me know if you need any more information.

[aklomp]

Quick question: do you get the same error if you run PHP in cli mode (from the commandline?) Any errors in the log? smbclient_state_free checks the return value of smbc_free_context and will print an error if any of the documented (in samba's libsmbclient.h) error states occur. Any chance you could run the php cli process under gdb to get a backtrace?

I'll try to reproduce this shortly when I have a bit more time.

[legsak1mbo]

Sorry, I should have mentioned that. No it doesn't happen in cli mode. Also, I've just found that if I run httpd manually rather than as a service then it also doesn't crash. So it seems to be linked to the init script (I'm using CentOS). I'll keep digging...

[legsak1mbo]

Confirmed. When Apache is run (as root) from the command line with:

/usr/sbin/httpd

...all works perfectly. When run with:

service httpd start

...the segfaults happen.

However, running:

/etc/init.d/httpd start

...seems to be fine.

[legsak1mbo]

Solved. It seems to require a HOME environment variable. Adding:

export HOME=/root

to /etc/init.d/httpd did it. I guess this is so that it can find smb.conf

[aklomp]

I'm fairly sure that the issue happens because the library is not properly threadsafe the PHP way. Specifically, the globals defined on line 63 and 64 should probably be guarded with PHP's ZTS macros to ensure thread safety. That whole area is something I don't know a lot about (it's all a black art as far as I can tell), but I'll try to read up on the dev docs and get a fix out.

[aklomp]

Re: fixed, if that's the cause, it would still be a very serious bug. You could check if that's really the cause by opening a shell, unsetting HOME, and running the script under the php cli.

[legsak1mbo]

Yep. Unsetting HOME caused an instant segfault.

[aklomp]

A missing $HOME seems like such a weird cause for a segfault, since I can't think of a good reason why libsmbclient has to know it. Its smb.conf is usually not in the user's homedir, and even then, testing shows that libsmbclient works without an smb.conf. Segfaulting is also such nasty behavior that I can't really imagine the cause being anything other than a bug in our project.

I tried hard to reproduce the behavior you described, but failed. Now that you've isolated a test case with just php-cli, could you maybe run it under gdb or valgrind to get a backtrace of where exactly things go wrong? Tell me if you need pointers on how to do this.

In the meantime, I followed my own hunch and committed some code on a test branch in my repo that isolates all the global variables (all two of them!) into per-thread containers. This means that each thread gets its own copy of the file and context resource lists, which might prevent race conditions or null pointer dereferencing (a likely cause of segfaults). Could you maybe compile that branch to see if it fixes the issue?

[aklomp]

Found some documentation that claims that resource list pointers don't need to be threadsafe. Makes sense. So my test branch fixes what isn't broken.

[legsak1mbo]

Backtrace below...

(gdb) bt
#0  0x0000003d0fe78900 in strlen () from /lib64/libc.so.6
#1  0x0000003d0fe78636 in strdup () from /lib64/libc.so.6
#2  0x00002aae36183e8c in ?? () from /usr/lib64/libsmbclient.so.0
#3  0x00002aae36186032 in ?? () from /usr/lib64/libsmbclient.so.0
#4  0x00002aae3618a4cc in ?? () from /usr/lib64/libsmbclient.so.0
#5  0x00002aae3618abf5 in ?? () from /usr/lib64/libsmbclient.so.0
#6  0x00002aae36170bb5 in ?? () from /usr/lib64/libsmbclient.so.0
#7  0x00002aae36170d5c in smbc_new_context () from /usr/lib64/libsmbclient.so.0
#8  0x00002aae35f166bb in zif_smbclient_state_new (ht=0, return_value=0x2aae36508a31, return_value_ptr=0x0, this_ptr=0x0, return_value_used=-16843009) at /root/php_libsmbclient/new/libsmbclient-php-master/libsmbclient.c:280
#9  0x000000000059e654 in dtrace_execute_internal (execute_data_ptr=0x2aae355183e0, fci=0x0, return_value_used=1) at /usr/src/debug/php-5.5.12/Zend/zend_dtrace.c:97
#10 0x00002aae358d1eb3 in xdebug_execute_internal () from /usr/lib64/php/modules/xdebug.so
#11 0x0000000000634293 in zend_do_fcall_common_helper_SPEC (execute_data=0x2aae355183e0) at /usr/src/debug/php-5.5.12/Zend/zend_vm_execute.h:552
#12 0x0000000000620278 in execute_ex (execute_data=0x2aae355183e0) at /usr/src/debug/php-5.5.12/Zend/zend_vm_execute.h:363
#13 0x000000000059e761 in dtrace_execute_ex (execute_data=0x2aae355183e0) at /usr/src/debug/php-5.5.12/Zend/zend_dtrace.c:73
#14 0x00002aae358d4881 in xdebug_execute_ex () from /usr/lib64/php/modules/xdebug.so
#15 0x00000000005a0cf3 in zend_call_function (fci=0x7fff5c1c8890, fci_cache=0x7fff5c1c88e0) at /usr/src/debug/php-5.5.12/Zend/zend_execute_API.c:939
#16 0x000000000056d217 in user_stream_create_object (uwrap=0x2aae355505b0, context=<value optimized out>) at /usr/src/debug/php-5.5.12/main/streams/userspace.c:321
#17 0x000000000056d304 in user_wrapper_stat_url (wrapper=<value optimized out>, url=0x2aae355a0168 "smb://;user:pass@testserver/test/test.jpg", flags=0, ssb=0x0, context=0xfefefefefefefeff)
    at /usr/src/debug/php-5.5.12/main/streams/userspace.c:1457
#18 0x0000000000565bff in _php_stream_stat_path (path=0x2aae355a0168 "smb://;user:pass@testserver/test/test.jpg", flags=0, ssb=0x7fff5c1c8a80, context=0x0) at /usr/src/debug/php-5.5.12/main/streams/streams.c:1942
#19 0x0000000000505fd2 in php_stat (filename=0x2aae355a0168 "smb://;user:pass@testserver/test/test.jpg", filename_length=<value optimized out>, type=17, return_value=0x2aae3554f3d8) at /usr/src/debug/php-5.5.12/ext/standard/filestat.c:906
#20 0x00000000005071d8 in php_if_stat (ht=<value optimized out>, return_value=0x2aae3554f3d8, return_value_ptr=<value optimized out>, this_ptr=<value optimized out>, return_value_used=<value optimized out>)
    at /usr/src/debug/php-5.5.12/ext/standard/filestat.c:1180
#21 0x00002aae3ddde5c6 in phar_stat (ht=1, return_value=0x2aae3554f3d8, return_value_ptr=0x0, this_ptr=0x0, return_value_used=1) at /usr/src/debug/php-5.5.12/ext/phar/func_interceptors.c:1034
#22 0x000000000059e654 in dtrace_execute_internal (execute_data_ptr=0x2aae35518280, fci=0x0, return_value_used=1) at /usr/src/debug/php-5.5.12/Zend/zend_dtrace.c:97
#23 0x00002aae358d1eb3 in xdebug_execute_internal () from /usr/lib64/php/modules/xdebug.so
#24 0x0000000000634293 in zend_do_fcall_common_helper_SPEC (execute_data=0x2aae35518280) at /usr/src/debug/php-5.5.12/Zend/zend_vm_execute.h:552
#25 0x0000000000620278 in execute_ex (execute_data=0x2aae35518280) at /usr/src/debug/php-5.5.12/Zend/zend_vm_execute.h:363
#26 0x000000000059e761 in dtrace_execute_ex (execute_data=0x2aae35518280) at /usr/src/debug/php-5.5.12/Zend/zend_dtrace.c:73
#27 0x00002aae358d4881 in xdebug_execute_ex () from /usr/lib64/php/modules/xdebug.so
#28 0x00000000005adcca in zend_execute_scripts (type=8, retval=0x0, file_count=3) at /usr/src/debug/php-5.5.12/Zend/zend.c:1316
#29 0x000000000055168e in php_execute_script (primary_file=0x7fff5c1cb5b0) at /usr/src/debug/php-5.5.12/main/main.c:2506
#30 0x0000000000659b0d in do_cli (argc=2, argv=0x1186ec30) at /usr/src/debug/php-5.5.12/sapi/cli/php_cli.c:994
#31 0x000000000065a4a8 in main (argc=2, argv=0x1186ec30) at /usr/src/debug/php-5.5.12/sapi/cli/php_cli.c:1378

[aklomp]

Thanks. You may want to anonymize that.

So it's a strlen somewhere when allocating a new context. Interesting!

[legsak1mbo]

LOL, already done. I blame fatigue.

[aklomp]

So as I read the stack trace, we first call smbclient_state_new, then that calls smbc_new_context inside libsmbclient, and then after some jumping around, we get a segfault when executing strlen. The interesting thing to me is that the culprit doesn't seem to be our project, since smbc_new_context is a fairly harmless call that takes no parameters. We don't have an opportunity to slip a null pointer in.

So I dove into the Samba source code. smbc_new_context is defined in ./source3/libsmb/libsmb_context.c. The function itself does not call strlen or strdup, but it does make a call to SMBC_module_init, and that function... reads the value of the HOME environment variable. Well well well! And if it can't find that, it does a bunch of hokey fallback stuff until it eventually settles for default values. My hunch is that this error path is not well tested and might be buggy.

So from the looks of it, you found a Samba bug, for which the workaround is to make the config file easy to find by putting it in a default spot or by defining $HOME. Your fix was right all along!

[adambant]

Btw, I was having a segfault in Apache with the libsmbclient 0.4 compiled on Mac OS X 10.9.4 using Homebrew PHP54 and Homebrew Samba3.

Adding the HOME key to the Environment variables (per this thread) in
/System/Library/LaunchDaemons/org.apache.httpd.plist made the segfaults go away.

EnvironmentVariables

HOME
/etc/apache2

Now I get this
"Warning: smbclient_state_init() expects parameter 1 to be resource, boolean given" which leads me to believe the smbclient_state_new() code isn't segfaulting because of smbc_new_context anymore, but also isn't getting a valid resource returned.

with this code.
$smb_state = smbclient_state_new();
$result = smbclient_state_init($smb_state, null, null, null);

[aklomp]

The warning about the resource being a boolean occurs because smbclient_state_new() is returning false for some reason. Feeding false to smbclient_state_init() will make it complain about an unexpected parameter type. The real question is why smbclient_state_new() is returning false. An error message should have been written to your PHP logs, could you please check?

[adambant]

Yea I gathered that. Nothing in the logs.

[aklomp]

Then we're in the twilight zone. As I see it, either smbclient_state_new() returns false and prints an error, or it allocates an internal data structure and returns a resource. It can't reasonably return a boolean and not print an error. This has nothing to do with the underlying library returning NULL, since the resource return value is constructed completely by us; if the underlying library chokes somewhere, the only thing that will be NULL is state->ctx, not the returned resource.

Hmm. Could you please check with a var_dump that smbclient_state_new() is indeed returning false?

[adambant]

Ok, will do some more investigation on it. Yes it is strange. I have the call in a try/catch also and it doesn't trigger the catch block. Will report back as soon as I have more info.

[aklomp]

Found this bug in Samba's Bugzilla about exactly this issue.