linux-ext4 - Re: [PATCH] libext2fs: add new test: tst_inode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <0E958708-A440-43C9-B69F-4E677AB82580@dilger.ca>
Date:	Wed, 14 Sep 2011 14:47:08 -0600
From:	Andreas Dilger <adilger@...ger.ca>
To:	Theodore Ts'o <tytso@....edu>
Cc:	Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] libext2fs: add new test: tst_inode_size

On 2011-09-14, at 11:16 AM, Theodore Ts'o wrote:
> This test makes sure the size of the ext2_inode is what we expect
> 
> +#define offsetof(type, member)  __builtin_offsetof (type, member)
> +#define check_field(x) cur_offset = do_field(#x, sizeof(inode.x),	\
> +				offsetof(struct ext2_inode, x), \
> +				cur_offset)

One thing I noticed with this check_field() macro is that it doesn't
actually detect the case if the size of a field is changed.  This hit
me when I was making a cleanup to the large journal patch which renamed
s_jnl_blocks[15] to s_jnl_size_lo and s_jnl_blocks[16] to s_jnl_size_hi
for clarity.  The tst_super_size test passed just fine, but the e2fsck
test scripts failed in weird and wonderful ways.

A better solution might be to explicitly pass the expected field size
instead of getting both the size and offset from the structure itself.
Since these structures change very rarely it isn't much maintenance,
but it would be lousy if code was released that had some incorrect
field offset because someone increased or decreased an earlier field
without thinking enough, and those fields weren't used in normal tests.

I can submit a patch if you are interested.

> +static int do_field(const char *field, size_t size, int offset, int cur_offset)
> +{
> +	if (offset != cur_offset) {
> +		printf("Warning!  Unexpected offset at %s\n", field);
> +		exit(1);
> +	}
> +	printf("%8d %-30s %3u\n", offset, field, (unsigned) size);
> +	return offset + size;
> +}
> +
> +void check_structure_fields()
> +{
> +#if (__GNUC__ >= 4)
> +	int cur_offset = 0;
> +
> +	printf("%8s %-30s %3s\n", "offset", "field", "size");
> +	check_field(i_mode);
> +	check_field(i_uid);
> +	check_field(i_size);
> +	check_field(i_atime);
> +	check_field(i_ctime);
> +	check_field(i_mtime);
> +	check_field(i_dtime);
> +	check_field(i_gid);
> +	check_field(i_links_count);
> +	check_field(i_blocks);
> +	check_field(i_flags);
> +	check_field(osd1.linux1.l_i_version);
> +	check_field(i_block);
> +	check_field(i_generation);
> +	check_field(i_file_acl);
> +	check_field(i_size_high);
> +	check_field(i_faddr);
> +	check_field(osd2.linux2.l_i_blocks_hi);
> +	check_field(osd2.linux2.l_i_file_acl_high);
> +	check_field(osd2.linux2.l_i_uid_high);
> +	check_field(osd2.linux2.l_i_gid_high);
> +	check_field(osd2.linux2.l_i_reserved2);
> +	printf("Ending offset is %d\n\n", cur_offset);
> +#endif
> +}
> +
> +
> +int main(int argc, char **argv)
> +{
> +	int l = sizeof(struct ext2_inode);
> +
> +	check_structure_fields();
> +	printf("Size of struct ext2_inode is %d\n", l);
> +	if (l != 256) {
> +		exit(1);
> +	}
> +	exit(0);
> +}
> -- 
> 1.7.4.1.22.gec8e1.dirty
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html