lib/blkid/list.h: Fix undefined behavior in list_entry() macro

PATCH New Sami Liedes 1 participant 8 messages Dec 13, 2012 1 message not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Sami Liedes

author

2012-12-13 22:29 UTC #1 lore raw

PATCH 1/8 lib/blkid/list.h: Fix undefined behavior in list_entry() macro

Update list_entry() macro, which is basically the same as the container_of() macro in the kernel, to use offsetof() to fix undefined behavior.

Caught using clang -fsanitize=undefined.

Signed-off-by Sami Liedes <[email protected]>

---
 lib/blkid/list.h |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

lib/blkid/list.h +3 -2

index c1cbfec..d15228e 100644
@@ -148,8 +148,9 @@ _INLINE_ void list_splice(struct list_head *list, struct list_head *head)
  * @type:	the type of the struct this is embedded in.
  * @member:	the name of the list_struct within the struct.
  */
-#define list_entry(ptr, type, member) \
-	((type *)((char *)(ptr)-(unsigned long)(&((type *)0)->member)))
+#define list_entry(ptr, type, member) ({		       \
+	const typeof( ((type *)0)->member ) *__mptr = (ptr);   \
+	(type *)( (char *)__mptr - offsetof(type,member) );})
 
 /**
  * list_for_each - iterate over elements in a list

Signature · 1 line

1.7.10.4

Sami Liedes

author

2012-12-13 22:39 UTC #2 lore raw

PATCH 2/8 lib/ext2fs/rbtree.h: Fix container_of() undefined behavior

In reply to a message not in the archive

The code before the macro definition #undefs and redefines offsetof(). Unfortunately the new definition actually causes undefined behavior.

The code checks for __compiler_offsetof() before redefining. However I'm not sure where it is supposed to be defined.

Just enclose the redefinition in #ifndef __GNUC__ for now.

Caught using clang -fsanitize=undefined.

Signed-off-by Sami Liedes <[email protected]>

---
 lib/ext2fs/rbtree.h |    2 ++
 1 file changed, 2 insertions(+)

lib/ext2fs/rbtree.h +2 -0

index 16defb5..088c352 100644
@@ -96,12 +96,14 @@ static inline struct page * rb_insert_page_cache(struct inode * inode,
 
 #include <stdlib.h>
 
+#ifndef __GNUC__
 #undef offsetof
 #ifdef __compiler_offsetof
 #define offsetof(TYPE,MEMBER) __compiler_offsetof(TYPE,MEMBER)
 #else
 #define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
 #endif
+#endif
 
 #define container_of(ptr, type, member) ({			\
 	const __typeof__( ((type *)0)->member ) *__mptr = (ptr);	\

Signature · 1 line

1.7.10.4

Sami Liedes

author

2012-12-13 22:38 UTC #3 lore raw

PATCH 3/8 e2fsck/pass1.c: Fix undefined behavior in check_blocks()

In reply to a message not in the archive

The offending code is this:

   pb.max_blocks = 1 << (31 - fs->super->s_log_block_size);

While pb.max_blocks is of type blk64_t, the intermediate result of the expression '1 << 31' is int. However 1 << 31 does not fit in a 32-bit signed int, causing undefined behavior.

Caught using clang -fsanitize=undefined.

Signed-off-by Sami Liedes <[email protected]>

---
 e2fsck/pass1.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

e2fsck/pass1.c +1 -1

index a4bd956..9cd8832 100644
@@ -2095,7 +2095,7 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
 	pb.previous_block = 0;
 	pb.is_dir = LINUX_S_ISDIR(inode->i_mode);
 	pb.is_reg = LINUX_S_ISREG(inode->i_mode);
-	pb.max_blocks = 1 << (31 - fs->super->s_log_block_size);
+	pb.max_blocks = ((blk64_t)1) << (31 - fs->super->s_log_block_size);
 	pb.inode = inode;
 	pb.pctx = pctx;
 	pb.ctx = ctx;

Signature · 1 line

1.7.10.4

Sami Liedes

author

2012-12-13 22:39 UTC #4 lore raw

PATCH 4/8 lib/ext2fs/block.c: Fix undefined behavior in block_iterate_tind()

In reply to a message not in the archive

The intermediate result of limit*limit*limit is too big to fit in a 32-bit integer, causing undefined behavior. Fix by casting one of the limits to the result type of e2_blkcnt_t.

Caught using clang -fsanitize=undefined.

Signed-off-by Sami Liedes <[email protected]>

---
 lib/ext2fs/block.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

lib/ext2fs/block.c +1 -1

index 68dcb03..dd3911d 100644
@@ -251,7 +251,7 @@ static int block_iterate_tind(blk_t *tind_block, blk_t ref_block,
 	}
 	check_for_ro_violation_return(ctx, ret);
 	if (!*tind_block || (ret & BLOCK_ABORT)) {
-		ctx->bcount += limit*limit*limit;
+		ctx->bcount += ((e2_blkcnt_t)limit)*limit*limit;
 		return ret;
 	}
 	if (*tind_block >= ext2fs_blocks_count(ctx->fs->super) ||

Signature · 1 line

1.7.10.4

Sami Liedes

author

2012-12-13 22:38 UTC #5 lore raw

PATCH 5/8 e2fsck/revoke.c: Fix undefined behavior in hash()

In reply to a message not in the archive

There's a shift by (hash_shift - 12) in hash(), but hash() is called with hash_shift=10, resulting in a negative shift and thus undefined behavior.

A simple and stupid minimal fix is to just change the subtracted amount to 10.

Caught by clang -fsanitize=undefined.

Signed-off-by Sami Liedes <[email protected]>

---
 e2fsck/revoke.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

e2fsck/revoke.c +1 -1

index 38c265e..dddef4d 100644
@@ -115,7 +115,7 @@ static inline int hash(journal_t *journal, unsigned long block)
 
 	return ((block << (hash_shift - 6)) ^
 		(block >> 13) ^
-		(block << (hash_shift - 12))) & (table->hash_size - 1);
+		(block << (hash_shift - 10))) & (table->hash_size - 1);
 }
 
 static int insert_revoke_hash(journal_t *journal, unsigned long blocknr,

Signature · 1 line

1.7.10.4

Sami Liedes

author

2012-12-13 22:38 UTC #6 lore raw

PATCH 6/8 lib/ext2fs/kernel-list.h: Fix undefined behavior in list_entry() macro

In reply to a message not in the archive

Fix the macro, which is essentially a copy of the container_of() in the kernel, to use offsetof instead of a null pointer dereference.

Caught by clang -fsanitize=undefined.

Signed-off-by Sami Liedes <[email protected]>

---
 lib/ext2fs/kernel-list.h |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

lib/ext2fs/kernel-list.h +4 -2

index e07d06b..85ae213 100644
@@ -103,8 +103,10 @@ static __inline__ void list_splice(struct list_head *list, struct list_head *hea
 	}
 }
 
-#define list_entry(ptr, type, member) \
-	((type *)((char *)(ptr)-(unsigned long)(&((type *)0)->member)))
+#define list_entry(ptr, type, member) ({                      \
+       const typeof( ((type *)0)->member ) *__mptr = (ptr);   \
+       (type *)( (char *)__mptr - offsetof(type,member) );})
+
 
 #define list_for_each(pos, head) \
         for (pos = (head)->next; pos != (head); pos = pos->next)

Signature · 1 line

1.7.10.4

Sami Liedes

author

2012-12-13 22:38 UTC #7 lore raw

PATCH 7/8 lib/ext2fs/qcow2.h: Fix #defined 1<<63 values to be unsigned.

In reply to a message not in the archive

QCOW_OFLAG_COPIED in qcow2.h and e2image.c was defined as a signed value, which however does not fit in a signed 64-bit integer.

Caught by clang -fsanitize=undefined.

Signed-off-by Sami Liedes <[email protected]>

---
 lib/ext2fs/qcow2.h |    4 ++--
 misc/e2image.c     |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

lib/ext2fs/qcow2.h +2 -2

index 81e0ec9..b997653 100644
@@ -30,8 +30,8 @@
 
 #define QCOW_MAGIC (('Q' << 24) | ('F' << 16) | ('I' << 8) | 0xfb)
 #define QCOW_VERSION		2
-#define QCOW_OFLAG_COPIED	(1LL << 63)
-#define QCOW_OFLAG_COMPRESSED	(1LL << 62)
+#define QCOW_OFLAG_COPIED	(1LLU << 63)
+#define QCOW_OFLAG_COMPRESSED	(1LLU << 62)
 
 #define QCOW_COMPRESSED		1
 #define QCOW_ENCRYPTED		2

misc/e2image.c +1 -1

index e6ea52a..36769c1 100644
@@ -47,7 +47,7 @@ extern int optind;
 #include "../version.h"
 #include "nls-enable.h"
 
-#define QCOW_OFLAG_COPIED     (1LL << 63)
+#define QCOW_OFLAG_COPIED     (1LLU << 63)
 
 
 const char * program_name = "e2image";

Signature · 1 line

1.7.10.4

Sami Liedes

author

2012-12-13 22:38 UTC #8 lore raw

PATCH 8/8 e2fsck/jfs_user.h: Fix b_data alignment in struct buffer_head

In reply to a message not in the archive

buffer_head.b_data needs to be 8-byte aligned to prevent an unaligned access via a 64-bit pointer in e.g. scan_revoke_records().

Caught using clang -fsanitize=undefined.

Signed-off-by Sami Liedes <[email protected]>

---
 e2fsck/jfs_user.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

e2fsck/jfs_user.h +1 -1

index 3cccd3f..fdaf1b2 100644
@@ -22,7 +22,7 @@ struct buffer_head {
 	int	 	b_dirty;
 	int	 	b_uptodate;
 	int	 	b_err;
-	char		b_data[1024];
+	char		b_data[1024]  __attribute__ ((aligned (8)));
 };
 
 struct inode {

Signature · 1 line

1.7.10.4