On Tue, Nov 08, 2011 at 11:47:54PM +0200, Mikolaj Golub wrote: > > On Sun, 6 Nov 2011 20:10:41 +0200 Kostik Belousov wrote: > > KB> On Sat, Nov 05, 2011 at 10:37:46PM +0200, Mikolaj Golub wrote: > >> > >> http://people.freebsd.org/~trociny/env.sys.3.patch > > KB> Oops, I missed this in the previous review. You cannot use fubyte in > KB> proc_read_mem(). fubyte reads a byte from the address space of the > current > KB> process. The fix is easy, use proc_rwmem for 1 byte. > > KB> I do not think that fall back to single byte read is warranted for > KB> proc_read_mem calls e.g. for ps_strings. Add a flag to indicate whether > KB> the proc_read_mem should fall back to byte read ? > > KB> I would prefer using sizeof(uint64_t) and sizeof(uint32_t) instead of 8 > KB> and 4 constants in the align checks. > > KB> Might be, add PROC_ASSERT_HELD() to get_ps_string() ? > > KB> procfs patch looks good. > > Thanks. The updated version: > > http://people.freebsd.org/~trociny/env.sys.4.patch > > Investigating cases when EFAULT was returned and if the fallback was > successful I noticed that most of the cases were when p->p_comm changed during > the read, so the process was in exec in that time. In order to avoid this > error I added a check for P_INEXEC flag. And now you return success and nothing gets copied out for the process in P_INEXEC state. Either you should return an error like EAGAIN, or consider the P_INEXEC state as transitional and wait till process leaves it. Or, ignore the state as it was before, and return whatever error proc_rwmem generated (my preference).
> > After this I observed EFAULT (very rarely) only when reading arg or env > strings and fallback was successful for those cases. So I modified the patch > to do fallback only when reading strings (as it was in one of my earlier > versions but with wrong fubyte), and returned your comment which explains why > it may happen :-) > > Also in the procfs patch I have added the check for process state. > > The userland part has not been changed since my first report: > > http://people.freebsd.org/~trociny/env.user.patch > > -- > Mikolaj Golub
pgpLo6P15pt0f.pgp
Description: PGP signature
