Sent from my iPhone
On Aug 27, 2008, at 0:27, Jay <[EMAIL PROTECTED]> wrote:
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);
Or even better store size after the store to mask.
That is:
int tmp = getpagesize();
*(volatile*)&mask = ~((long)tmp - 1);
*(volatile*)&size = tmp;
Thanks,
Andrew Pinski
\
} \
\
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
