Date: Fri, 16 Dec 2005 22:19:43 +0000 From: "Wojciech A. Koszek" <dunstan@freebsd.czest.pl> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-hackers@freebsd.org, phk@freebsd.org, Peter Jeremy <PeterJeremy@optushome.com.au> Subject: Re: [CALL FOR TESTERS] New system call: abort2() Message-ID: <20051216221943.GB58739@FreeBSD.czest.pl> In-Reply-To: <200512161114.14398.jhb@freebsd.org> References: <20051215223745.GA37768@FreeBSD.czest.pl> <20051216091057.GQ77268@cirb503493.alcatel.com.au> <200512161114.14398.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Dec 16, 2005 at 11:14:12AM -0500, John Baldwin wrote:
> On Friday 16 December 2005 04:10 am, Peter Jeremy wrote:
> > On Thu, 2005-Dec-15 22:37:45 +0000, Wojciech A. Koszek wrote:
> > > abort2(const char *why, int nargs, void **args);
> > >
> > >"why" is reason of program abort, "nargs" is number of arguments
> > >passed in "args". Both "why" and "args" (with "%p" format) will be
> > >printed via log(9). Sample output:
> > >[..]
> > >pid <3004> <abort2> abort2: ABORT2 <arg0:0x62612f2e>
> > >pid <3019> <abort2> abort2: invalid argument
> > >[..]
> >
> > I don't believe the following code is correct. uap->args is a
> > userspace pointer so uap->args[i] is dereferencing a userspace
> > argument in kernelspace.
> > + arg = uargs[i] = (void *) fuword(uap->args[i]);
> > I think it should be fuword(uap->args + i);
> >
> > I don't see the point of the following test. "arg" is printed using
> > %p and never de-referenced so there's no reason it can't be NULL. I
> > would see that a legitimate use of abort2() is when the application
> > detects that a pointer is unexpectedly NULL. Aborting on -1 is less
> > clear - if fuword() fails, it will return -1 but, equally, a faulty
> > user application may have left -1 in a pointer. (Note that mmap(2)
> > returns -1 on error so it's not inconceivable that a pointer could
> > contain -1).
> >
> > + /* Prevent from faults in user-space */
> > + if (arg == NULL || arg == (void *)-1) {
> > + error = EINVAL;
> > + break;
> > + }
> >
> > Taking the above into account, I believe the code should be:
> > + if (uap->args == NULL)
> > + break;
> > + error = copyin(uap->args, uargs, uap->nargs * sizeof (void
> > *)); + if (error != 0)
> > + break;
>
> Agreed. Also, copyinstr() can provide a better interface for copying the why
> string in. Also, the PROC LOCK isn't needed for reading the static p_pid and
> p_comm fields of struct proc. Also, I second the other comments of do { }
> while(0) vs goto. Many existing syscalls use 'goto out;' for error handling,
> and I think that is one of the very few cases when goto is useful and not
> harmful.
Thanks for the suggestions and comments!
My question is related with copying string from user-space: the only
difference I can see between those functions (other than operating of
strings/sbufs) is that sbuf_copyin() looses 'done' [1]. Since current
abort2() makes use of sbuf(9), I'll have to have additional buffer just
for string copying and than copy it to sbuf. Would you prefer this
solution or complete migration from sbufs to strl..()?
[1] Couldn't sbuf_copyin() simply return 'done' from copyinstr()
embedded in it, since it already returns -1 on failure? This function
is used in two places, which make no use of return value.
--
* Wojciech A. Koszek && dunstan@FreeBSD.czest.pl
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20051216221943.GB58739>