cgsecurity / testdisk

TestDisk & PhotoRec
https://www.cgsecurity.org/
GNU General Public License v2.0
1.69k stars 205 forks source link

Array-bounds issue #124

Closed Exactlywb closed 2 years ago

Exactlywb commented 2 years ago

Hi! I've built testdisk using -O2 -Werror=array-bounds on gcc 11.2 and got an error:

In file included from file_ext.c:32:
file_ext.c: In function ‘file_rename_ext’:
file_ext.c:73:38: error: array subscript ‘const struct ext2_super_block[0]’ is partly outside array bounds of ‘unsigned char[512]’ [-Werror=array-bounds]
   73 |   block_nr=(unsigned long int)le32(sb->s_first_data_block) + (unsigned long int)le16(sb->s_block_group_nr)*le32(sb->s_blocks_per_group);
      |                                      ^~
common.h:595:19: note: in definition of macro ‘le32’
  595 | #define le32(x)  (x)
      |                   ^
file_ext.c:56:17: note: while referencing ‘buffer’
   56 |   unsigned char buffer[512];
      |                 ^~~~~~

Here I've seen two problems. Let's see truncated code example:

unsigned char buffer [512];
const struct ext2_super_block *sb = (const struct ext2_super_block *)&buffer;

The first problem is there's only 512 bytes for the struct ext2_super_block? As I understand, Superblock is exactly 1024 bytes in length. And the second one is that similar memory casts violates strict aliasing rule. So I think it's better to use memcpy instead of to the structure pointer cast. The same strict aliasing violence may be found here

I hope my report will be useful for you.

Thanks

cgsecurity commented 2 years ago

Fixed by commit 0ae7969c1243a0fc048b34cc7b24c54acc257029 (There was a typo in the previous git comment...)

Exactlywb commented 2 years ago

Thanks! But what do you think about strict aliasing rule violation in this part of code?

cgsecurity commented 2 years ago

The -fstrict-aliasing option is enabled at levels -O2, -O3, -Os. -Wstrict-aliasing This option is only active when -fstrict-aliasing is active. It warns about code that might break the strict aliasing rules that the compiler is using for optimization. The warning does not catch all cases, but does attempt to catch the more common pitfalls. It is included in -Wall. It is equivalent to -Wstrict-aliasing=3

gcc 12.1.1 doesn't warn about aliasing problem, so I don't think there is a real problem...

Exactlywb commented 2 years ago

Thank you for the quick response!