[PATCH] Fix fopen to return EFAULT when filename is NULL

[Waldemar Kozaczuk]

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---
libc/stdio/fopen.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/libc/stdio/fopen.c b/libc/stdio/fopen.c
index 83452407..c6053c21 100644
--- a/libc/stdio/fopen.c
+++ b/libc/stdio/fopen.c
@@ -10,6 +10,11 @@ FILE *fopen(const char *restrict filename, const char *restrict mode)
int fd;
int flags;

+ if (filename == NULL) {
+ errno = EFAULT;
+ return 0;
+ }
+
/* Check for valid initial mode character */
if (!strchr("rwa", *mode)) {
errno = EINVAL;
--
2.20.1

[Nadav Har'El]

On Sat, May 11, 2019 at 2:40 PM Waldemar Kozaczuk <jwkoz...@gmail.com> wrote:

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---
 libc/stdio/fopen.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/libc/stdio/fopen.c b/libc/stdio/fopen.c
index 83452407..c6053c21 100644
--- a/libc/stdio/fopen.c
+++ b/libc/stdio/fopen.c
@@ -10,6 +10,11 @@ FILE *fopen(const char *restrict filename, const char *restrict mode)
        int fd;
        int flags;

+       if (filename == NULL) {
+               errno = EFAULT;
+               return 0;
+       }

I'm curious why you wanted to do this change, for two reasons:

1. Is there a real program which calls fopen() on a null pointer and expect it to fail with EFAULT instead of crashing?

2. Since fopen() just calls the open() system call, shouldn't we do this change in open()'s implementation?

Because fopen.c really came from Musl, I wouldn't want to do changes in it which aren't necessary.

 

+
        /* Check for valid initial mode character */
        if (!strchr("rwa", *mode)) {
                errno = EINVAL;
--
2.20.1

--
You received this message because you are subscribed to the Google Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/20190511114024.4928-1-jwkozaczuk%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Waldek Kozaczuk]

Please see my other explanation in the thread email related to the patch “Enhance getopt functions to work with PIEs”. There I am talking about how unmodified iperf was failing and it was because of nullptr filename. Based on strace of iperf3 on Linux fopen on Linux seems to return EFAULT. Glibc-ism?

Easy to write independent test program :-)

Sent from my iPhone

[Waldek Kozaczuk]

Or it could be that at this layer - musl - it is not handled but instead at the syscall level in Linux. Either way I think we should fix it somewhere. 

Sent from my iPhone

On May 11, 2019, at 08:06, Nadav Har'El <n...@scylladb.com> wrote:

[Nadav Har'El]

On Sat, May 11, 2019 at 3:15 PM Waldek Kozaczuk <jwkoz...@gmail.com> wrote:

Or it could be that at this layer - musl - it is not handled but instead at the syscall level in Linux. Either way I think we should fix it somewhere. 

Yes. I prefer that we fix it in the level of the open() system call, please.

In Linux, the kernel cannot "crash" when the user gives it bad pointers - it needs to return an error to the caller. So open() returns an EFAULT if its filename parameter is a bad pointer (including a null pointer).

We never did this in OSv and I assumed it was OK - it is an application bug that it passes bad pointers to system calls. But if you found cases it matters, we can easily fix it for the case of a null pointer (for other cases of bad pointers, this a bigger mess to fix, and I wouldn't do it before we find a need).