Le mardi 11 janvier 2011 17:16:20, Eric Blake a écrit :
> On 01/11/2011 09:05 AM, Jim Meyering wrote:
> >> BTW do you agree with merging my fist two patch?
> >
> > I haven't looked carefully, but this use of sprintf will
> > dereference NULL when malloc fails:
> >
> > - char *result = (bufsize < OPENAT_BUFFER_SIZE ? buf : xmalloc
> > (bufsize)); + char *result = (bufsize < OPENAT_BUFFER_SIZE ? buf :
> > malloc (bufsize));
> >
> > sprintf (result, PROC_SELF_FD_FORMAT, fd, file);
>
> The idea of making openat() fail with ENOMEM instead of calling
> xalloc_die() is reasonable to me (POSIX doesn't explicitly document
> ENOMEM as a typical error from openat(), but it DOES have a global
> statement that any standardized functions can return errno values not
> documented in the standard provided that the value returned is accurate
> to the error in question and more specific than any of the documented
> errors).
I have also corrected a bug openat not testing NULL path. I return EFAULT like
my Linux here.
>
> But like Jim said, it means you now have to do NULL checks throughout
> the rest of the openat implementation, in order to properly fail with
> ENOMEM instead of crashing. If a caller really cares about detecting
> ENOMEM, they can call xalloc_die themselves (but more likely, if
> openat() fails with ENOMEM, an app that uses xmalloc will likely fail
> with xalloc_die shortly thereafter on the next attempted allocation,
> even if such allocation is preparing the error() message for the
> detected failure of openat()).
Revised patch here
> And fixing the xmalloc/malloc problem is only half the problem, you
> still have to address the problem of chdir() failures before you've
> completely made the openat() emulation robust. I also agree with your
> approach of separating the two issues into separate patches.
I know
Bastien
From 0c638872ae7d33a36c6548c720aa1333b7683510 Mon Sep 17 00:00:00 2001
From: Bastien ROUCARIES <roucaries.bastien+gnu...@gmail.com>
Date: Tue, 11 Jan 2011 18:15:44 +0100
Subject: [PATCH 2/3] Reject early NULL path and empty path in *at function
Reject early and set errno when user pass a NULL string or empty string
as path for *at function.
Previous version leads to a NULL deference in case of NULL string.
---
lib/at-func.c | 14 ++++++++++++++
lib/at-func2.c | 15 ++++++++++++++-
lib/openat-proc.c | 7 -------
lib/openat.c | 15 +++++++++++++++
4 files changed, 43 insertions(+), 8 deletions(-)
diff --git a/lib/at-func.c b/lib/at-func.c
index 31a75f1..f7e2667 100644
--- a/lib/at-func.c
+++ b/lib/at-func.c
@@ -71,6 +71,20 @@ AT_FUNC_NAME (int fd, char const *file AT_FUNC_POST_FILE_PARAM_DECLS)
if (fd == AT_FDCWD || IS_ABSOLUTE_FILE_NAME (file))
return CALL_FUNC (file);
+
+ /* NULL string are forbidden */
+ if (file == NULL)
+ {
+ errno = EFAULT;
+ return FUNC_FAIL;
+ }
+
+ /* empty string */
+ if (!*file)
+ {
+ errno = ENOENT;
+ return FUNC_FAIL;
+ }
{
char proc_buf[OPENAT_BUFFER_SIZE];
diff --git a/lib/at-func2.c b/lib/at-func2.c
index 29e6772..1cb55cc 100644
--- a/lib/at-func2.c
+++ b/lib/at-func2.c
@@ -73,11 +73,24 @@ at_func2 (int fd1, char const *file1,
Try some optimizations to reduce fd to AT_FDCWD, or to at least
avoid converting an absolute name or doing a double chdir. */
-
if ((fd1 == AT_FDCWD || IS_ABSOLUTE_FILE_NAME (file1))
&& (fd2 == AT_FDCWD || IS_ABSOLUTE_FILE_NAME (file2)))
return func (file1, file2); /* Case 0-2, 4-6, 8-10. */
+ /* NULL string are forbidden */
+ if (file1 == NULL || file2 == NULL)
+ {
+ errno = EFAULT;
+ return -1;
+ }
+
+ /* empty string */
+ if (!*file1 || !*file2)
+ {
+ errno = ENOENT;
+ return -1;
+ }
+
/* If /proc/self/fd works, we don't need any stat or chdir. */
{
char proc_buf1[OPENAT_BUFFER_SIZE];
diff --git a/lib/openat-proc.c b/lib/openat-proc.c
index d543491..9bd88bf 100644
--- a/lib/openat-proc.c
+++ b/lib/openat-proc.c
@@ -64,13 +64,6 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file)
{
static int proc_status = 0;
- /* Make sure the caller gets ENOENT when appropriate. */
- if (!*file)
- {
- buf[0] = '\0';
- return buf;
- }
-
if (! proc_status)
{
/* Set PROC_STATUS to a positive value if /proc/self/fd is
diff --git a/lib/openat.c b/lib/openat.c
index 55e12e2..64b1dca 100644
--- a/lib/openat.c
+++ b/lib/openat.c
@@ -174,6 +174,21 @@ openat_permissive (int fd, char const *file, int flags, mode_t mode,
if (fd == AT_FDCWD || IS_ABSOLUTE_FILE_NAME (file))
return open (file, flags, mode);
+
+ /* NULL string are forbidden */
+ if (file == NULL)
+ {
+ errno = EFAULT;
+ return -1;
+ }
+
+ /* empty string */
+ if (!*file)
+ {
+ errno = ENOENT;
+ return -1;
+ }
+
{
char buf[OPENAT_BUFFER_SIZE];
--
1.7.1
From 38458bc47a2560f697dbb017cddcaeac32d9bb63 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bastien=20ROUCARI=C3=88S?= <roucaries.bastien+gnu...@gmail.com>
Date: Tue, 11 Jan 2011 17:39:12 +0100
Subject: [PATCH 1/3] Avoid xmalloc in openat implementation
Do not call xmalloc but return NULL in case of error. Caller are safe
and fallback to fchdir implementation.
---
lib/openat-proc.c | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/lib/openat-proc.c b/lib/openat-proc.c
index 51a8aa2..d543491 100644
--- a/lib/openat-proc.c
+++ b/lib/openat-proc.c
@@ -28,6 +28,7 @@
#include <stdio.h>
#include <string.h>
#include <unistd.h>
+#include <stdlib.h>
#include "dirname.h"
#include "intprops.h"
@@ -42,6 +43,11 @@
#undef open
#undef close
+/* In this file we always malloc a stricly positive size.
+ repl_malloc(size >0) and malloc(size >0) are equivalent
+ Use therefore use the system malloc. */
+#undef malloc
+
#define PROC_SELF_FD_FORMAT "/proc/self/fd/%d/%s"
#define PROC_SELF_FD_NAME_SIZE_BOUND(len) \
@@ -98,7 +104,9 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file)
else
{
size_t bufsize = PROC_SELF_FD_NAME_SIZE_BOUND (strlen (file));
- char *result = (bufsize < OPENAT_BUFFER_SIZE ? buf : xmalloc (bufsize));
+ char *result = (bufsize < OPENAT_BUFFER_SIZE ? buf : malloc (bufsize));
+ if (NULL == result)
+ return NULL;
sprintf (result, PROC_SELF_FD_FORMAT, fd, file);
return result;
}
--
1.7.1
From 8897b4ea3b14cce62493c9f439c841a3f131a6ae Mon Sep 17 00:00:00 2001
From: Bastien ROUCARIES <roucaries.bastien+gnu...@gmail.com>
Date: Tue, 11 Jan 2011 18:32:19 +0100
Subject: [PATCH 3/3] Bail out early in case of ENOMEM in openat_proc_name
Return early in caller functions of openat_proc_name in case of unexpected error
---
lib/at-func.c | 4 ++++
lib/at-func2.c | 12 ++++++++++++
lib/openat-proc.c | 10 ++++++++--
lib/openat.c | 4 ++++
4 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/lib/at-func.c b/lib/at-func.c
index f7e2667..7dadf54 100644
--- a/lib/at-func.c
+++ b/lib/at-func.c
@@ -89,6 +89,10 @@ AT_FUNC_NAME (int fd, char const *file AT_FUNC_POST_FILE_PARAM_DECLS)
{
char proc_buf[OPENAT_BUFFER_SIZE];
char *proc_file = openat_proc_name (proc_buf, fd, file);
+
+ if (!proc_file && errno != ENOTSUP)
+ return FUNC_FAIL;
+
if (proc_file)
{
FUNC_RESULT proc_result = CALL_FUNC (proc_file);
diff --git a/lib/at-func2.c b/lib/at-func2.c
index 1cb55cc..1c3610b 100644
--- a/lib/at-func2.c
+++ b/lib/at-func2.c
@@ -97,12 +97,24 @@ at_func2 (int fd1, char const *file1,
char *proc_file1 = ((fd1 == AT_FDCWD || IS_ABSOLUTE_FILE_NAME (file1))
? (char *) file1
: openat_proc_name (proc_buf1, fd1, file1));
+
+ if (!proc_file1 && errno != ENOTSUP)
+ return -1;
+
if (proc_file1)
{
char proc_buf2[OPENAT_BUFFER_SIZE];
char *proc_file2 = ((fd2 == AT_FDCWD || IS_ABSOLUTE_FILE_NAME (file2))
? (char *) file2
: openat_proc_name (proc_buf2, fd2, file2));
+
+ if (!proc_file2 && errno != ENOTSUP)
+ {
+ if (proc_file1 != proc_buf1 && proc_file1 != file1)
+ free(proc_file1);
+ return -1;
+ }
+
if (proc_file2)
{
int proc_result = func (proc_file1, proc_file2);
diff --git a/lib/openat-proc.c b/lib/openat-proc.c
index 9bd88bf..c945c1c 100644
--- a/lib/openat-proc.c
+++ b/lib/openat-proc.c
@@ -93,13 +93,19 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file)
}
if (proc_status < 0)
- return NULL;
+ {
+ errno = ENOTSUP;
+ return NULL;
+ }
else
{
size_t bufsize = PROC_SELF_FD_NAME_SIZE_BOUND (strlen (file));
char *result = (bufsize < OPENAT_BUFFER_SIZE ? buf : malloc (bufsize));
if (NULL == result)
- return NULL;
+ {
+ errno = ENOMEM;
+ return NULL;
+ }
sprintf (result, PROC_SELF_FD_FORMAT, fd, file);
return result;
}
diff --git a/lib/openat.c b/lib/openat.c
index 64b1dca..477abf2 100644
--- a/lib/openat.c
+++ b/lib/openat.c
@@ -193,6 +193,10 @@ openat_permissive (int fd, char const *file, int flags, mode_t mode,
{
char buf[OPENAT_BUFFER_SIZE];
char *proc_file = openat_proc_name (buf, fd, file);
+
+ if (!proc_file && errno != ENOTSUP)
+ return -1;
+
if (proc_file)
{
int open_result = open (proc_file, flags, mode);
--
1.7.1
