Re: [PATCH 4/4] device/chario.c (char_write): avoid segmentation fault

[Richard Braun]

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 ?

> > 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.

> > 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.

-- 
Richard Braun