Re: [PATCH v2] Modify mtd-utils intgck utility to test extended attribute set/get for UBIFS

[Artem Bityutskiy <dedekind1@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5542 bytes --]

Hi,

On Thu, 2012-05-10 at 16:05 -0700, Subodh Nijsure wrote:
> Changes since v1:
>         Randomize value of extended attribute.
>         Update dir_entry structure to keep track of extended attribute and
>         check it during verification phase, as suggested during v1 review.
> 	Now compiling and running integck requires libattr.
> 
> Signed-off-by: Subodh Nijsure <snijsure@grid-net.com>

Please, fix these compiler warnings:

gcc  -Wall -g -O2 -I../../../include -I../../../ubi-utils//include -c ../../../ubi-utils//libubi.c -o libubi.o
ar cr libubi.a libubi.o
gcc  -Wall -g -O2 -I../../../include -I../../../ubi-utils//include    integck.c libubi.a   -o integck
integck.c: In function ‘xattr_check’:
integck.c:379:2: warning: implicit declaration of function ‘getxattr’ [-Wimplicit-function-declaration]
integck.c: In function ‘assign_xattr’:
integck.c:410:2: warning: implicit declaration of function ‘setxattr’ [-Wimplicit-function-declaration]
integck.c: In function ‘xattr_check’:
integck.c:380:2: warning: ‘ret’ may be used uninitialized in this function [-Wuninitialized]

Below are some comments - some of them apply to more places, so do not
forget to check other places in your patch for the same things.

> diff --git a/tests/fs-tests/integrity/Makefile b/tests/fs-tests/integrity/Makefile
> index 4d6fc7d..b814f77 100644
> --- a/tests/fs-tests/integrity/Makefile
> +++ b/tests/fs-tests/integrity/Makefile
> @@ -25,7 +25,7 @@ $(TARGETS): libubi.a
>  # Disable optimizations to make it possible to use gdb comfortably
>  # Use -rdynamic to have stack backtraces
>  debug: libubi.a
> -	gcc $(CFLAGS) -O0 -D INTEGCK_DEBUG -rdynamic integck.c libubi.a -o integck
> +	gcc $(CFLAGS) -O0 -D INTEGCK_DEBUG -rdynamic integck.c libubi.a -o integck -lattr
>  
>  clean:
>  	rm -f *.o $(TARGETS) libubi.a
> diff --git a/tests/fs-tests/integrity/integck.c b/tests/fs-tests/integrity/integck.c
> index 30322cd..7bd73bf 100644
> --- a/tests/fs-tests/integrity/integck.c
> +++ b/tests/fs-tests/integrity/integck.c
> @@ -91,6 +91,7 @@
>  		normsg(fmt " (line %d)", ##__VA_ARGS__, __LINE__);           \
>  } while(0)
>  
> +static int xattr_count;
>  /* The variables below are set by command line arguments */
>  static struct {
>  	long repeat_cnt;
> @@ -198,6 +199,7 @@ struct dir_entry_info /* Each entry in a directory has one of these */
>  	struct dir_entry_info *next_link; /* List of hard links for same file */
>  	struct dir_entry_info *prev_link; /* List of hard links for same file */
>  	char *name;
> +	char *xattr_value; /* Extended attribute on this directory */
>  	struct dir_info *parent; /* Parent directory */
>  	union {
>  		struct file_info *file;
> @@ -360,6 +362,60 @@ static char *cat_paths(const char *a, const char *b)
>  }
>  
>  /*
> + * Verify security.selinux extendend attribute for given path
> + */
> +void xattr_check(char *path, char *xattr_value)
> +{
> +	int ret;
> +	char buf[255];

Introduce a macro for max xattr length instead of a magic number.

> +	int attrLen;
> +	/* Check if path actually exists */
> +	if (access(path, F_OK) != 0)
> +		return;
> +	if (xattr_value == NULL)
> +		return;

It is faster to first check xattr_value, then make the "access" syscall.

Also, please the style this file uses - "if (!xattr)".

> +	attrLen = strlen(xattr_value) + 1;
> +	v("retrieve extended attribute for  %s", path);
> +	getxattr(path, "security.selinux", buf, attrLen);
> +	CHECK(ret == 0);
> +	ret = strncmp(buf, xattr_value, attrLen);
> +	if (ret != 0) {
> +		printf("ret != 0 PATH %s\n", path);
> +		printf("Expected value %s and retrived val %s\n",
> +			xattr_value, buf);

Use errmsg().

> +		CHECK(ret == 0);
> +	} else {
> +		CHECK(ret == 0);
> +	}
> +}
> +
> +/*
> + * Assign security.selinux extendend attribute for this path
> + */
> +char *assign_xattr(const char *path)
> +{
> +	int ret;
> +	char value[255], *xattr_value;
> +	int attrLen;
> +
> +	sprintf(value, "root:object_r:bin_t%d", xattr_count);
> +	xattr_count++;
> +	xattr_value = (char *)malloc(strlen(value)+1);

sizefo(value) instead of strlen.
Remove the cast after malloc.

> +	if (xattr_value == NULL)
> +		return NULL;
> +	strcpy(xattr_value, value);
> +	attrLen = strlen(xattr_value) + 1;
> +	v("assign extended attribute to %s", path);
> +	/* printf("%s assigned %s\n", path, xattr_value); */
Kill this comment.

> +	ret = setxattr(path, "security.selinux", xattr_value, attrLen, 0x0);
> +	if (ret == 0)
> +		return xattr_value;
> +	else {
> +		free(xattr_value);
> +		return NULL;
> +	}
> +}

Please, do not assign an xattr to all files and directories - better a
give it a 20% chance or something like this. This is better for test
coverage I thing. Add something like:

if (random_no(5) != 0)
	return

at the beginning of this function.


> diff --git a/tests/ubi-tests/Makefile b/tests/ubi-tests/Makefile
> index 2c47a9f..ba8e7e0 100644
> --- a/tests/ubi-tests/Makefile
> +++ b/tests/ubi-tests/Makefile
> @@ -8,7 +8,8 @@ LIBS = libubi
>  TARGETS=io_update volrefcnt integ io_paral io_read io_basic \
>            mkvol_basic mkvol_bad mkvol_paral rsvol
>  
> -CFLAGS += -I$(LIBUBI_HEADER_PATH) -I $(KERNELHDR) -lpthread
> +CFLAGS += -I$(LIBUBI_HEADER_PATH) -I$(KERNELHDR) -lpthread
> +LDFLAGS += -lpthread

This change seems to have nothing to do with integck - please, kill this
chunk.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]