On Sat, Nov 05, 2011 at 05:40:22PM +0200, Mikolaj Golub wrote:
>
> On Sat, 5 Nov 2011 15:58:01 +0200 Kostik Belousov wrote:
>
> KB> + if (error == EFAULT) {
> KB> + for (i = 0; i < len; i++) {
> KB> + c = fubyte(sptr + i);
> KB> + if (c < 0)
> KB> As a purely stylistical issue, compare with -1.
>
> KB> + return (EFAULT);
> KB> + buf[i] = (char)c;
> KB> + if (c == '\0')
> KB> + break;
> KB> + }
> KB> + error = 0;
> KB> + }
> KB> + return error;
> KB> Put () around error.
>
> Thanks.
>
> KB> + /*
> KB> + * Check that that the address is in user space.
> KB> + */
> KB> + if (vptr + 1 < VM_MIN_ADDRESS + 1 || vptr >= VM_MAXUSER_ADDRESS)
> KB> + return (ENOEXEC);
> KB> Why is this needed ?
>
> I saw this check in libkvm for ps_argvstr and ps_envstr and decided to add it
> too. Just some additional check that ps_string fields, which can be
> overwritten by the user, look reasonable. If you think this is not very useful
> I remove it.
The proc_rwmem() must handle access outside the user VA (and it does).
>
> KB> I think that the aux vector must be naturally aligned. You can return
> KB> ENOEXEC early if vptr is not aligned.
>
> Not sure I see what you mean. vptr for auxv is calculated just couple lines
> above, and I check the result here, in the part common for all vector types.
You do not check for the alignment. Am I wrong ?
>
> BTW, investigating the cases when I got
>
> procstat: sysctl: kern.proc.args: 58002: 8: Exec format error
>
> they were because the PROC_VECTOR_MAX limit (512 entries, as it is in
> linprocfs and libkvm) is small for real world cases:
>
> get_proc_vector(pid = rm[47883], type = 0): vsize (3009) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[47883], type = 0): vsize (3009) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[47890], type = 0): vsize (3008) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[47890], type = 0): vsize (3008) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[47897], type = 0): vsize (4511) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[47897], type = 0): vsize (4511) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[47897], type = 0): vsize (4511) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[48044], type = 0): vsize (611) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[52189], type = 0): vsize (772) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[52192], type = 0): vsize (1157) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[55685], type = 0): vsize (1041) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[55687], type = 0): vsize (1040) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[55690], type = 0): vsize (1559) > PROC_VECTOR_MAX
> (512))
>
> So I am going to change it to ARG_MAX and use independent limit (256 entries)
> for auxv.
Ok.
>
> KB> Why the blank after the loop statement in get_ps_strings() ?
>
> Sorry, what blank you mean? I don't see it in get_ps_strings(). May be you
> mean the blank line in get_proc_vector() before return?
+ for (sptr = proc_vector[i]; ; sptr += GET_PS_STRINGS_CHUNK_SZ) {
+
+ error = proc_read_mem(td, p, (vm_offset_t)sptr,
The line between for() and error = .
>
> KB> There shall be blank lines after the '{' in proc_getargv() and
> proc_getenvv().
>
> Ah, sure.
>
> KB> Note that using cached pargs is somewhat inconsistent with the digging
> KB> into ps_strings.
>
> KB> procfs_doproccmdline() can benefit from your work.
>
> Thanks, I will look at it :-).
>
> --
> Mikolaj Golub
pgpD3ZL2Z2fRc.pgp
Description: PGP signature
