Re: Patch for Unix socket peer credentials

In article <[e-mail]>,
  "James F. Hranicky" <[e-mail]> writes:

> Are negative values allowed on Linux? AFAICT, if the credentials aren't 
> available on Linux, say when I check a TCPServer socket's credentials
> after accepting a connection from another host, the system call returns
> 0 but sets the uid & gid to -1:

An example of negative UID is on NFS: "nobody" is -2.  [RFC 1094]

> +if have_macro("SO_PEERCRED", "sys/socket.h")
> +  $defs <<  "-DHAVE_SO_PEERCRED "
> +end

Uselessly complex.
Just use #if defined(SO_PEERCRED) in socket.c.

> +/*
> + * Document-method: peer_cred
> + * call-seq: socket.peer_cred => hash
> + *      hash[:uid]  => ruid || euid
> + *      hash[:gid]  => rgid || egid 
> + *      hash[:ruid] => ruid
> + *      hash[:euid] => euid
> + *      hash[:rgid] => rgid
> + *      hash[:egid] => egid
> + * }

unmatched closing curly brace?

>  static VALUE
> +bsock_peer_cred(sock)
> +    VALUE sock;
> +{
> +    char buf[1024];
> +    socklen_t len = sizeof buf;

It seems that buf is not used.

> +#if defined(HAVE_GETPEERUCRED)
> +    ucred_t *creds;
> +#elif defined(HAVE_SO_PEERCRED)
> +    struct ucred creds;
> +#elif defined(HAVE_GETPEEREID)
> +    int euid, egid;
> +#else
> +    rb_raise(rb_eSocket, "peer_cred not implemented on this platform");

There is rb_notimplement().
See other usages of rb_notimplement().

> +    kuid = rb_str_intern(rb_str_new2("uid"));
> +    kgid = rb_str_intern(rb_str_new2("gid"));
> +    kruid = rb_str_intern(rb_str_new2("ruid"));
> +    krgid = rb_str_intern(rb_str_new2("rgid"));
> +    keuid = rb_str_intern(rb_str_new2("euid"));
> +    kegid = rb_str_intern(rb_str_new2("egid"));

rb_str_new2 and rb_str_intern is too complex here.
Use ID2SYM(rb_intern("uid")), etc.

> +    ahash = rb_hash_new();
> +
> +    rb_hash_aset(ahash, kuid, Qnil);
> +    rb_hash_aset(ahash, kgid, Qnil);
> +    rb_hash_aset(ahash, kruid, Qnil);
> +    rb_hash_aset(ahash, krgid, Qnil);
> +    rb_hash_aset(ahash, keuid, Qnil);
> +    rb_hash_aset(ahash, kegid, Qnil);

I think the hash entries which OS doesn't provides are useless.

% ./ruby -v -rsocket -e '
File.unlink("/tmp/s") rescue nil
serv = UNIXServer.new("/tmp/s")
sock = UNIXSocket.new("/tmp/s")
s = serv.accept
p s.peer_cred
'
ruby 1.8.5 (2006-07-21) [i686-linux]
{:egid=>1000, :rgid=>nil, :uid=>1000, :gid=>1000, :euid=>1000, :ruid=>nil}

I think :rgid=>nil and :ruid=>nil should be removed as follows.

{:egid=>1000, :uid=>1000, :gid=>1000, :euid=>1000}

> +#if defined(HAVE_GETPEERUCRED)
> +    if ((creds = malloc(ucred_size())) == NULL)
> +        rb_sys_fail("malloc");

Why memory allocation?

> +    if (getpeerucred(fileno(fptr->f), &creds) < 0)
> +        rb_sys_fail("getpeerucred(2)");

It seems getpeerucred allocates memory.
http://docs.sun.com/app/docs/doc/816-5168/6mbb3hrcl?l=ja&a=view

> +    rb_hash_aset(ahash, kuid, INT2FIX(creds.uid));
> +    rb_hash_aset(ahash, kgid, INT2FIX(creds.gid));
> +    rb_hash_aset(ahash, keuid, INT2FIX(creds.uid));
> +    rb_hash_aset(ahash, kegid, INT2FIX(creds.gid));

I'd like to replace kuid by ID2SYM(rb_intern("uid")), to
avoid variables and unused variable initialization.

> +    if ((hashval = rb_hash_aref(ahash, kuid)) == Qnil) 
> +        rb_raise(rb_eSocket, "Invalid credentials: uid is nil");
> +
> +    if ((hashval = rb_hash_aref(ahash, kgid)) == Qnil)
> +        rb_raise(rb_eSocket, "Invalid credentials: gid is nil");

It seems a defensive programming.

> +    rb_define_method(rb_cBasicSocket, "peer_cred", bsock_peer_cred, 0);

Of course, the last possible problem is the method name.
I'm not sure that matz accept peer_cred.
-- 
Tanaka Akira