hello, Ted
In my rhel5 system, there are lots of loop links in the /dev/.udev/failed folder, which makes the e2fsprogs
'make check' stuck in 't_ext_jnl_rm'. I'm not sure if the loop links are generated by udev defect or misconfiguration,
but anyway, I think the ext2fs_find_block_device() should do some sanity check to avoid infinite loop.
I made a simple patch which breaks the loop when depth reaching 5 in ext2fs_find_block_device(), any comments? Thank you.
>From 5ef3b82266e7fb5edbce4febc4924b0beaccacf1 Mon Sep 17 00:00:00 2001
From: Niu Yawei <[email protected]>
Date: Wed, 2 Nov 2011 04:31:11 +0800
Subject: [PATCH] Avoid infinite loop in ext2fs_find_block_device()
The ext2fs_find_block_device() should stop searching when the
directory depth reaching a certain threshold, otherwise, it could
run into infinite loop if there are loop links in the device
directory.
Signed-off-by: Niu Yawei <[email protected]>
---
lib/ext2fs/finddev.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/lib/ext2fs/finddev.c b/lib/ext2fs/finddev.c
index 13ef14b..61f9754 100644
--- a/lib/ext2fs/finddev.c
+++ b/lib/ext2fs/finddev.c
@@ -128,6 +128,7 @@ char *ext2fs_find_block_device(dev_t device)
struct dir_list *list = 0, *new_list = 0;
struct dir_list *current;
char *ret_path = 0;
+ int level = 0;
/*
* Add the starting directories to search...
@@ -154,6 +155,9 @@ char *ext2fs_find_block_device(dev_t device)
if (list == 0) {
list = new_list;
new_list = 0;
+ /* Avoid infinite loop */
+ if (++level > 5)
+ break;
}
}
free_dirlist(&list);
--
1.7.1
On 11/3/11 1:45 AM, Niu wrote:
> hello, Ted
>
> In my rhel5 system, there are lots of loop links in the /dev/.udev/failed folder, which makes the e2fsprogs
> 'make check' stuck in 't_ext_jnl_rm'. I'm not sure if the loop links are generated by udev defect or misconfiguration,
> but anyway, I think the ext2fs_find_block_device() should do some sanity check to avoid infinite loop.
>
> I made a simple patch which breaks the loop when depth reaching 5 in ext2fs_find_block_device(), any comments? Thank you.
My only concern would be that depth 5 isn't totally unreasonable in real life, and this causes it to silently stop searching, right?
Would there be much harm in making the limit much higher, to be fairly sure that it has wandered off into the weeds?
I guess this function isn't very frequently called outside of make check though, just from tune2fs' remove_journal_device() (for external journals I guess?) so I suppose it's not too worrisome.
-Eric
> From 5ef3b82266e7fb5edbce4febc4924b0beaccacf1 Mon Sep 17 00:00:00 2001
> From: Niu Yawei <[email protected]>
> Date: Wed, 2 Nov 2011 04:31:11 +0800
> Subject: [PATCH] Avoid infinite loop in ext2fs_find_block_device()
>
> The ext2fs_find_block_device() should stop searching when the
> directory depth reaching a certain threshold, otherwise, it could
> run into infinite loop if there are loop links in the device
> directory.
>
> Signed-off-by: Niu Yawei <[email protected]>
> ---
> lib/ext2fs/finddev.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/lib/ext2fs/finddev.c b/lib/ext2fs/finddev.c
> index 13ef14b..61f9754 100644
> --- a/lib/ext2fs/finddev.c
> +++ b/lib/ext2fs/finddev.c
> @@ -128,6 +128,7 @@ char *ext2fs_find_block_device(dev_t device)
> struct dir_list *list = 0, *new_list = 0;
> struct dir_list *current;
> char *ret_path = 0;
> + int level = 0;
>
> /*
> * Add the starting directories to search...
> @@ -154,6 +155,9 @@ char *ext2fs_find_block_device(dev_t device)
> if (list == 0) {
> list = new_list;
> new_list = 0;
> + /* Avoid infinite loop */
> + if (++level > 5)
> + break;
> }
> }
> free_dirlist(&list);
On Nov 3, 2011, at 10:47 AM, Eric Sandeen wrote:
>
> My only concern would be that depth 5 isn't totally unreasonable in real life, and this causes it to silently stop searching, right?
> Would there be much harm in making the limit much higher, to be fairly sure that it has wandered off into the weeds?
Agreed, the kernel currently uses a limit of 8. And we should use a #define for this in lib/ext2fs/ext2fsP.h, and use it for both finddev.c and lib/ext2fs/namei.c.
-- Ted
>From 81bfd58b3980f940c23f87f891365a289df776ec Mon Sep 17 00:00:00 2001
From: Niu Yawei <[email protected]>
Date: Wed, 2 Nov 2011 04:31:11 +0800
Subject: [PATCH] e2fsprogs: maximum nested link count
Define EXT2FS_MAX_NESTED_LINKS as 8, and check the link count
not exceeding it in ext2fs_find_block_device() and follow_link().
Signed-off-by: Niu Yawei <[email protected]>
---
lib/ext2fs/ext2fsP.h | 2 ++
lib/ext2fs/finddev.c | 5 +++++
lib/ext2fs/namei.c | 3 ++-
3 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/lib/ext2fs/ext2fsP.h b/lib/ext2fs/ext2fsP.h
index b182d7f..82e1ba0 100644
--- a/lib/ext2fs/ext2fsP.h
+++ b/lib/ext2fs/ext2fsP.h
@@ -11,6 +11,8 @@
#include "ext2fs.h"
+#define EXT2FS_MAX_NESTED_LINKS 8
+
/*
* Badblocks list
*/
diff --git a/lib/ext2fs/finddev.c b/lib/ext2fs/finddev.c
index 13ef14b..311608d 100644
--- a/lib/ext2fs/finddev.c
+++ b/lib/ext2fs/finddev.c
@@ -34,6 +34,7 @@
#include "ext2_fs.h"
#include "ext2fs.h"
+#include "ext2fsP.h"
struct dir_list {
char *name;
@@ -128,6 +129,7 @@ char *ext2fs_find_block_device(dev_t device)
struct dir_list *list = 0, *new_list = 0;
struct dir_list *current;
char *ret_path = 0;
+ int level = 0;
/*
* Add the starting directories to search...
@@ -154,6 +156,9 @@ char *ext2fs_find_block_device(dev_t device)
if (list == 0) {
list = new_list;
new_list = 0;
+ /* Avoid infinite loop */
+ if (++level >= EXT2FS_MAX_NESTED_LINKS)
+ break;
}
}
free_dirlist(&list);
diff --git a/lib/ext2fs/namei.c b/lib/ext2fs/namei.c
index 6bbb124..a936474 100644
--- a/lib/ext2fs/namei.c
+++ b/lib/ext2fs/namei.c
@@ -20,6 +20,7 @@
#include "ext2_fs.h"
#include "ext2fs.h"
+#include "ext2fsP.h"
static errcode_t open_namei(ext2_filsys fs, ext2_ino_t root, ext2_ino_t base,
const char *pathname, size_t pathlen, int follow,
@@ -45,7 +46,7 @@ static errcode_t follow_link(ext2_filsys fs, ext2_ino_t root, ext2_ino_t dir,
*res_inode = inode;
return 0;
}
- if (link_count++ > 5) {
+ if (link_count++ >= EXT2FS_MAX_NESTED_LINKS) {
return EXT2_ET_SYMLINK_LOOP;
}
/* FIXME-64: Actually, this is FIXME EXTENTS */
--
1.7.1
> On Nov 3, 2011, at 10:47 AM, Eric Sandeen wrote:
>
>> My only concern would be that depth 5 isn't totally unreasonable in real life, and this causes it to silently stop searching, right?
>> Would there be much harm in making the limit much higher, to be fairly sure that it has wandered off into the weeds?
> Agreed, the kernel currently uses a limit of 8. And we should use a #define for this in lib/ext2fs/ext2fsP.h, and use it for both finddev.c and lib/ext2fs/namei.c.
>
> -- Ted
>
On Fri, Nov 04, 2011 at 08:50:38PM +0800, Niu wrote:
> From 81bfd58b3980f940c23f87f891365a289df776ec Mon Sep 17 00:00:00 2001
> From: Niu Yawei <[email protected]>
> Date: Wed, 2 Nov 2011 04:31:11 +0800
> Subject: [PATCH] e2fsprogs: maximum nested link count
>
> Define EXT2FS_MAX_NESTED_LINKS as 8, and check the link count
> not exceeding it in ext2fs_find_block_device() and follow_link().
>
> Signed-off-by: Niu Yawei <[email protected]>
Thanks, applied.
- Ted
On Fri, 4 Nov 2011 at 20:50, Niu wrote:
> diff --git a/lib/ext2fs/ext2fsP.h b/lib/ext2fs/ext2fsP.h
> index b182d7f..82e1ba0 100644
> --- a/lib/ext2fs/ext2fsP.h
> +++ b/lib/ext2fs/ext2fsP.h
> @@ -11,6 +11,8 @@
>
> #include "ext2fs.h"
>
[...]
> --- a/lib/ext2fs/finddev.c
> +++ b/lib/ext2fs/finddev.c
> @@ -34,6 +34,7 @@
>
> #include "ext2_fs.h"
> #include "ext2fs.h"
> +#include "ext2fsP.h"
On the very real chance of making a fool of myself: if ext2fsP.h includes
ext2fs.h and now finddev.c will include ext2fsP.h - shouldn't we omit the '#include
"ext2fs.h"' in finddev.c (and namei.c) now?
I guess the compiler/preprocessor will figure this out, just wondering about
code cleanliness. Then again we have names like 'ext2_fs.h' and
'ext2fs.h', I guess only an insider will know the difference :-)
Christian.
--
BOFH excuse #97:
Small animal kamikaze attack on power supplies