[apparmor] [RFC] libapparmor: Don't expose a value-result param in aa_getpeercon_raw()

[John Johansen]

On 06/25/2013 01:19 PM, Tyler Hicks wrote:
> Unify aa_getprocattr_raw() and aa_getpeercon_raw() function prototypes
> by hiding the len value-result parameter.
> 
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> Cc: Seth Arnold <seth.arnold at canonical.com>
> Cc: John Johansen <john.johansen at canonical.com>
> ---
> 
> Code speaks louder and more clearly than my poor explanation, so here's an RFC
> patch on the idea.
> 
> I think that this addresses Seth's distaste of the len parameter. It makes the
> libapparmor API a little more predictable, but hides a useful feature of the
> getsockopt() syscall which will make external callers guess at buffer
> reallocation sizes (but they already do that with aa_getprocattr_raw()).
> 
> I'm fine with or without this patch, as I don't expect (m)any applications to
> use aa_getpeercon_raw().
> 
NAK

aa_getpeercon_raw is a shipped interface, we are not changing it with out a real
good reason.

Taking away a useful feature because its a little odd isn't a good enough reason.

Nor is this patch correct as aa_getpeercon actually reallies on *len being
set in the error case to do its buffer dynamic buffer allocation. With this
patch it tries once and if it fails that is it even if it could have the
proper size.


> Tyler
> 
>  libraries/libapparmor/doc/aa_getcon.pod       |  8 +-------
>  libraries/libapparmor/src/apparmor.h          |  2 +-
>  libraries/libapparmor/src/kernel_interface.c  | 23 ++++++++++++++++++++---
>  libraries/libapparmor/swig/SWIG/libapparmor.i |  2 +-
>  4 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/libraries/libapparmor/doc/aa_getcon.pod b/libraries/libapparmor/doc/aa_getcon.pod
> index d7f2ba6..e0a97e5 100644
> --- a/libraries/libapparmor/doc/aa_getcon.pod
> +++ b/libraries/libapparmor/doc/aa_getcon.pod
> @@ -41,7 +41,7 @@ B<int aa_gettaskcon(pid_t target, char **con, char **mode);>
>  
>  B<int aa_getcon(char **con, char **mode);>
>  
> -B<int aa_getpeercon_raw(int fd, char *buf, int *len, char **mode);>
> +B<int aa_getpeercon_raw(int fd, char *buf, int len, char **mode);>
>  
>  B<int aa_getpeercon(int fd, char **con, char **mode);>
>  
> @@ -105,12 +105,6 @@ The confinement data is too large to fit in the supplied buffer.
>  
>  =back
>  
> -=head1 NOTES
> -
> -If aa_getpeercon_raw returns -1 and errno is ERANGE, the value of size can be
> -used to reallocate buf so that it is sufficiently large enough to store the
> -confinement data.
> -
>  =head1 BUGS
>  
>  None known. If you find any, please report them at
> diff --git a/libraries/libapparmor/src/apparmor.h b/libraries/libapparmor/src/apparmor.h
> index 79bc69c..f781fb7 100644
> --- a/libraries/libapparmor/src/apparmor.h
> +++ b/libraries/libapparmor/src/apparmor.h
> @@ -48,7 +48,7 @@ extern int aa_getprocattr_raw(pid_t tid, const char *attr, char *buf, int len,
>  extern int aa_getprocattr(pid_t tid, const char *attr, char **buf, char **mode);
>  extern int aa_gettaskcon(pid_t target, char **con, char **mode);
>  extern int aa_getcon(char **con, char **mode);
> -extern int aa_getpeercon_raw(int fd, char *buf, int *len, char **mode);
> +extern int aa_getpeercon_raw(int fd, char *buf, int len, char **mode);
>  extern int aa_getpeercon(int fd, char **con, char **mode);
>  
>  #define __macroarg_counter(Y...) __macroarg_count1 ( , ##Y)
> diff --git a/libraries/libapparmor/src/kernel_interface.c b/libraries/libapparmor/src/kernel_interface.c
> index 7524188..59e6c82 100644
> --- a/libraries/libapparmor/src/kernel_interface.c
> +++ b/libraries/libapparmor/src/kernel_interface.c
> @@ -559,7 +559,7 @@ int aa_getcon(char **con, char **mode)
>  #endif
>  
>  /**
> - * aa_getpeercon_raw - get the confinement of the socket's peer (other end)
> + * getpeercon_raw - get the confinement of the socket's peer (other end)
>   * @fd: socket to get peer confinement for
>   * @buf: buffer to store the result in
>   * @len: initially contains size of the buffer, returns size of data read
> @@ -568,7 +568,7 @@ int aa_getcon(char **con, char **mode)
>   * Returns: length of confinement data including null termination or -1 on error
>   *          if errno == ERANGE then @len will hold the size needed
>   */
> -int aa_getpeercon_raw(int fd, char *buf, int *len, char **mode)
> +static int getpeercon_raw(int fd, char *buf, int *len, char **mode)
>  {
>  	socklen_t optlen = *len;
>  	char *mode_str;
> @@ -608,6 +608,23 @@ out:
>  }
>  
>  /**
> + * aa_getpeercon_raw - get the confinement of the socket's peer (other end)
> + * @fd: socket to get peer confinement for
> + * @buf: buffer to store the result in
> + * @len: the size of the buffer
> + * @mode: if set will point to mode string within buffer if it is present
> + *
> + * Returns: length of confinement data including null termination or -1 on error
> + */
> +
> +int aa_getpeercon_raw(int fd, char *buf, int len, char **mode)
> +{
> +	int l = len;
> +
> +	return getpeercon_raw(fd, buf, &l, mode);
> +}
> +
> +/**
>   * aa_getpeercon - get the confinement of the socket's peer (other end)
>   * @fd: socket to get peer confinement for
>   * @con: pointer to allocated buffer with the confinement string
> @@ -634,7 +651,7 @@ int aa_getpeercon(int fd, char **con, char **mode)
>  			return -1;
>  		memset(buffer, 0, size);
>  
> -		rc = aa_getpeercon_raw(fd, buffer, &size, mode);
> +		rc = getpeercon_raw(fd, buffer, &size, mode);
>  		/* size should contain actual size needed if errno == ERANGE */
>  	} while (rc == -1 && errno == ERANGE && size > last_size);
>  
> diff --git a/libraries/libapparmor/swig/SWIG/libapparmor.i b/libraries/libapparmor/swig/SWIG/libapparmor.i
> index 1d3ca07..a65dd28 100644
> --- a/libraries/libapparmor/swig/SWIG/libapparmor.i
> +++ b/libraries/libapparmor/swig/SWIG/libapparmor.i
> @@ -25,5 +25,5 @@ extern int aa_getprocattr_raw(pid_t tid, const char *attr, char *buf, int len,
>  extern int aa_getprocattr(pid_t tid, const char *attr, char **buf, char **mode);
>  extern int aa_gettaskcon(pid_t target, char **con, char **mode);
>  extern int aa_getcon(char **con, char **mode);
> -extern int aa_getpeercon_raw(int fd, char *buf, int *len, char **mode);
> +extern int aa_getpeercon_raw(int fd, char *buf, int len, char **mode);
>  extern int aa_getpeercon(int fd, char **con, char **mode);
>