[patch 0/3] [RFC] kernel/glibc mismatch of "readlink" syscall?

[patch 0/3] [RFC] kernel/glibc mismatch of "readlink" syscall?

[Daniel Gollub]

Hi,

found something which looks for me like a kernel/glibc syscall mismatch. At
least the parameter list of "readlink" is different in the kernel compared to
glibc, POSIX and linux-man-pages. I'm not quite sure if this difference was
intended or not ... 

man 3p readlink:
ssize_t readlink(const char *restrict path, char *restrict buf, size_t bufsize);

http://www.opengroup.org/onlinepubs/000095399/functions/readlink.html:
size_t readlink(const char *restrict path, char *restrict buf, size_t bufsize);

glibc (/usr/include/unistd.h):
size_t readlink (__const char *__restrict __path, char *__restrict __buf, size_t

man 2 readlink:
ssize_t readlink(const char *path, char *buf, size_t bufsiz);
                                              ^^^^^^
linux-2.6/include/linux/syscalls.h:
asmlinkage long sys_readlink(const char __user *path, char __user *buf, int
		bufsiz);                                                ^^^


All readlink prototypes, expect the one in the kernel, have an unsigned
buffer size. Even the readlink(2) man-page, which also describes an error
statement like this:

EINVAL bufsiz is not positive.

Note: the same man-page defined bufsiz as type of size_t (unsigned).

While reviewing LTP i discovered that the "readlink03" syscall test contains a 
testcase to do a functional error-path test for "EINVAL bufsiz is not positive".
This testcase is using the glibc readlink() interface, which cause a unsigned
cast of the value "-1" and let the testcase fail (actually due to gcc/glibc
fortify checks and cause a __chk_fail()).

Before workarounding the testcase, or not applying -D_FORTIFY_SOURCE=2 on LTP
build, i try to understand if there is any reason for this mismatch between
kernel and glibc/POSIX. Regarding the man-page, i'm quite certain this was a
copy&paste-error by coping the prototype from the POSIX man-page.

Even sys_readlinkat(), which got introduced a long time after sys_readlink(),
got a signed buffer size. Intended?

In the rare case all this was unintended, find patches for kernel, man-pages
and LTP to change the kernel readlink syscall interface to a unsigned buffer
size.

Thoughts?

best regards,
Daniel

[patch 1/3] [RFC] Change sys_readlink/sys_readlinkat buffer size parameter to size_t (POSIX mismatch)

[Daniel Gollub]

[-- Attachment #1: linux-readlink-bufsize-size_t.diff --]
[-- Type: text/plain, Size: 9818 bytes --]

man-page for readlink syscalls prototype with buffer size as type of size_t:
"ssize_t readlink(const char *path, char *buf, size_t bufsiz);"

Kernel readlink syscall interface has signed buffer size and doesn't match with
man-page described prototype.

POSIX and glibc implementation of the readlink interface have an unsigned buffer size.

This patch changes the syscall interface of sys_readlink and sys_readlinkat, by
changing the buffer size argument type to "size_t".

A buffer size of 0 will end in return value 0. This is _different_ behavior to
original sys_readlink/sys_readlinkat interface. But equal/similar to POSIXs one.


Signed-off-by: Daniel Gollub <dgollub@suse.de>


---

fs/9p/vfs_inode.c        |    4 ++--
 fs/bad_inode.c           |    2 +-
 fs/ecryptfs/inode.c      |    2 +-
 fs/gfs2/ops_inode.c      |    2 +-
 fs/hppfs/hppfs.c         |    2 +-
 fs/namei.c               |    8 ++++----
 fs/ocfs2/symlink.c       |    2 +-
 fs/proc/base.c           |    6 +++---
 fs/stat.c                |    8 ++++----
 include/linux/fs.h       |    8 ++++----
 include/linux/syscalls.h |    4 ++--
 11 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index e83aa5e..dd70525 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -898,7 +898,7 @@ ino_t v9fs_qid2ino(struct p9_qid *qid)
  *
  */
 
-static int v9fs_readlink(struct dentry *dentry, char *buffer, int buflen)
+static int v9fs_readlink(struct dentry *dentry, char *buffer, size_t buflen)
 {
 	int retval;
 
@@ -952,7 +952,7 @@ done:
  */
 
 static int v9fs_vfs_readlink(struct dentry *dentry, char __user * buffer,
-			     int buflen)
+			     size_t buflen)
 {
 	int retval;
 	int ret;
diff --git a/fs/bad_inode.c b/fs/bad_inode.c
index 5f1538c..5342a55 100644
--- a/fs/bad_inode.c
+++ b/fs/bad_inode.c
@@ -238,7 +238,7 @@ static int bad_inode_rename (struct inode *old_dir, struct dentry *old_dentry,
 }
 
 static int bad_inode_readlink(struct dentry *dentry, char __user *buffer,
-		int buflen)
+		size_t buflen)
 {
 	return -EIO;
 }
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 89209f0..378a1e2 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -602,7 +602,7 @@ out_lock:
 }
 
 static int
-ecryptfs_readlink(struct dentry *dentry, char __user * buf, int bufsiz)
+ecryptfs_readlink(struct dentry *dentry, char __user * buf, size_t bufsiz)
 {
 	int rc;
 	struct dentry *lower_dentry;
diff --git a/fs/gfs2/ops_inode.c b/fs/gfs2/ops_inode.c
index 534e1e2..8680062 100644
--- a/fs/gfs2/ops_inode.c
+++ b/fs/gfs2/ops_inode.c
@@ -894,7 +894,7 @@ out:
  */
 
 static int gfs2_readlink(struct dentry *dentry, char __user *user_buf,
-			 int user_size)
+			 size_t user_size)
 {
 	struct gfs2_inode *ip = GFS2_I(dentry->d_inode);
 	char array[GFS2_FAST_NAME_SIZE], *buf = array;
diff --git a/fs/hppfs/hppfs.c b/fs/hppfs/hppfs.c
index 2b3d182..488f95d 100644
--- a/fs/hppfs/hppfs.c
+++ b/fs/hppfs/hppfs.c
@@ -637,7 +637,7 @@ static const struct super_operations hppfs_sbops = {
 };
 
 static int hppfs_readlink(struct dentry *dentry, char __user *buffer,
-			  int buflen)
+			  size_t buflen)
 {
 	struct dentry *proc_dentry;
 
diff --git a/fs/namei.c b/fs/namei.c
index 4ea63ed..63e01e4 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2712,7 +2712,7 @@ asmlinkage long sys_rename(const char __user *oldname, const char __user *newnam
 	return sys_renameat(AT_FDCWD, oldname, AT_FDCWD, newname);
 }
 
-int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen, const char *link)
+int vfs_readlink(struct dentry *dentry, char __user *buffer, size_t buflen, const char *link)
 {
 	int len;
 
@@ -2721,7 +2721,7 @@ int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen, const c
 		goto out;
 
 	len = strlen(link);
-	if (len > (unsigned) buflen)
+	if (len > buflen)
 		len = buflen;
 	if (copy_to_user(buffer, link, len))
 		len = -EFAULT;
@@ -2734,7 +2734,7 @@ out:
  * have ->follow_link() touching nd only in nd_set_link().  Using (or not
  * using) it for any given inode is up to filesystem.
  */
-int generic_readlink(struct dentry *dentry, char __user *buffer, int buflen)
+int generic_readlink(struct dentry *dentry, char __user *buffer, size_t buflen)
 {
 	struct nameidata nd;
 	void *cookie;
@@ -2768,7 +2768,7 @@ static char *page_getlink(struct dentry * dentry, struct page **ppage)
 	return kmap(page);
 }
 
-int page_readlink(struct dentry *dentry, char __user *buffer, int buflen)
+int page_readlink(struct dentry *dentry, char __user *buffer, size_t buflen)
 {
 	struct page *page = NULL;
 	char *s = page_getlink(dentry, &page);
diff --git a/fs/ocfs2/symlink.c b/fs/ocfs2/symlink.c
index cbd03df..3295ff7 100644
--- a/fs/ocfs2/symlink.c
+++ b/fs/ocfs2/symlink.c
@@ -101,7 +101,7 @@ bail:
 
 static int ocfs2_readlink(struct dentry *dentry,
 			  char __user *buffer,
-			  int buflen)
+			  size_t buflen)
 {
 	int ret;
 	char *link;
diff --git a/fs/proc/base.c b/fs/proc/base.c
index b5918ae..14b90b9 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1333,7 +1333,7 @@ out:
 	return ERR_PTR(error);
 }
 
-static int do_proc_readlink(struct path *path, char __user *buffer, int buflen)
+static int do_proc_readlink(struct path *path, char __user *buffer, size_t buflen)
 {
 	char *tmp = (char*)__get_free_page(GFP_TEMPORARY);
 	char *pathname;
@@ -1357,7 +1357,7 @@ static int do_proc_readlink(struct path *path, char __user *buffer, int buflen)
 	return len;
 }
 
-static int proc_pid_readlink(struct dentry * dentry, char __user * buffer, int buflen)
+static int proc_pid_readlink(struct dentry * dentry, char __user * buffer, size_t buflen)
 {
 	int error = -EACCES;
 	struct inode *inode = dentry->d_inode;
@@ -2246,7 +2246,7 @@ static const struct file_operations proc_coredump_filter_operations = {
  * /proc/self:
  */
 static int proc_self_readlink(struct dentry *dentry, char __user *buffer,
-			      int buflen)
+			      size_t buflen)
 {
 	struct pid_namespace *ns = dentry->d_sb->s_fs_info;
 	pid_t tgid = task_tgid_nr_ns(current, ns);
diff --git a/fs/stat.c b/fs/stat.c
index 7c46fbe..b521674 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -292,13 +292,13 @@ asmlinkage long sys_newfstat(unsigned int fd, struct stat __user *statbuf)
 }
 
 asmlinkage long sys_readlinkat(int dfd, const char __user *pathname,
-				char __user *buf, int bufsiz)
+				char __user *buf, size_t bufsiz)
 {
 	struct path path;
 	int error;
 
-	if (bufsiz <= 0)
-		return -EINVAL;
+	if (unlikely(!bufsiz))
+		return 0;
 
 	error = user_path_at(dfd, pathname, 0, &path);
 	if (!error) {
@@ -319,7 +319,7 @@ asmlinkage long sys_readlinkat(int dfd, const char __user *pathname,
 }
 
 asmlinkage long sys_readlink(const char __user *path, char __user *buf,
-				int bufsiz)
+				size_t bufsiz)
 {
 	return sys_readlinkat(AT_FDCWD, path, buf, bufsiz);
 }
diff --git a/include/linux/fs.h b/include/linux/fs.h
index a6a625b..816dc29 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1333,7 +1333,7 @@ struct inode_operations {
 	int (*mknod) (struct inode *,struct dentry *,int,dev_t);
 	int (*rename) (struct inode *, struct dentry *,
 			struct inode *, struct dentry *);
-	int (*readlink) (struct dentry *, char __user *,int);
+	int (*readlink) (struct dentry *, char __user *,size_t);
 	void * (*follow_link) (struct dentry *, struct nameidata *);
 	void (*put_link) (struct dentry *, struct nameidata *, void *);
 	void (*truncate) (struct inode *);
@@ -2029,16 +2029,16 @@ extern const struct file_operations generic_ro_fops;
 
 #define special_file(m) (S_ISCHR(m)||S_ISBLK(m)||S_ISFIFO(m)||S_ISSOCK(m))
 
-extern int vfs_readlink(struct dentry *, char __user *, int, const char *);
+extern int vfs_readlink(struct dentry *, char __user *, size_t, const char *);
 extern int vfs_follow_link(struct nameidata *, const char *);
-extern int page_readlink(struct dentry *, char __user *, int);
+extern int page_readlink(struct dentry *, char __user *, size_t);
 extern void *page_follow_link_light(struct dentry *, struct nameidata *);
 extern void page_put_link(struct dentry *, struct nameidata *, void *);
 extern int __page_symlink(struct inode *inode, const char *symname, int len,
 		gfp_t gfp_mask);
 extern int page_symlink(struct inode *inode, const char *symname, int len);
 extern const struct inode_operations page_symlink_inode_operations;
-extern int generic_readlink(struct dentry *, char __user *, int);
+extern int generic_readlink(struct dentry *, char __user *, size_t);
 extern void generic_fillattr(struct inode *, struct kstat *);
 extern int vfs_getattr(struct vfsmount *, struct dentry *, struct kstat *);
 void inode_add_bytes(struct inode *inode, loff_t bytes);
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index d6ff145..e9582d1 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -326,7 +326,7 @@ asmlinkage ssize_t sys_sendfile(int out_fd, int in_fd,
 asmlinkage ssize_t sys_sendfile64(int out_fd, int in_fd,
 				loff_t __user *offset, size_t count);
 asmlinkage long sys_readlink(const char __user *path,
-				char __user *buf, int bufsiz);
+				char __user *buf, size_t bufsiz);
 asmlinkage long sys_creat(const char __user *pathname, int mode);
 asmlinkage long sys_open(const char __user *filename,
 				int flags, int mode);
@@ -581,7 +581,7 @@ asmlinkage long sys_newfstatat(int dfd, char __user *filename,
 asmlinkage long sys_fstatat64(int dfd, char __user *filename,
 			       struct stat64 __user *statbuf, int flag);
 asmlinkage long sys_readlinkat(int dfd, const char __user *path, char __user *buf,
-			       int bufsiz);
+			       size_t bufsiz);
 asmlinkage long sys_utimensat(int dfd, char __user *filename,
 				struct timespec __user *utimes, int flags);
 asmlinkage long compat_sys_futimesat(unsigned int dfd, char __user *filename,

[patch 2/3] [man-pages] Remove "bufsize is not positive" from readlink(2) error section

[Daniel Gollub]

[-- Attachment #1: man-pages-readlink-2.diff --]
[-- Type: text/plain, Size: 778 bytes --]

The error statement "bufsize is not positive" is obsolete, if this proposed
syscall interface change get accepted.

Note: Kernel 2.6.27 and older shipped sys_readlink with a signed "bufsiz" 
argument. Man-page document the readlink syscall always as unsigend (size_t).

Signed-off-by: Daniel Gollub <dgollub@suse.de>

---

 man2/readlink.2 |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/man2/readlink.2 b/man2/readlink.2
index b94db21..470c222 100644
--- a/man2/readlink.2
+++ b/man2/readlink.2
@@ -86,10 +86,6 @@ Search permission is denied for a component of the path prefix.
 extends outside the process's allocated address space.
 .TP
 .B EINVAL
-.I bufsiz
-is not positive.
-.TP
-.B EINVAL
 The named file is not a symbolic link.
 .TP
 .B EIO

[patch 3/3] [ltp] Remove testcase for "Buffer size is not positive" in kernel/syscalls/readlink/readlink03

[Daniel Gollub]

[-- Attachment #1: syscalls-readlink-bufsize-mismatch.diff --]
[-- Type: text/plain, Size: 1707 bytes --]

Remove testcase for "Buffer size is not positive" in testcase readlink03, since
buffer size argument is type of size_t (unsigned) and negative value are not
possible. With proposed sys_readlink modification a buffer size of 0 ends in a
return value of 0, not -1 anymore.

(This modification depends on man-pages and linux syscalls modifications,
 provided by this patch queue)

Signed-off-by: Daniel Gollub <dgollub@suse.de>


---
 testcases/kernel/syscalls/readlink/readlink03.c |    3 ---
 1 file changed, 3 deletions(-)

Index: ltp-full-20081017/testcases/kernel/syscalls/readlink/readlink03.c
===================================================================
--- ltp-full-20081017.orig/testcases/kernel/syscalls/readlink/readlink03.c
+++ ltp-full-20081017/testcases/kernel/syscalls/readlink/readlink03.c
@@ -25,8 +25,6 @@
  *   1) readlink(2) returns -1 and sets errno to EACCES if search/write
  *	permission is denied in the directory where the symbolic link
  *	resides.
- *   2) readlink(2) returns -1 and sets errno to EINVAL if the buffer size
- *	is not positive.
  *   3) readlink(2) returns -1 and sets errno to EINVAL if the specified
  *	file is not a symbolic link file.
  *   4) readlink(2) returns -1 and sets errno to ENAMETOOLONG if the 
@@ -119,7 +117,6 @@ struct test_case_t {		/* test case struc
 	int (*setupfunc)();
 } Test_cases[] = {
 	{ SYM_FILE1, "No Search permissions to process", EACCES, 1, setup1 },
-	{ SYM_FILE2, "Buffer size is not positive", EINVAL, -1, setup2 },
 	{ TEST_FILE2, "File is not symbolic link", EINVAL, 1, no_setup },
 	{ Longpathname, "Symlink path too long", ENAMETOOLONG, 1, lpath_setup },
 	{ "", "Symlink Pathname is empty", ENOENT, 1, no_setup },

Re: [patch 0/3] [RFC] kernel/glibc mismatch of "readlink" syscall?

[Michael Kerrisk]

[Adding the man-pages historian to the CC.]

Hi Daniel,

On Thu, Oct 23, 2008 at 9:50 AM, Daniel Gollub <dgollub@suse.de> wrote:
> Hi,
>
> found something which looks for me like a kernel/glibc syscall mismatch. At
> least the parameter list of "readlink" is different in the kernel compared to
> glibc, POSIX and linux-man-pages. I'm not quite sure if this difference was
> intended or not ...
>
> man 3p readlink:
> ssize_t readlink(const char *restrict path, char *restrict buf, size_t bufsize);
>
> http://www.opengroup.org/onlinepubs/000095399/functions/readlink.html:
> size_t readlink(const char *restrict path, char *restrict buf, size_t bufsize);
>
> glibc (/usr/include/unistd.h):
> size_t readlink (__const char *__restrict __path, char *__restrict __buf, size_t
>
> man 2 readlink:
> ssize_t readlink(const char *path, char *buf, size_t bufsiz);
>                                              ^^^^^^
> linux-2.6/include/linux/syscalls.h:
> asmlinkage long sys_readlink(const char __user *path, char __user *buf, int
>                bufsiz);                                                ^^^
>
>
> All readlink prototypes, expect the one in the kernel, have an unsigned
> buffer size. Even the readlink(2) man-page, which also describes an error
> statement like this:
>
> EINVAL bufsiz is not positive.

I agree; the inconsistency is strange.  Probably it was a historical accident.

> Note: the same man-page defined bufsiz as type of size_t (unsigned).

A little history, as it appears to me...

It looks like the Linux man page came from BSD.  The 4.3BSD man page
documented the type as "int" (and did not document an EINVAL error for
a negative bufsize), and even today the FreeBSD (6.2) man page
documents the type as "int" (and still does not document an EINVAL
error for this case), and that is how the argument is prototyped in
FreeBSD 6.2's <unistd.h>.  (I haven't tested what FreeBSD actually
does with a negative bufsize value.)

In 1993, when Linux man-pages-1.0 took the page from BSD (that page
was timestamped 19991), it looks like someone must have changed the
type of bufsize to "size_t" in the man page SYNOPSIS.  Now that could
be to match Linux libc of the time, which was already using "size_t"
(even though the then current kernel used "int"), or it could have
been to match the current standards (SUSv1, which was based on the
original POSIX.1, documents the type as "size"t").

The EINVAL error was added to man-pages-1.18 in 1997 (even though, as
you note, the type was "size_t").  I suspect (this was well before I
had any association with man-pages) that was done to reflect kernel
reality (since one could bypass glibc invoke the syscall directly),
but obviously it is inconsistent with the prototype.

> While reviewing LTP i discovered that the "readlink03" syscall test contains a
> testcase to do a functional error-path test for "EINVAL bufsiz is not positive".
> This testcase is using the glibc readlink() interface, which cause a unsigned
> cast of the value "-1" and let the testcase fail (actually due to gcc/glibc
> fortify checks and cause a __chk_fail()).
>
> Before workarounding the testcase, or not applying -D_FORTIFY_SOURCE=2 on LTP
> build, i try to understand if there is any reason for this mismatch between
> kernel and glibc/POSIX. Regarding the man-page, i'm quite certain this was a
> copy&paste-error by coping the prototype from the POSIX man-page.

(See above -- the type might have been taken either from POSIX.1 or
glibc, and that might have been quite deliberate.)

> Even sys_readlinkat(), which got introduced a long time after sys_readlink(),
> got a signed buffer size. Intended?

Probably done to match sys_readlink().

> In the rare case all this was unintended, find patches for kernel, man-pages
> and LTP to change the kernel readlink syscall interface to a unsigned buffer
> size.
>
> Thoughts?

Your proposed kernel patch is an ABI change, albeit one that quite
likely would affect no applications.  So it might not hurt any one.
On the other hand, is there a benefit to making the change?

Perhaps the best think to do is simply to add a note to the man page
about this inconsistency.  (It's not sufficient to just remove the
EINVAL error as you propose in one of your patches, since that can
still occur when bypassing glibc.)

Perhaps others have some thoughts?

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [patch 0/3] [RFC] kernel/glibc mismatch of "readlink" syscall?

[Andries E. Brouwer]

On Fri, Oct 24, 2008 at 05:53:25PM -0500, Michael Kerrisk wrote:

> Hi Daniel,
> 
> On Thu, Oct 23, 2008 at 9:50 AM, Daniel Gollub <dgollub@suse.de> wrote:
>>
>> found something which looks for me like a kernel/glibc syscall mismatch.
>>
>> The standard and the manpages and glibc have
>> ssize_t readlink(..., size_t bufsize);
>>
>> But the kernel has
>>
>> linux-2.6/include/linux/syscalls.h:
>> asmlinkage long sys_readlink(..., int bufsiz);
>>
>> All readlink prototypes, expect the one in the kernel, have an unsigned
>> buffer size.
> 
> I agree; the inconsistency is strange.

Hmm. I am inclined not to agree. There is no reason to expect
any particular relation between the kernel prototype of sys_foo
(if such a function exists)
and the user space prototype of the foo() C-library function.

The POSIX standard, man-pages, libc include files document the
userspace / libc interface, not the system call interface.
The system call interface is mostly undocumented.

The kernel prototype is chosen for kernel-internal or historical reasons.

Andries


sys_ppc32.c:
/* Note: it is necessary to treat bufsiz as an unsigned int ... */
asmlinkage long compat_sys_readlink(..., u32 bufsiz)

Re: [patch 0/3] [RFC] kernel/glibc mismatch of "readlink" syscall?

[Kai Henningsen]

Am Fri, 24 Oct 2008 17:53:25 -0500
schrieb "Michael Kerrisk" <mtk.manpages@googlemail.com>:

> Hi Daniel,
> 
> On Thu, Oct 23, 2008 at 9:50 AM, Daniel Gollub <dgollub@suse.de>
> wrote:

> > EINVAL bufsiz is not positive.

> The EINVAL error was added to man-pages-1.18 in 1997 (even though, as
> you note, the type was "size_t").  I suspect (this was well before I
> had any association with man-pages) that was done to reflect kernel
> reality (since one could bypass glibc invoke the syscall directly),
> but obviously it is inconsistent with the prototype.

Actually, it's not inconsistent as described, though perhaps that is
unintentional. "Not positive" isn't the same as "negative", as zero
isn't positive either, and zero is certainly a possible value of an
unsigned type

Re: [patch 0/3] [RFC] kernel/glibc mismatch of "readlink" syscall?

[Daniel Gollub]

On Friday 31 October 2008 16:02:48 Kai Henningsen wrote:
> Am Fri, 24 Oct 2008 17:53:25 -0500
>
> schrieb "Michael Kerrisk" <mtk.manpages@googlemail.com>:
> > Hi Daniel,
> >
> > On Thu, Oct 23, 2008 at 9:50 AM, Daniel Gollub <dgollub@suse.de>
> >
> > wrote:
> > > EINVAL bufsiz is not positive.
> >
> > The EINVAL error was added to man-pages-1.18 in 1997 (even though, as
> > you note, the type was "size_t").  I suspect (this was well before I
> > had any association with man-pages) that was done to reflect kernel
> > reality (since one could bypass glibc invoke the syscall directly),
> > but obviously it is inconsistent with the prototype.
>
> Actually, it's not inconsistent as described, though perhaps that is
> unintentional. "Not positive" isn't the same as "negative", as zero
> isn't positive either, and zero is certainly a possible value of an
> unsigned type

True.

But there is still the problem for the ltp syscall test "readlink03", when 
using the glibc "readlink" interface, by calling readlink with a buffer size 
of "-1".

Calling "-1" seems to be a valid code/error-path in the linux syscall 
"readlink", since there is a check for less-equal zero.

But the less zero, condition can't be reached via the glibc "readlink" 
interface since this would cause fortify-check to fail (when buliding with -
D_FORITFY_SOURCE=2).

To "workaround" the fortify check, by not compiling the testcase with -
D_FORTIFY_SOURCE=2, or trying to test the linux readlink interface by calling 
directly syscall() in the testcase ... both suggestion are just workarounds - 
no real solutions.

We could also just remove the testcase of buffer size "-1".

The problem is still, how to test the "readlink" syscall in LTP?

best regards,
Daniel

Re: [patch 0/3] [RFC] kernel/glibc mismatch of "readlink" syscall?

[Michael Kerrisk]

Daniel,

Daniel Gollub wrote:
> On Friday 31 October 2008 16:02:48 Kai Henningsen wrote:
>> Am Fri, 24 Oct 2008 17:53:25 -0500
>>
>> schrieb "Michael Kerrisk" <mtk.manpages@googlemail.com>:
>>> Hi Daniel,
>>>
>>> On Thu, Oct 23, 2008 at 9:50 AM, Daniel Gollub <dgollub@suse.de>
>>>
>>> wrote:
>>>> EINVAL bufsiz is not positive.
>>> The EINVAL error was added to man-pages-1.18 in 1997 (even though, as
>>> you note, the type was "size_t").  I suspect (this was well before I
>>> had any association with man-pages) that was done to reflect kernel
>>> reality (since one could bypass glibc invoke the syscall directly),
>>> but obviously it is inconsistent with the prototype.
>> Actually, it's not inconsistent as described, though perhaps that is
>> unintentional. "Not positive" isn't the same as "negative", as zero
>> isn't positive either, and zero is certainly a possible value of an
>> unsigned type
> 
> True.

So, at this stage I don't plan to make any change to man-pages.
(Let me know if you think this is the wrong course.)

> But there is still the problem for the ltp syscall test "readlink03", when 
> using the glibc "readlink" interface, by calling readlink with a buffer size 
> of "-1".
> 
> Calling "-1" seems to be a valid code/error-path in the linux syscall 
> "readlink", since there is a check for less-equal zero.
> 
> But the less zero, condition can't be reached via the glibc "readlink" 
> interface since this would cause fortify-check to fail (when buliding with -
> D_FORITFY_SOURCE=2).
> 
> To "workaround" the fortify check, by not compiling the testcase with -
> D_FORTIFY_SOURCE=2, or trying to test the linux readlink interface by calling 
> directly syscall() in the testcase ... both suggestion are just workarounds - 
> no real solutions.
> 
> We could also just remove the testcase of buffer size "-1".
> 
> The problem is still, how to test the "readlink" syscall in LTP?

I'd say: remove this test.  And add one for bufsiz==0 if there isn't one
already.

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html