[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 4/4] device/chario.c (char_write): avoid segmentation fault
From: |
Marin Ramesa |
Subject: |
Re: [PATCH 4/4] device/chario.c (char_write): avoid segmentation fault |
Date: |
Sun, 15 Dec 2013 14:43:50 +0100 |
On 15.12.2013 14:00:22, Richard Braun wrote:
> On Sat, Dec 14, 2013 at 01:32:27PM +0100, Marin Ramesa wrote:
> > On 14.12.2013 12:45:32, Richard Braun wrote:
> > > I really don't see a problem in that code, you'll have to
> > > describe it better.
> >
> > So a pointer to io_data is cast to a pointer to a vm_map_copy
> > structure and then passed to vm_map_copyout() which uses the
> > vm_map_copy structure members as if the contents of the memory at
> > the source address were vm_map_copy structure objects, not objects
> > of io_data which is of unknown size and origin to the function.
> > This can lead to a segmentation fault, as is shown in that short
> > test program.
> >
> > My patch makes sure the io_data actually came from a vm_map_copy
> > structure.
>
> What makes you think the content could be something else than a
> vm_map_copy object ?
Well io_data is a pointer to char, not a pointer to vm_map_copy. And
there is not one member in io_req structure that keeps track of the
io_data size. The function char_write() could be called with io_data
having it's origin in something other than vm_map_copy.
> > > On the other hand, your patch relies on knowing the target
> > > types, although vm_map_copy_t is explicitely described as being
> > > opaque.
> >
> > I don't see a problem with it. There is an explicit cast to
> > vm_map_copy_t. It's first member type is known.
>
> The target type of the cast is a pointer (a very bad historical
> practice in Mach that consists of typedef'ing pointers to structures
> in order to somehow add an abstraction level intended to make the
> coding style more object-oriented). In proper C, a cast means the
> programmer is certain about the data type. Here, it is used to call
> vm_map_copyout without emitting warnings. It has nothing to do with
> accessing the data itself, just passing it around, so no, the member
> type is not know, not from the point of view of modules outside the
> VM system. By doing that, you're violating the intended opacity of
> the type (see vm_map.h, "vm_map_copy_t [exported; contents
> invisible]"). The problem with that approach is that, if someone,
> someday, decides to change that type, he wouldn't know he would also
> have to change the code you introduce, and actually, he shouldn't
> have to. That knowledge should be completely contained in
> the vm_map module.
OK, I get it.
> > > and I also don't understand why you assume the data to be null-
> > > terminated.
> >
> > Structures are always null-terminated in memory. So if data came
> > from a structure (like vm_map_copy) it has to be null-terminated.
> > If not, a random '\0' will be found, sizes will not match, and
> > function will return.
>
> No, nothing guarantees that structures are null-terminated. This is a
> very wrong assumption. In addition, even if it was possible for the
> data to be something else than a vm_map_copy (in which case we'd want
> an assertion, because it should *never* happen), the data size could
> be 0, in which case simply accessing the first byte might cause a
> crash.
The tests I've run always show null-termination. But you're right, the
structure could very well contain a '\0' in which case strlen()
wouldn't work. But there has to be some way to detect the end of a
structure in memory without knowing the types.