| Message ID | ME3P282MB39685DC6FC209A91F072BA3CD3719@ME3P282MB3968.AUSP282.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | elf: Fix marking root dir as nonexist in open_path | expand |
On 5/8/23 11:45, Moody Liu via Libc-alpha wrote: > When dlopen is being called, efforts have been made to improve > future lookup performance. This includes marking a search path > as non-existent using `stat`. However, if the root directory > is given as a search path, there exists a bug which erroneously > marks it as non-existing. Thanks for working on the patch. Could you please file a bug in bugzilla for this? > > The bug is reproduced under the following sequence: > > 1. dlopen is called to open a shared library, with at least: > 1) a dependency 'A.so' not directly under the '/' directory > (e.g. /lib/A.so), and > 2) another dependency 'B.so' resides in '/'. Are you able to write a containerized test case for this? We have tests-container testing for just such "/" scenarios with a distinct mount namespace for the tests. > 2. for this bug to reproduce, 'A.so' should be searched *before* 'B.so'. > 3. it first tries to find 'A.so' in /, (e.g. /A.so): > - this will (obviously) fail, > - since it's the first time we have seen the '/' directory, > its 'status' is 'unknown'. > 4. `buf[buflen - namelen - 1] = '\0'` is executed: > - it intends to remove the leaf and its final slash, > - because of the speciality of '/', its buflen == namelen + 1, > - it erroneously clears the entire buffer. > 6. it then calls 'stat' with the empty buffer: > - which will result in an error. > 7. so it marks '/' as 'nonexisting', future lookups will not consider > this path. > 8. while /B.so *does* exist, failure to look it up in the '/' > directory leads to a 'cannot open shared object file' error. > > This patch fixes the bug by preventing 'buflen', an index to put '\0', > from being set to 0, so that the root '/' is always kept. > Relative search paths are always considered as 'existing' so this > wont be affected. > > Suggested-by: Qixing ksyx Xue <qixingxue@outlook.com> > --- > elf/dl-load.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/elf/dl-load.c b/elf/dl-load.c > index fcb39a78d4..10757dd5a5 100644 > --- a/elf/dl-load.c > +++ b/elf/dl-load.c > @@ -1865,7 +1865,11 @@ open_path (const char *name, size_t namelen, int mode, > test whether there is any directory at all. */ > struct __stat64_t64 st; > > - buf[buflen - namelen - 1] = '\0'; > + /* We only have absolute paths go into this branch. > + In the rare case where 'this_dir' is only a '/', we > + must keep it. */ > + buflen = MAX(buflen - namelen - 1, 1); > + buf[buflen] = '\0'; > > if (__stat64_time64 (buf, &st) != 0 > || ! S_ISDIR (st.st_mode))
diff --git a/elf/dl-load.c b/elf/dl-load.c index fcb39a78d4..10757dd5a5 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -1865,7 +1865,11 @@ open_path (const char *name, size_t namelen, int mode, test whether there is any directory at all. */ struct __stat64_t64 st; - buf[buflen - namelen - 1] = '\0'; + /* We only have absolute paths go into this branch. + In the rare case where 'this_dir' is only a '/', we + must keep it. */ + buflen = MAX(buflen - namelen - 1, 1); + buf[buflen] = '\0'; if (__stat64_time64 (buf, &st) != 0 || ! S_ISDIR (st.st_mode))
When dlopen is being called, efforts have been made to improve future lookup performance. This includes marking a search path as non-existent using `stat`. However, if the root directory is given as a search path, there exists a bug which erroneously marks it as non-existing. The bug is reproduced under the following sequence: 1. dlopen is called to open a shared library, with at least: 1) a dependency 'A.so' not directly under the '/' directory (e.g. /lib/A.so), and 2) another dependency 'B.so' resides in '/'. 2. for this bug to reproduce, 'A.so' should be searched *before* 'B.so'. 3. it first tries to find 'A.so' in /, (e.g. /A.so): - this will (obviously) fail, - since it's the first time we have seen the '/' directory, its 'status' is 'unknown'. 4. `buf[buflen - namelen - 1] = '\0'` is executed: - it intends to remove the leaf and its final slash, - because of the speciality of '/', its buflen == namelen + 1, - it erroneously clears the entire buffer. 6. it then calls 'stat' with the empty buffer: - which will result in an error. 7. so it marks '/' as 'nonexisting', future lookups will not consider this path. 8. while /B.so *does* exist, failure to look it up in the '/' directory leads to a 'cannot open shared object file' error. This patch fixes the bug by preventing 'buflen', an index to put '\0', from being set to 0, so that the root '/' is always kept. Relative search paths are always considered as 'existing' so this wont be affected. Suggested-by: Qixing ksyx Xue <qixingxue@outlook.com> --- elf/dl-load.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)