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

[Tyler Hicks]

On 09/30/2016 02:28 PM, Seth Arnold wrote:
> 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.

I don't like the casts, either, but they're our only option if we don't
want -Wsign-compare warnings.

Thanks for the review! I suspect that you already reviewed the first
patch but didn't (n)ack it because you found the issue in the second
patch. If you have any comments on it, I'd appreciate them.

Tyler

> 
> 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: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160930/f33ac658/attachment.pgp>