[apparmor] [PATCH v1.1 2/2] libapparmor: Be consistent with the type used for buffer sizes

[Seth Arnold]

On Fri, Sep 30, 2016 at 02:07:28PM -0500, Tyler Hicks wrote:
> The features_struct.size variable is used to hold a buffer size and it
> is also passed in as the size parameter to read(). It should be a size_t
> instead of an int.
> 
> A new helper function, features_buffer_remaining(), is created to handle
> the two places where the remaining bytes in the features buffer are
> calculated.
> 
> This patch also changes the size parameter of load_features_dir() to a
> size_t to match the same parameter of load_features_file() as well as
> the features_struct.size change described above.
> 
> Two casts were needed when comparing signed types to unsigned types.
> These casts are safe because the signed value is checked for "< 0"
> immediately before the casts.
> 
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>

I'm not a big fan of the casts but I'm not sure C leaves us much choice.
The pre-cast checks are nice.

Acked-by: Seth Arnold <seth.arnold at canonical.com>

Thanks


> ---
> 
> * Changes since v1:
>   - Subtract fst->buffer from fst->pos and ensure the result is not greater
>     than remaining before subtracting
>   - Move the remaining buffer calculation into a helper function
>   - Use the helper function in features_dir_cb(), which didn't have a sanity
>     check on the value of remaining
> 
>  libraries/libapparmor/src/features.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/libraries/libapparmor/src/features.c b/libraries/libapparmor/src/features.c
> index 088c4ea..c656c37 100644
> --- a/libraries/libapparmor/src/features.c
> +++ b/libraries/libapparmor/src/features.c
> @@ -23,6 +23,7 @@
>  #include <stdio.h>
>  #include <string.h>
>  #include <stdarg.h>
> +#include <stddef.h>
>  #include <stdlib.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
> @@ -42,7 +43,7 @@ struct aa_features {
>  
>  struct features_struct {
>  	char *buffer;
> -	int size;
> +	size_t size;
>  	char *pos;
>  };
>  
> @@ -51,17 +52,30 @@ struct component {
>  	size_t len;
>  };
>  
> -static int features_snprintf(struct features_struct *fst, const char *fmt, ...)
> +static int features_buffer_remaining(struct features_struct *fst,
> +				     size_t *remaining)
>  {
> -	va_list args;
> -	int i, remaining = fst->size - (fst->pos - fst->buffer);
> +	ptrdiff_t offset = fst->pos - fst->buffer;
>  
> -	if (remaining < 0) {
> +	if (offset < 0 || fst->size < (size_t)offset) {
>  		errno = EINVAL;
> -		PERROR("Invalid features buffer offset\n");
> +		PERROR("Invalid features buffer offset (%td)\n", offset);
>  		return -1;
>  	}
>  
> +	*remaining = fst->size - offset;
> +	return 0;
> +}
> +
> +static int features_snprintf(struct features_struct *fst, const char *fmt, ...)
> +{
> +	va_list args;
> +	int i;
> +	size_t remaining;
> +
> +	if (features_buffer_remaining(fst, &remaining) == -1)
> +		return -1;
> +
>  	va_start(args, fmt);
>  	i = vsnprintf(fst->pos, remaining, fmt, args);
>  	va_end(args);
> @@ -70,7 +84,7 @@ static int features_snprintf(struct features_struct *fst, const char *fmt, ...)
>  		errno = EIO;
>  		PERROR("Failed to write to features buffer\n");
>  		return -1;
> -	} else if (i >= remaining) {
> +	} else if ((size_t)i >= remaining) {
>  		errno = ENOBUFS;
>  		PERROR("Feature buffer full.");
>  		return -1;
> @@ -157,7 +171,10 @@ static int features_dir_cb(int dirfd, const char *name, struct stat *st,
>  
>  	if (S_ISREG(st->st_mode)) {
>  		ssize_t len;
> -		int remaining = fst->size - (fst->pos - fst->buffer);
> +		size_t remaining;
> +
> +		if (features_buffer_remaining(fst, &remaining) == -1)
> +			return -1;
>  
>  		len = load_features_file(dirfd, name, fst->pos, remaining);
>  		if (len < 0)
> @@ -176,7 +193,7 @@ static int features_dir_cb(int dirfd, const char *name, struct stat *st,
>  }
>  
>  static ssize_t load_features_dir(int dirfd, const char *path,
> -				 char *buffer, int size)
> +				 char *buffer, size_t size)
>  {
>  	struct features_struct fst = { buffer, size, buffer };
>  
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160930/d57c7edd/attachment.pgp>