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.
