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

[Tyler Hicks]

On 2013-06-25 13:58:19, John Johansen wrote:
> 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.

This is incorrect. This patch changes aa_getpeercon() to use the
internal getpeercon(), which does set *len upon error.

However, I'm not arguing for this patch. I'm just trying to get
sarnold's ack so I can move forward with this patch set (with or without
this RFC patch). :)

Tyler

> 
> 
> > 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);
> > 
> 
-------------- 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/20130625/1cd8eab3/attachment-0001.pgp>