great work comments inline
On Fri, 2010-02-26 at 11:49 +1100, Angus Salkeld wrote: > Hi > > This patch is to try and prevent people putting in thread unsafe calls into > the code. > > A couple of functions (getenv,setenv,..) can result in either a call to a new > version or the clib version > depending on if tsafe_on() has been called. It helps you by catching > pthread_create() and fork() > and turning tsafe on after a pthread_create() call and turning tsafe off > after a call to fork(). > > All the other functions call assert(0), if more of these need to be capable > of been turned > on then please tell me. > > This is just an initial post I need to test the setenv/getenv function some > more. > > -Angus > > Signed-off-by: Angus Salkeld <[email protected]> > --- > exec/Makefile.am | 2 +- > exec/main.c | 6 +- > exec/tsafe.c | 700 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > exec/tsafe.h | 43 ++++ > 4 files changed, 749 insertions(+), 2 deletions(-) > create mode 100644 exec/tsafe.c > create mode 100644 exec/tsafe.h > > diff --git a/exec/Makefile.am b/exec/Makefile.am > index a9aebc9..8ea3be5 100644 > --- a/exec/Makefile.am > +++ b/exec/Makefile.am > @@ -37,7 +37,7 @@ INCLUDES = -I$(top_builddir)/include > -I$(top_srcdir)/include $(nss_CFLAGS) $(rd > > TOTEM_SRC = coropoll.c totemip.c totemnet.c totemudp.c \ > totemrrp.c totemsrp.c totemmrp.c totempg.c \ > - crypto.c wthread.c > + crypto.c wthread.c tsafe.c > if BUILD_RDMA > TOTEM_SRC += totemiba.c > endif > diff --git a/exec/main.c b/exec/main.c > index 63a6d5f..6655b7f 100644 > --- a/exec/main.c > +++ b/exec/main.c > @@ -85,6 +85,7 @@ > #include "service.h" > #include "schedwrk.h" > #include "evil.h" > +#include "tsafe.h" > > LOGSYS_DECLARE_SYSTEM ("corosync", > LOGSYS_MODE_OUTPUT_STDERR | LOGSYS_MODE_THREADED | LOGSYS_MODE_FORK, > @@ -1239,7 +1240,7 @@ static void main_service_ready (void) > > } > > -int main (int argc, char **argv) > +int main (int argc, char **argv, char **envp) > { > const char *error_string; > struct totem_config totem_config; > @@ -1477,6 +1478,9 @@ int main (int argc, char **argv) > } > logsys_fork_completed(); > > + /* callthis after our fork() */ > + tsafe_init (envp); > + > corosync_timer_init ( > serialize_lock, > serialize_unlock, > diff --git a/exec/tsafe.c b/exec/tsafe.c > new file mode 100644 > index 0000000..0c8049f > --- /dev/null > +++ b/exec/tsafe.c > @@ -0,0 +1,700 @@ > +/* > + * Copyright (c) 2010 Red Hat, Inc. > + * > + * All rights reserved. > + * > + * Author: Angus Salkeld <[email protected]> > + * > + * This software licensed under BSD license, the text of which follows: > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions are > met: > + * > + * - Redistributions of source code must retain the above copyright notice, > + * this list of conditions and the following disclaimer. > + * - Redistributions in binary form must reproduce the above copyright > notice, > + * this list of conditions and the following disclaimer in the > documentation > + * and/or other materials provided with the distribution. > + * - Neither the name of the MontaVista Software, Inc. nor the names of its > + * contributors may be used to endorse or promote products derived from > this > + * software without specific prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS > IS" > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF > + * THE POSSIBILITY OF SUCH DAMAGE. > + */ > + > +#include <config.h> > +#include <unistd.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <math.h> > +#include <string.h> > +#include <assert.h> > +#include <time.h> > +#include <sys/types.h> > +#include <sys/socket.h> > +#include <netinet/in.h> > +#include <arpa/inet.h> > +#include <grp.h> > +#include <pwd.h> > +#include <netdb.h> > +#include <nl_types.h> > +#include <libgen.h> > +#include <dlfcn.h> > +#include <utmpx.h> > +#include <search.h> > +#include <locale.h> > +#include <dirent.h> > +#include <pthread.h> > + > +#include <tsafe.h> > + > +#ifndef RTLD_NEXT > +#define RTLD_NEXT ((void *) -1l) > +#endif /* !RTLD_NEXT */ > + > +/* > + * The point of this file is to prevent people from using functions that are > + * known not to be thread safe (see man pthreads). > + */ > + > +static int tsafe_disabled = 1; > +static int tsafe_inited = 0; > +#define CORO_ENVIRON_LEN 256 > +static char *coro_environ[CORO_ENVIRON_LEN]; Shouldn't this atleast alloc the size of the current environment? If enviornment is larger then 256 entries, there could be problems. Dynamic allocation of this variable makes more sense to me. > + > + > +void tsafe_init (char **envp) > +{ > + int i; > + printf("%s\n", __func__); > + > + for (i = 0; envp[i] != NULL; i++) { > + if (i >= (CORO_ENVIRON_LEN - 1)) { > + printf("dumping env: %s\n",coro_environ[i]); printfs in libraries are evil. :) If you want a logging function, add a tsafe_log_register() or some other api which takes a function the user can specify. If it starts unspecified, do nothing. > + } else { > + coro_environ[i] = strdup (envp[i]); > + } > + } > + for (; i < (CORO_ENVIRON_LEN - 1); i++) > + coro_environ[i] = NULL; > + > + if (i >= (CORO_ENVIRON_LEN - 1)) > + i = (CORO_ENVIRON_LEN - 1); > + coro_environ[i] = NULL; > + > + tsafe_off (); > + tsafe_inited = 1; > +} > + > +void tsafe_off (void) > +{ > + printf("%s\n", __func__); > + tsafe_disabled = 1; > + this variable should likely be protected by mutex. > } > + > +void tsafe_on (void) > +{ > + printf("%s\n", __func__); > + tsafe_disabled = 0; > +} > + > +static void* _get_real_func_(const char * func_name) > +{ > + void * func = NULL; > +#ifdef COROSYNC_BSD > + static void *handle; > + /* > + * On BSD we open the libc and retrive the pointer to "func_name". > + */ > + handle = dlopen(TCONN_LIBC_PATH, RTLD_LAZY); > + func = dlsym (handle, func_name); > +#else > + /* > + * On linux/Sun we can set func to next instance of "func_name", > + * which is the original "func_name" > + */ > + func = dlsym (RTLD_NEXT, func_name); > +#endif > + return func; > +} > + > +/* we are implementing these functions only to know when to turn tsafe > + * on and off. > + */ > +int pthread_create(pthread_t *thread, const pthread_attr_t *attr, > + void *(*start_routine) (void *), void *arg) > +{ > + static int (*real_pthread_create)(pthread_t *thread, const > pthread_attr_t *attr, > + void *(*start_routine) (void *), void *arg) = NULL; > + > + if (tsafe_inited && tsafe_disabled) { > + printf("%s\n", __func__); > + tsafe_on (); > + } > + > + if (real_pthread_create == NULL) { > + real_pthread_create = _get_real_func_ ("pthread_create"); > + } > + return real_pthread_create (thread, attr, start_routine, arg); > +} > + > +pid_t fork(void) > +{ > + static pid_t (*real_fork)(void) = NULL; > + > + if (tsafe_inited && !tsafe_disabled) { > + tsafe_off (); > + } > + tsafe_off should only be called in the event we are in the child after a fork. I recommend catching the return value of real_fork, and if it is 0, call tsafe_off. > + if (real_fork == NULL) { > + real_fork = _get_real_func_ ("fork"); > + } > + return real_fork (); > +} > + > + > + > + > +/* > + * we will allow this one as there doesn't seem to be any other option. > + * and only used in lcr > + * char *dlerror(void) > + * { > + * } > + */ > + > +/* the following functions are needed so we are either banning them or > + * re-implementing them (depending on tsafe_on). > + */ > +char *getenv(const char *name) > +{ > + static char* (*real_getenv)(const char *name) = NULL; > + int entry; > + int found = 0; > + char *eq; > + > + if (!tsafe_inited || tsafe_disabled) { > + if (real_getenv == NULL) { > + real_getenv = _get_real_func_ ("getenv"); > + } > + return real_getenv (name); > + } > + for (entry = 0; entry < CORO_ENVIRON_LEN; entry++) { > + if (coro_environ[entry] == NULL) { > + found = 0; > + break; > + } else if (strncmp(name, coro_environ[entry], strlen(name)) == > 0) { This call looks unsafe. Assume name is ENVIRONMENT_NAME_ABC and coro_environ[entry] = ENVRIONMENT_NAME_ABCDEF. In this case, the strncmp would return 0 (since it only compares at most strlen (name)). strcmp might make more sense here. > + found = 1; > + break; > + } > + } > + > + if (found) { > + eq = strchr (coro_environ[entry], '='); > + return (eq + 1); > + } > + return NULL; > +} > + > +int setenv(const char *name, const char *value, int overwrite) > +{ > + static int (*real_setenv)(const char *name, const char *value, int > overwrite) = NULL; > + int ret = 0; > + int entry; > + int found = 0; > + int entry_len = 0; > + > + if (!tsafe_inited || tsafe_disabled) { > + if (real_setenv == NULL) { > + real_setenv = _get_real_func_ ("setenv"); > + } > + return real_setenv (name, value, overwrite); > + } > + for (entry = 0; entry < CORO_ENVIRON_LEN; entry++) { > + if (coro_environ[entry] == NULL) { > + found = 0; > + break; > + } else if (strncmp(name, coro_environ[entry], strlen(name)) == > 0) { > + found = 1; > + break; > + } > + } > + if (entry == (CORO_ENVIRON_LEN - 1)) { > + /* no space, last entry must be NULL */ > + ret = -1; > + goto clean_up; > + } > + > + if (found && !overwrite) { > + /* refuse to modify */ > + ret = 0; > + goto clean_up; > + } > + > + if (found && overwrite) { > + // don't free, another thread might be referencing it. > + //free(coro_environ[entry]); > + } > + > + entry_len = strlen(name) + strlen(value) + 2; > + coro_environ[entry] = malloc(entry_len); > + if (coro_environ[entry] == NULL) { > + ret = -1; > + goto clean_up; > + } > + snprintf (coro_environ[entry], entry_len, "%s=%s", name, value); > + ret = 0; > +clean_up: > + // unlock > + return ret; > +} Might make more sense to entirely ban the use of setenv when tsafe is on (pthread_create was called). When tsafe is off, call the real_getenv function instead. > + > +int unsetenv(const char *name) > +{ > + static int (*real_unsetenv)(const char *name) = NULL; > + int entry; > + int found = 0; > + > + if (!tsafe_inited || tsafe_disabled) { > + if (real_unsetenv == NULL) { > + real_unsetenv = _get_real_func_ ("unsetenv"); > + } > + return real_unsetenv (name); > + } > + for (entry = 0; entry < CORO_ENVIRON_LEN; entry++) { > + if (coro_environ[entry] == NULL) { > + found = 0; > + break; > + } else if (strncmp(name, coro_environ[entry], strlen(name)) == > 0) { > + found = 1; > + break; > + } > + } > + > + if (found) { > + // don't free > + //free(coro_environ[entry]); > + coro_environ[entry] = NULL; > + } > + return 0; > +} > + > +int putenv(char *string) > +{ > + static int (*real_putenv)(char *string) = NULL; > + int ret = 0; > + char *eq; > + int entry; > + int found; > + > + if (!tsafe_inited || tsafe_disabled) { > + if (real_putenv == NULL) { > + real_putenv = _get_real_func_ ("putenv"); > + } > + return real_putenv (string); > + } > + for (entry = 0; entry < CORO_ENVIRON_LEN; entry++) { > + if (coro_environ[entry] == NULL) { > + found = 0; > + break; > + } else { > + eq = strchr (coro_environ[entry], '='); > + if (strncmp(string, coro_environ[entry], (eq - string)) > == 0) { > + found = 1; > + break; > + } > + } > + } > + if (entry == (CORO_ENVIRON_LEN - 1)) { > + /* no space, last entry must be NULL */ > + ret = -1; > + goto clean_up; > + } > + > + if (found) { > + // don't free > + //free(coro_environ[entry]); > + } > + coro_environ[entry] = string; > + ret = 0; > +clean_up: > + // unlock > + return ret; > +} > + > + this code between the ifdefs can be deleted. tz_set and strftime are safe in the case that tsafe = off (ie no threads are running). > +#ifdef WHEN_I_GET_TIME > +//tz_set > + > +size_t strftime(char *s, size_t max, const char *format, > + const struct tm *tm) > +{ > +} > + > +#endif > + > + > +/* all the following functions are banned and will result in an abort. > + */ > + These functions should only be banned when tsafe=on. when tsafe=off, they should be called as normal. > +char *asctime(const struct tm *tm) > +{ > + assert(0); > + return NULL; > +} > + > +char *basename(char *path) > +{ > + assert(0); > + return NULL; > +} > + > +char *catgets(nl_catd catalog, int set_number, > + int message_number, const char *message) > +{ > + assert(0); > + return NULL; > +} > + > +#ifdef _XOPEN_SOURCE > +char *crypt(const char *key, const char *salt) > +{ > + assert(0); > + return NULL; > +} > +#endif > + > +char *ctermid(char *s) > +{ > + assert(0); > + return NULL; > +} > + > +char *ctime(const time_t *timep) > +{ > + assert(0); > + return NULL; > +} > + > +char *dirname(char *path) > +{ > + assert(0); > + return NULL; > +} > + > +double drand48(void) > +{ > + assert(0); > +} > + > +#ifdef _XOPEN_SOURCE > +void encrypt(char block[64], int edflag) > +{ > + assert(0); > +} > +#endif > + > +void endgrent(void) > +{ > + assert(0); > +} > + > +void endpwent(void) > +{ > + assert(0); > +} > + > +#ifdef _XOPEN_SOURCE > +struct tm *getdate(const char *string) > +{ > + assert(0); > + return NULL; > +} > +#endif > + > +struct group *getgrent(void) > +{ > + assert(0); > + return NULL; > +} > + > +struct group *getgrgid(gid_t gid) > +{ > + assert(0); > + return NULL; > +} > + > +struct group *getgrnam(const char *name) > +{ > + assert(0); > + return NULL; > +} > + > +struct hostent *gethostent(void) > +{ > + assert(0); > + return NULL; > +} > + > +char *getlogin(void) > +{ > + assert(0); > + return NULL; > +} > + > +struct netent *getnetbyaddr(uint32_t net, int type) > +{ > + assert(0); > + return NULL; > +} > + > +struct netent *getnetbyname(const char *name) > +{ > + assert(0); > + return NULL; > +} > + > +struct netent *getnetent(void) > +{ > + assert(0); > + return NULL; > +} > + > +struct protoent *getprotobyname(const char *name) > +{ > + assert(0); > + return NULL; > +} > + > +struct protoent *getprotobynumber(int proto) > +{ > + assert(0); > + return NULL; > +} > + > +struct protoent *getprotoent(void) > +{ > + assert(0); > + return NULL; > +} > + > +struct passwd *getpwent(void) > +{ > + assert(0); > + return NULL; > +} > + > +struct passwd *getpwnam(const char *name) > +{ > + assert(0); > + return NULL; > +} > + > +struct passwd *getpwuid(uid_t uid) > +{ > + assert(0); > + return NULL; > +} > + > +struct servent *getservent(void) > +{ > + assert(0); > + return NULL; > +} > + > +struct servent *getservbyname(const char *name, const char *proto) > +{ > + assert(0); > + return NULL; > +} > + > +struct servent *getservbyport(int port, const char *proto) > +{ > + assert(0); > + return NULL; > +} > + > +struct utmpx *getutxent(void) > +{ > + assert(0); > + return NULL; > +} > + > +struct utmpx *getutxid(const struct utmpx *a) > +{ > + assert(0); > + return NULL; > +} > + > +struct utmpx *getutxline(const struct utmpx *a) > +{ > + assert(0); > + return NULL; > +} > + > +struct tm *gmtime(const time_t *timep) > +{ > + assert(0); > + return NULL; > +} > + > +int hcreate(size_t nel) > +{ > + assert(0); > + return 0; > +} > + > +ENTRY *hsearch(ENTRY item, ACTION action) > +{ > + assert(0); > + return NULL; > +} > + > +void hdestroy(void) > +{ > + assert(0); > +} > + > +char *inet_ntoa(struct in_addr in) > +{ > + assert(0); > + return NULL; > +} > + > +char *l64a(long value) > +{ > + assert(0); > + return NULL; > +} > + > +double lgamma(double x) > +{ > + assert(0); > + return 0; > + > +} > + > +float lgammaf(float x) > +{ > + assert(0); > + return 0; > +} > + > +long double lgammal(long double x) > +{ > + assert(0); > + return 0; > +} > + > +struct lconv *localeconv(void) > +{ > + assert(0); > + return NULL; > +} > + > +struct tm *localtime(const time_t *timep) > +{ > + assert(0); > + return NULL; > +} > + > +long int lrand48(void) > +{ > + assert(0); > + return 0; > +} > + > +long int mrand48(void) > +{ > + assert(0); > + return 0; > +} > + > +struct utmpx *pututxline(const struct utmpx *a) > +{ > + assert(0); > + return NULL; > +} > + > +int rand(void) > +{ > + assert(0); > + return 0; > +} > + > + > +struct dirent *readdir(DIR *dirp) > +{ > + assert(0); > + return NULL; > +} > + > +void setgrent(void) > +{ > + assert(0); > +} > + > +#ifdef _XOPEN_SOURCE > +void setkey(const char *key) > +{ > + assert(0); > +} > +#endif > + > +void setpwent(void) > +{ > + assert(0); > +} > + > +void setutxent(void) > +{ > + assert(0); > +} > + > +char *strerror(int errnum) > +{ > + assert(0); > + return NULL; > +} > + > +char *strsignal(int sig) > +{ > + assert(0); > + return NULL; > +} > + > +char *strtok(char *str, const char *delim) > +{ > + assert(0); > + return NULL; > +} > + > +int system(const char *command) > +{ > + assert(0); > + return 0; > +} > + > +char *tmpnam(char *s) > +{ > + assert(0); > + return NULL; > +} > + > +char *ttyname(int fd) > +{ > + assert(0); > + return NULL; > +} > + > + > diff --git a/exec/tsafe.h b/exec/tsafe.h > new file mode 100644 > index 0000000..537a17c > --- /dev/null > +++ b/exec/tsafe.h > @@ -0,0 +1,43 @@ > +/* > + * Copyright (c) 2010 Red Hat, Inc. > + * > + * All rights reserved. > + * > + * Author: Angus Salkeld <[email protected]> > + * > + * This software licensed under BSD license, the text of which follows: > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions are > met: > + * > + * - Redistributions of source code must retain the above copyright notice, > + * this list of conditions and the following disclaimer. > + * - Redistributions in binary form must reproduce the above copyright > notice, > + * this list of conditions and the following disclaimer in the > documentation > + * and/or other materials provided with the distribution. > + * - Neither the name of the MontaVista Software, Inc. nor the names of its > + * contributors may be used to endorse or promote products derived from > this > + * software without specific prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS > IS" > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF > + * THE POSSIBILITY OF SUCH DAMAGE. > + */ > + > +#ifndef _TSAFE_H_ > +#define _TSAFE_H_ > + > +void tsafe_init (char **envp); > +void tsafe_off (void); > +void tsafe_on (void); these should be extern void XXXX Regards -steve > + > +#endif /* _TSAFE_H_ */ > + _______________________________________________ Openais mailing list [email protected] https://lists.linux-foundation.org/mailman/listinfo/openais
