GUACAMOLE-663: guacd SEGVs intermittently on systems with small(er) thread stack sizes #206

rvs · 2018-12-01T00:57:26Z

No description provided.

…hread stack sizes

necouchman · 2018-12-02T02:14:52Z

This looks okay to me, but I'm not confident in my ability to review the configure.ac changes. @mike-jumper anything that needs to be fixed before this one goes in?

mike-jumper · 2018-12-03T07:49:48Z

src/guacd/daemon.c

+    if (pthread_attr_init(&default_pthread_attr) ||
+        pthread_attr_setstacksize(&default_pthread_attr, PTHREAD_STACK_SIZE) ||
+        pthread_setattr_default_np(&default_pthread_attr)) {
+       fprintf(stderr, "Couldn't set requested pthread stack size of %d\n", PTHREAD_STACK_SIZE);


Why fprintf() early rather than guacd_log() after the logging system is ready?

mike-jumper · 2018-12-03T07:54:35Z

src/guacd/daemon.c

+    if (pthread_attr_init(&default_pthread_attr) ||
+        pthread_attr_setstacksize(&default_pthread_attr, PTHREAD_STACK_SIZE) ||
+        pthread_setattr_default_np(&default_pthread_attr)) {
+       fprintf(stderr, "Couldn't set requested pthread stack size of %d\n", PTHREAD_STACK_SIZE);


"Couldn't set requested pthread stack size of %d\n"

At the time this failure occurs, the user hasn't requested anything - the stack size is set permanently at build time. This message should be rephrased such that the nature of the error can be understood by the user running guacd.

Good point. Will fix both of these.

mike-jumper · 2018-12-03T08:03:33Z

src/guacd/daemon.c

@@ -19,6 +19,12 @@

 #include "config.h"

+#ifdef PTHREAD_STACK_SIZE
+#define _GNU_SOURCE 1


I'm concerned over explicitly setting _GNU_SOURCE and using the non-portable pthread_setattr_default_np(). There's no guarantee that pthread_setattr_default_np() will be available, or that it will be available if _GNU_SOURCE is set.

Testing for these within configure would be an improvement, but the non-portability is still concerning. Is there no portable way to do this?

pthread_setattr_default_np() is available for quite sometime with _GNU_SOURCE so I don't think this should be that big of a concern. To make it reliable I can add testing for pthread_setattr_default_np to the configure part of this patch. Consider this done.

As for the more portable way of doing this -- the only way I know of is to add add explicit setting of the stack size right before each call of pthread_create. Please let me know if you'd prefer this way of doing it (I can see myself wrapping it up in a single function of course).

mike-jumper · 2018-12-03T08:04:52Z

src/guacd/daemon.c

+        pthread_attr_setstacksize(&default_pthread_attr, PTHREAD_STACK_SIZE) ||
+        pthread_setattr_default_np(&default_pthread_attr)) {
+       fprintf(stderr, "Couldn't set requested pthread stack size of %d\n", PTHREAD_STACK_SIZE);
+       exit(EXIT_FAILURE);


Please use 4-space indents per level. See: http://guacamole.apache.org/guac-style/#general-style

Got it! Will fix.

mike-jumper · 2018-12-03T08:05:57Z

configure.ac

+# pthread stack size
+AC_ARG_WITH(pthread_stack,
+            [AS_HELP_STRING([--with-pthread_stack=<size in bytes>],
+                            [explicitly set pthread stack size (8MB is recommended)])


This may be confusing as "8MB" is not a value accepted by this option.

Would you prefer it to read "8388608 (8MB)"

That would probably be better (to avoid users trying things like --with-pthread_stack=8MB), though this may no longer be necessary based on your latest suggestion to simplify everything. I agree that it doesn't make sense to have a low-level option to tweak the stack size if it's known that the stack size absolutely must be at least 8MB.

mike-jumper · 2018-12-03T08:09:13Z

configure.ac

+if test "x$ac_cv_lib_pthread_pthread_create" = xyes; then
+        AC_MSG_CHECKING([whether default pthread stack is larger than 8MB])
+        ac_save_libs="$LIBS"
+        LIBS="$LIBS -lpthread"


Not all platforms use libpthread for POSIX threads. Some have this built into libc. If necessary, you might be able to use $PTHREAD_LIBS which is set by an earlier test:

guacamole-server/configure.ac

Lines 61 to 64 in bbb6afa

# libpthread

AC_CHECK_LIB([pthread], [pthread_create], [PTHREAD_LIBS=-lpthread

AC_DEFINE([HAVE_LIBPTHREAD],,

[Whether libpthread was found])])

I'm not sure I follow this comment -- this is exactly why I'm using.

What I mean is the failure of this test (whether pthread_create() is defined within libpthread) does not mean pthread_create() is not present at all, nor that the test for pthread_setattr_default_np() need not be performed.

This test is essentially a check for whether libpthread needs to be linked in. If it fails, pthread_create(), etc. are still used; POSIX threads are then simply assumed to be part of libc.

mike-jumper · 2018-12-03T08:09:28Z

configure.ac

+            ],pthread_stack_size=$withval
+              AC_DEFINE_UNQUOTED([PTHREAD_STACK_SIZE], [$pthread_stack_size], [pthread stack size (8MB is recommended)])
+)
+if test "x$ac_cv_lib_pthread_pthread_create" = xyes; then


Where does ac_cv_lib_pthread_pthread_create come from?

See the comment above. It is automagically created by autoconf because of the test you quoted (testing for pthread_create in a library called pthread).

mike-jumper · 2018-12-03T08:12:44Z

configure.ac

+          [AC_MSG_RESULT([no])
+           AC_MSG_WARN([
+  --------------------------------------------
+   Default pthread stack size is less than 8MB


Do we know for certain that 8MB satisfies our stack requirements?

Yes we do. This is the default with glibc.

mike-jumper · 2018-12-03T08:15:29Z

configure.ac

+        AC_MSG_CHECKING([whether default pthread stack is larger than 8MB])
+        ac_save_libs="$LIBS"
+        LIBS="$LIBS -lpthread"
+        AC_RUN_IFELSE([AC_LANG_SOURCE([[


The documentation for AC_RUN_IFELSE states:

Avoid running test programs if possible, because this prevents people from configuring your package for cross-compiling.

Is there any other way to achieve this which would not hurt cross-compilation?

I honestly don't know if there is. I couldn't come up with one. That said -- this is basically a runtime check for a warning message. As such I can remove it (while leaving the rest of the patch). It would, however, be very unfortunate to do so since in my personal example it cost me a day to figure out that it was the stack size that was killing guacd on alpine. Anybody building guacd in the future on a similar platform will surely appreciate getting this type of a message early.

ON THE OTHER HAND -- if you're willing to make it a firm guarantee -- not an option that this size of a stack is alway requested -- perhaps we can simplify this patch quite a bit. Is this something that would be better discussed on the mailing list?

ON THE OTHER HAND -- if you're willing to make it a firm guarantee -- not an option that this size of a stack is alway requested -- perhaps we can simplify this patch quite a bit.

Making this a firm guarantee sounds like the best idea, particularly since you've demonstrated things simply don't work unless the stack size is at least 8MB. I'd suggest:

If default stack size is less than 8MB, guacd should attempt to set the default stack size to 8MB.

If the default stack size cannot be set (either because setting the attribute fails or pthread_setattr_default_np() is unavailable, a warning should be logged.

Is this something that would be better discussed on the mailing list?

If there are other angles to consider, hopping onto the mailing list is always a good thing.

Ping @rvs - Any progress on this?

allquixotic · 2019-05-20T13:20:16Z

My ulimit -s returns 8192 (I think it's Kbytes, so 8 MB) and I get general protection faults in session guacds (not the parent guacd, though) somewhat intermittently. I set it to 65536 (for both hard and soft limit) and I still get intermittent GPFs. It's an "error:0 in libc-2.2.7.so" which could very well be a stack overflow, or it might be something else. Not sure.

Would setting ulimit -s 65536 before running guacd potentially fix this problem while this code is still being updated/reviewed? If so, I guess having this problem means my build either has a stack memory leak, or guacd actually needs much more than 8 MiB of stack to be stable, or ulimit -s isn't applying to guacd.

Note: I am not using a systemd start script to launch guacd. I'm just typing guacd directly as root.

OS: Ubuntu 18.04 64-bit
guacamole version: 1.0.0 compiled from source (using Ubuntu's freerdp-1.1)
Using RDP with xrdp/xorgxrdp (the xrdp bit works fine, directly connecting with Windows Remote Desktop is 100% stable and never crashes)

mike-jumper · 2019-05-20T13:45:45Z

@allquixotic, please use the dev@ or user@ mailing list if you have questions:

http://guacamole.apache.org/support/#mailing-lists

There are no known issues with the behavior you describe, and it's unlikely that what you are seeing is related to this change. It's also unlikely that a bug as fundamental as complete instability in the RDP support exists, and more likely that the cause is something else:

http://guacamole.apache.org/faq/#probably-not-a-bug

There are other factors which may cause a segfault, including accidentally mixing binaries from different guacamole-server builds. If there is a bug causing what you're seeing, it's worth chasing down, but this pull request is not the place for that.

GUACAMOLE-663: guacd SEGVs intermittently on systems with small(er) t…

5759200

…hread stack sizes

mike-jumper requested changes Dec 3, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GUACAMOLE-663: guacd SEGVs intermittently on systems with small(er) thread stack sizes #206

GUACAMOLE-663: guacd SEGVs intermittently on systems with small(er) thread stack sizes #206

rvs commented Dec 1, 2018

necouchman commented Dec 2, 2018

mike-jumper Dec 3, 2018

mike-jumper Dec 3, 2018

rvs Dec 4, 2018

mike-jumper Dec 3, 2018

rvs Dec 4, 2018

mike-jumper Dec 3, 2018

rvs Dec 4, 2018

mike-jumper Dec 3, 2018

rvs Dec 4, 2018

mike-jumper Jan 6, 2019

mike-jumper Dec 3, 2018

rvs Dec 4, 2018

mike-jumper Jan 6, 2019

mike-jumper Dec 3, 2018

rvs Dec 4, 2018

mike-jumper Dec 3, 2018

rvs Dec 4, 2018

mike-jumper Dec 3, 2018

rvs Dec 4, 2018

mike-jumper Jan 6, 2019

necouchman Mar 11, 2019

allquixotic commented May 20, 2019 •

edited

mike-jumper commented May 20, 2019

	# libpthread
	AC_CHECK_LIB([pthread], [pthread_create], [PTHREAD_LIBS=-lpthread
	AC_DEFINE([HAVE_LIBPTHREAD],,
	[Whether libpthread was found])])

GUACAMOLE-663: guacd SEGVs intermittently on systems with small(er) thread stack sizes #206

Are you sure you want to change the base?

GUACAMOLE-663: guacd SEGVs intermittently on systems with small(er) thread stack sizes #206

Conversation

rvs commented Dec 1, 2018

necouchman commented Dec 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allquixotic commented May 20, 2019 • edited

mike-jumper commented May 20, 2019

allquixotic commented May 20, 2019 •

edited