[apparmor] [PATCH 2/7] libapparmor: fix return value of aa_getpeercon_raw

[Tyler Hicks]

On 2013-06-24 15:20:53, Seth Arnold wrote:
> On Mon, Jun 24, 2013 at 12:10:29PM -0700, Tyler Hicks wrote:
> > As you'll see later in the man page patch, the return code and the
> > resulting value of the *size parameter can be different. If the buffer
> > passed into aa_getpeercon_raw() is too short for getsockopt() to store
> > the requested option, then aa_getpeercon_raw() would return -1, errno
> > will be ERANGE, and *size would be set to the buffer size needed for
> > getsockopt() to be successful.
> 
> Well, I was thinking more of the success case; returning the size in
> both the return value and the *len parameter just doesn't feel all that
> useful to me. 'rc' is just checked for == -1 in the one place that uses
> it, nothing would be done with the return value otherwise...
> 
> Don't get me wrong, I'm not actually opposed to it. But it doesn't
> feel useful.

I guess the alternative would be to not have *len be a value-result
param and only use it to indicate the size of buf.

Pros:
 1. Would be consistent with aa_getprocattr_raw()
 2. aa_getprocattr_raw() prototype wouldn't be as ugly as it is now

Cons:
 1. Caller would have to guess at the buffer reallocation size, despite
    the kernel interface providing that information on the first failure
 2. ??

Maybe a good middle ground would be something like this:

static int _getpeercon_raw(int fd, char *buf, int *len, char **mode);
int aa_getpeercon_raw(int fd, char *buf, int len, char **mode);
int aa_getpeercon(int fd, char **con, char **mode);

aa_getpeercon() could use the efficient, yet ugly, internal
_getpeercon_raw().

External callers that want to handle buffer allocation would use
aa_getpeercon_raw(). If buf isn't big enough, they get -1 and errno is
ERANGE. They'd have to guess at the buffer reallocation size, but the
API would match the existing aa_getprocattr_raw().

Thoughts?

Tyler

> 
> Thanks
> 
> int aa_getpeercon(int fd, char **con)
> {
>         int rc, size = INITIAL_GUESS_SIZE;
>         char *buffer = NULL;
> 
>         if (!con) {
>                 errno = EINVAL;
>                 return -1;
>         }
> 
>         do {
>                 buffer = realloc(buffer, size);
>                 if (!buffer)
>                         return -1;
>                 memset(buffer, 0, size);
> 
>                 rc = aa_getpeercon_raw(fd, buffer, &size, mode);
>                 /* size should contain actual size needed if errno == ERANGE */
> !!      } while (rc == -1 && errno == ERANGE && size > last_size);
> 
> !!      if (rc == -1) {
>                 free(buffer);
>                 *con = NULL;
>                 if (mode)
>                         *mode = NULL;
>                 size = -1;
>         } else
>                 *con = buffer;
> 
>         return size;
> }
> 



> -- 
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130624/2917c4f5/attachment-0001.pgp>