|
From: Tom H. <th...@cy...> - 2004-07-26 15:55:45
|
Bug 86000 points out a race condition in our shmat() implementation. The problem is that if the user doesn't specify an address to map the shared memory at then valgrind calls VG_(find_map_space) to find some space and passes that address to the system call. It doesn't call VG_(map_segment) to reserve the space until after the call returns however, and as shmat is part of the IPC system call that is treated as blocking this means that the address space may get used for something else before the system call is actually issued and the system call will then fail. I've got a patch to call VG_(map_segment) before the system call is made so that the space is reserved and won't be issued to anybody else and that does seem to solve the problem. It introduces a new problem however in that if the system call fails then POST(ipc) is never called and so the address space can't be released and will effectively be leaked. Anybody got any good solutions? Tom -- Tom Hughes (th...@cy...) Software Engineer, Cyberscience Corporation http://www.cyberscience.com/ |
|
From: Jeremy F. <je...@go...> - 2004-07-26 19:52:11
|
On Mon, 2004-07-26 at 16:55 +0100, Tom Hughes wrote: > Bug 86000 points out a race condition in our shmat() implementation. > > The problem is that if the user doesn't specify an address to map the > shared memory at then valgrind calls VG_(find_map_space) to find some > space and passes that address to the system call. > > It doesn't call VG_(map_segment) to reserve the space until after the > call returns however, and as shmat is part of the IPC system call that > is treated as blocking this means that the address space may get used > for something else before the system call is actually issued and the > system call will then fail. > > I've got a patch to call VG_(map_segment) before the system call is > made so that the space is reserved and won't be issued to anybody else > and that does seem to solve the problem. > > It introduces a new problem however in that if the system call fails > then POST(ipc) is never called and so the address space can't be released > and will effectively be leaked. > > Anybody got any good solutions? There's a few syscalls in which POST should be called on error exit (nanosleep, for example, needs to return the amount of unslept time on interrupt). I've been thinking that we should change may_block in sys_info into a bitfield, and encode a little more in there - like whether the syscall wants POST called on failure. (Another reason for this would be to allow conditional may_block-ness. Change PRE() to return a boolean, and add a flag into sys_info saying use the result of PRE rather than a fixed value for blockness. We could just change all the PREs to return the appropriate thing, or just use the existing value in the table.) J |
|
From: Nicholas N. <nj...@ca...> - 2004-07-27 10:45:54
|
On Mon, 26 Jul 2004, Jeremy Fitzhardinge wrote: > There's a few syscalls in which POST should be called on error exit > (nanosleep, for example, needs to return the amount of unslept time on > interrupt). I've been thinking that we should change may_block in > sys_info into a bitfield, and encode a little more in there - like > whether the syscall wants POST called on failure. > > (Another reason for this would be to allow conditional may_block-ness. > Change PRE() to return a boolean, and add a flag into sys_info saying > use the result of PRE rather than a fixed value for blockness. We could > just change all the PREs to return the appropriate thing, or just use > the existing value in the table.) Both ideas sound good to me. N |
|
From: Tom H. <th...@cy...> - 2004-07-28 22:59:38
Attachments:
valgrind-syscall-patch
|
In message <1090865863.5743.16.camel@localhost>
Jeremy Fitzhardinge <je...@go...> wrote:
> There's a few syscalls in which POST should be called on error exit
> (nanosleep, for example, needs to return the amount of unslept time on
> interrupt). I've been thinking that we should change may_block in
> sys_info into a bitfield, and encode a little more in there - like
> whether the syscall wants POST called on failure.
>
> (Another reason for this would be to allow conditional may_block-ness.
> Change PRE() to return a boolean, and add a flag into sys_info saying
> use the result of PRE rather than a fixed value for blockness. We could
> just change all the PREs to return the appropriate thing, or just use
> the existing value in the table.)
What I've done is to turn may_block into a flags word. There are
currently two flags define - MayBlock and PostOnFail. The latter
indicates that the POST routine should be called on a failure.
I've also added sys_flags to the thread state. The flags word for
the active system call is copied into that before the PRE routine
is called, and all subsequent tests are done on that flags word
rather than the one in the static table.
This means that a PRE routine can alter sys_flags in the thread
state if necessary, either to change the blocking flag, or to
indicate that POST needs to be called on fail.
Tha flags had to be put in the thread state rather than just
returned from the PRE routine because we need to get at them
when the syscall has completed, which is in a separate routine
and may be some time later for a blocking call.
The patch is attached - it also fixes the nanosleep issue you
mentioned. Unless somebody spots a problem I'll commit this and
then use it to fix the shmat bug.
Tom
--
Tom Hughes (th...@cy...)
Software Engineer, Cyberscience Corporation
http://www.cyberscience.com/
|
|
From: Jeremy F. <je...@go...> - 2004-07-29 01:03:31
|
On Wed, 2004-07-28 at 23:59 +0100, Tom Hughes wrote: > This means that a PRE routine can alter sys_flags in the thread > state if necessary, either to change the blocking flag, or to > indicate that POST needs to be called on fail. Yes, good. > The patch is attached - it also fixes the nanosleep issue you > mentioned. Unless somebody spots a problem I'll commit this and > then use it to fix the shmat bug. Looks good. Little style issue though: can you define the flags with (1 << 0), (1 << 1), etc? J |
|
From: Tom H. <th...@cy...> - 2004-07-29 08:41:55
|
In message <Pin...@he...>
Nicholas Nethercote <nj...@ca...> wrote:
> I'd prefer it if you didn't have to do this -- the ThreadState is
> already very big and this is just one more piece of state lying
> around, something we already have too much of. But if you're certain
> that it makes things easier than any non-sys_flags-state approach,
> then do it.
I'm not sure that we have any other solution that works. If we're
going to allow the flags to be changed by the PRE routine then we have
to save the changed flags somewhere that is visible to VG_(post_syscall)
which may be called some time later in a completely different context
when the syscall completes.
Tom
--
Tom Hughes (th...@cy...)
Software Engineer, Cyberscience Corporation
http://www.cyberscience.com/
|
|
From: Nicholas N. <nj...@ca...> - 2004-07-29 08:36:46
|
On Wed, 28 Jul 2004, Tom Hughes wrote: > I've also added sys_flags to the thread state. The flags word for > the active system call is copied into that before the PRE routine > is called, and all subsequent tests are done on that flags word > rather than the one in the static table. I'd prefer it if you didn't have to do this -- the ThreadState is already very big and this is just one more piece of state lying around, something we already have too much of. But if you're certain that it makes things easier than any non-sys_flags-state approach, then do it. N |
|
From: Nicholas N. <nj...@ca...> - 2004-07-29 08:43:31
|
On Thu, 29 Jul 2004, Tom Hughes wrote: > I'm not sure that we have any other solution that works. If we're > going to allow the flags to be changed by the PRE routine then we have > to save the changed flags somewhere that is visible to VG_(post_syscall) > which may be called some time later in a completely different context > when the syscall completes. Does PRE have to be able to change the flags? N |
|
From: Tom H. <th...@cy...> - 2004-07-29 08:59:38
|
In message <Pin...@he...>
Nicholas Nethercote <nj...@ca...> wrote:
> On Thu, 29 Jul 2004, Tom Hughes wrote:
>
>> I'm not sure that we have any other solution that works. If we're
>> going to allow the flags to be changed by the PRE routine then we have
>> to save the changed flags somewhere that is visible to VG_(post_syscall)
>> which may be called some time later in a completely different context
>> when the syscall completes.
>
> Does PRE have to be able to change the flags?
If it does it we want so solve the shmat problem so that shmat gets
the POST routine called on failure without it being called for all
the sub-reasons of the IPC system call.
Likewise if we want to stop all fcntl calls being treated as blocking
even when they aren't really.
Tom
--
Tom Hughes (th...@cy...)
Software Engineer, Cyberscience Corporation
http://www.cyberscience.com/
|