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

[Marin Ramesa]

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.