Skip to content
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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
40 changes: 40 additions & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,46 @@ AC_SUBST([COMMON_SSH_INCLUDE], '-I$(top_srcdir)/src/common-ssh')
AC_SUBST([TERMINAL_LTLIB], '$(top_builddir)/src/terminal/libguac_terminal.la')
AC_SUBST([TERMINAL_INCLUDE], '-I$(top_srcdir)/src/terminal $(PANGO_CFLAGS) $(PANGOCAIRO_CFLAGS) $(COMMON_INCLUDE)')

# 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)])
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

],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
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does ac_cv_lib_pthread_pthread_create come from?

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

AC_MSG_CHECKING([whether default pthread stack is larger than 8MB])
ac_save_libs="$LIBS"
LIBS="$LIBS -lpthread"
Copy link
Contributor

Choose a reason for hiding this comment

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

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])])

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

AC_RUN_IFELSE([AC_LANG_SOURCE([[
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. If default stack size is less than 8MB, guacd should attempt to set the default stack size to 8MB.
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping @rvs - Any progress on this?

#include <pthread.h>

int main() {
pthread_attr_t default_pthread_attr;
size_t stacksize;

#ifdef PTHREAD_STACK_SIZE
return PTHREAD_STACK_SIZE < 8*1024*1024;
#else
pthread_attr_init(&default_pthread_attr);
pthread_attr_getstacksize(&default_pthread_attr, &stacksize);
return stacksize < 8*1024*1024;
#endif
}

]])],
[AC_MSG_RESULT([yes])],
[AC_MSG_RESULT([no])
AC_MSG_WARN([
--------------------------------------------
Default pthread stack size is less than 8MB
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we do. This is the default with glibc.

You may want to manually set it with --with-pthread_stack option
--------------------------------------------])
])
LIBS="$ac_save_libs"
fi


# Init directory
AC_ARG_WITH(init_dir,
[AS_HELP_STRING([--with-init-dir=<path>],
Expand Down
17 changes: 17 additions & 0 deletions src/guacd/daemon.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@

#include "config.h"

#ifdef PTHREAD_STACK_SIZE
#define _GNU_SOURCE 1
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

#include <pthread.h>
#undef _GNU_SOURCE
#endif

#include "conf.h"
#include "conf-args.h"
#include "conf-file.h"
Expand Down Expand Up @@ -275,6 +281,17 @@ int main(int argc, char* argv[]) {
/* General */
int retval;

#ifdef PTHREAD_STACK_SIZE
/* Set default stack size */
pthread_attr_t default_pthread_attr;
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

"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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Will fix both of these.

exit(EXIT_FAILURE);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it! Will fix.

}
#endif

/* Load configuration */
guacd_config* config = guacd_conf_load();
if (config == NULL || guacd_conf_parse_args(config, argc, argv))
Expand Down