On Dec 20 10:12, Ken Brown wrote: > On 12/20/2024 8:13 AM, Corinna Vinschen wrote: > > In case we're called via fhandler_base::raw_read(): > > > > Assuming the region to read is enclosing the noreserve() region... > > > > - addr = 0x100000000, len = 256K > > - rec->get_address() = 0x100010000, len = 64K > > > > --> start_addr = addr = 0x100000000 > > len = 256K > > > > rec->match() returns > > > > u_addr = 0x100010000, u_len = 64K > > > > commit_len = 64K - (0x100000000 - 0x100010000) > > = 64K - 0x0xFFFFFFFFFFFF0000 > > = 128K > > > > Note: commit_len is size_t, so it's unsigned! > > > > if (commit_len (128K) > len (256K))? Nope! > > > > --> VirtualAlloc (0x100000000, 128K, MEM_COMMIT, rec->gen_protect ()); > > > > If this VirtualAlloc fails, the process is screwed and gets a > > well-deserved SIGBUS. > > > > I'm not saying that there's a chance I'm missing something, but > > as of now, it looks ok to me. > > > > > The only thing I can come up with immediately is that we should sort the > > > list > > > of mmap_records in order of their starting addresses. Then if start_addr > > > == > > > u_addr, we commit u_len bytes, otherwise we fail. > > > > I don't understand this one. The code loops over all records, > > so where's the problem? > I think I just missed several crucial things, such as commit_len being > unsigned. I also didn't pay attention to the conditions under which the > function is called. I have to study the code further, but I'm sure you're > right. Thanks for the explanation, and sorry for the noise.
No worries. The expression size_t commit_len = u_len - (start_addr - u_addr); may be right, but it's not generally intelligible. If you'd like to take a stab of changing the code to be more clear, I'm happy. Corinna
