gcc 4.3.1
config/darwin.h:
#define ENABLE_EXECUTE_STACK \
extern void __enable_execute_stack (void *); \
void \
__enable_execute_stack (void *addr) \
{ \
extern int mprotect (void *, size_t, int); \
extern int getpagesize (void); \
static int size; \
static long mask; \
\
char *page, *end; \
\
if (size == 0) \
{ \
size = getpagesize(); \
mask = ~((long) size - 1); \
} \
\
page = (char *) (((long) addr) & mask); \
end = (char *) ((((long) (addr + (TARGET_64BIT ? 48 : 40))) & mask) +
size); \
\
/* 7 == PROT_READ | PROT_WRITE | PROT_EXEC */ \
(void) mprotect (page, end - page, 7); \
}
contains obvious race condition.
between the storing of size and the storing of mask.
One thread might store size, making it not zero, and another thread
could then decide size is stored (it is), and that mask is stored (it isn't),
and go ahead and use the uninitialize zero value of mask.
easy fix: don't cache mask. Calling mprotect is going to expensive anyway.
Even getpagesize might be very cheap. Might not be worth caching.
like so (diff written by hand, and lots of extra whitespace
so mail programs maybe don't ruin the rest of the formating)
extern int getpagesize (void); \
static int size; \
< static long mask; \
> long mask; \
\
char *page, *end; \
\
if (size == 0) \
{ \
size = getpagesize(); \
< mask = ~((long) size - 1); \
} \
> mask = ~((long) size - 1); \
Or maybe just make the compile-time constant.
same problem in netbsd.h
no problem in openbsd.h, sol2.h, osf.h -- no cache
no problem in freebsd.h, no use of page size
And yes, I know Apple doesn't even support this and that the whole
thing is maybe controversial, but that is independent.
The race condition shouldn't be there.
You might also fix it by checking if mask is zero, but that wouldn't suffice,
without a memory barrier and/or volatile -- to force mask to be stored after
size or such.
You don't really want volatile, or, if you do put in volatile, you want to have
locals
to cache the globals, to avoid unnecessary extra fetches.
(Yes, I'm micro optimizing and de-micro-optimzing, I realize.)
Really just not static caching mask and possibly not caching pagesize is a good
simple reliable efficient solution.
Caching the values in locals would be goodness imho though, like, again just
composed here, not compiled:
extern int mprotect (void *, size_t, int); \
extern int getpagesize (void); \
static size_t static_size;
\
size_t mask; \
size_t size; \
\
char *page, *end; \
\
size = static_size;
if (size == 0) \
{ \
size = getpagesize(); \
static_size = size;
} \
mask = ~(size -1); \
\
page = (char *) (((size_t) addr) & mask); \
end = (char *) ((((size_t) (addr + (TARGET_64BIT ? 48 : 40))) & mask) +
size); \
\
/* 7 == PROT_READ | PROT_WRITE | PROT_EXEC */ \
(void) mprotect (page, end - page, 7); \
size_t is a good type to use here, right?
Best to stick to unsigned imho.
And sizeof(void*) == sizeof(size_t), more often than sizeof(void*) ==
sizeof(long), eh?
- Jay
