Fix permissions for files created by sim65. #719

ppelleti · 2018-08-17T06:49:35Z

Files created by my programs running under sim65 had really weird permissions:

--w-r--r-x  1 ppelleti  staff  19 Aug 16 23:22 json.test.print.tmp

So, for example, my program running under sim65 was not able to read the file it had just written.

This is because the third argument to open (mode) was not being specified in paravirt.c, so it was just picking up random crud off the stack to use as the mode.

I added a mode of 0666, and now my program runs correctly.

Files created by my programs running under sim65 had really weird permissions: --w-r--r-x 1 ppelleti staff 19 Aug 16 23:22 json.test.print.tmp So, for example, my program running under sim65 was not able to read the file it had just written. This is because the third argument to open (mode) was not being specified in paravirt.c, so it was just picking up random crud off the stack to use as the mode. I added a mode of 0666, and now my program runs correctly.

oliverschmidt · 2018-08-17T09:21:55Z

Thanks for the error report combined with a fix proposal :-)

However, I see an issue with your fix: Looking e.g. at http://man7.org/linux/man-pages/man2/open.2.html it doesn't seem advisable to just cast "some" numeric constant to mode_t.

Additionally the cc65 code base is so far compatible with Visual C++. Their doc says: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/open-wopen

So it seems more appropriate to include sys/stat.h and use S_IREAD | S_IWRITE.

But wouldn't it after all be most appropriate to add a sys/stat.h to cc65 defining S_IREAD and S_IWRITE and then have code that checks Mode in the same way Flags is checked? Meaning that the cc65 program controls the mode (restricted to S_IREAD and S_IWRITE).

Implements this suggestion: cc65#719 (comment)

ppelleti · 2018-08-17T18:00:41Z

Thanks for the error report combined with a fix proposal :-)

Is it preferable to file a bug first, and then a pull request?

But wouldn't it after all be most appropriate to add a sys/stat.h to cc65 defining S_IREAD and S_IWRITE and then have code that checks Mode in the same way Flags is checked? Meaning that the cc65 program controls the mode (restricted to S_IREAD and S_IWRITE).

I've gone ahead and implemented this suggestion, and modified fopen to pass S_IREAD | S_IWRITE to open.

However, it's probably a common mistake to omit the mode, so in paravirt.c I also implemented a default: if the mode argument is not present, it defaults to S_IREAD | S_IWRITE.

Since S_IREAD and S_IWRITE are aliases for S_IRUSR and S_IWUSR, one consequence of this approach is that the files created will only be readable and/or writable by user, rather than obeying the user's umask. However, this probably isn't a big deal.

ppelleti · 2018-08-17T18:03:38Z

Hmm.. this code built fine on OS X, but the Travis build (presumably under Linux?) fails with:

sim65/paravirt.c: In function ‘PVOpen’:
sim65/paravirt.c:217:18: error: ‘S_IREAD’ undeclared (first use in this function)
         OMode |= S_IREAD;
                  ^
sim65/paravirt.c:217:18: note: each undeclared identifier is reported only once for each function it appears in
sim65/paravirt.c:220:18: error: ‘S_IWRITE’ undeclared (first use in this function)
         OMode |= S_IWRITE;
                  ^
make[1]: *** [../wrk/sim65/paravirt.o] Error 1

The Linux build was failing with: sim65/paravirt.c: In function ‘PVOpen’: sim65/paravirt.c:217:18: error: ‘S_IREAD’ undeclared (first use in this function) OMode |= S_IREAD; ^ sim65/paravirt.c:217:18: note: each undeclared identifier is reported only once for each function it appears in sim65/paravirt.c:220:18: error: ‘S_IWRITE’ undeclared (first use in this function) OMode |= S_IWRITE; ^ make[1]: *** [../wrk/sim65/paravirt.o] Error 1

oliverschmidt · 2018-08-17T18:31:14Z

Is it preferable to file a bug first, and then a pull request?

Nope, not at all!

I've gone ahead and implemented this suggestion, and modified fopen to pass S_IREAD | S_IWRITE to open.

:-)

However, it's probably a common mistake to omit the mode, so in paravirt.c I also implemented a default: if the mode argument is not present, it defaults to S_IREAD | S_IWRITE.

I see and agree.

Since S_IREAD and S_IWRITE are aliases for S_IRUSR and S_IWUSR, one consequence of this approach is that the files created will only be readable and/or writable by user, rather than obeying the user's umask. However, this probably isn't a big deal.

That's exactly what I thought when I made my proposal.

Microsoft loves putting underscores in front of everything!

ppelleti · 2018-08-17T19:08:23Z

I got the Linux build passing, but the Windows build is still failing:

sim65/paravirt.c: In function ‘PVOpen’:
sim65/paravirt.c:53:22: error: ‘_S_IREAD’ undeclared (first use in this function)
 #    define S_IREAD  _S_IREAD
                      ^
sim65/paravirt.c:231:18: note: in expansion of macro ‘S_IREAD’
         OMode |= S_IREAD;
                  ^
sim65/paravirt.c:53:22: note: each undeclared identifier is reported only once for each function it appears in
 #    define S_IREAD  _S_IREAD
                      ^
sim65/paravirt.c:231:18: note: in expansion of macro ‘S_IREAD’
         OMode |= S_IREAD;
                  ^
sim65/paravirt.c:60:22: error: ‘_S_IWRITE’ undeclared (first use in this function)
 #    define S_IWRITE _S_IWRITE
                      ^
sim65/paravirt.c:234:18: note: in expansion of macro ‘S_IWRITE’
         OMode |= S_IWRITE;
                  ^
make[1]: *** [../wrk/sim65/paravirt.o] Error 1
make: *** [bin] Error 2
The command "make bin USER_CFLAGS=-Werror CROSS_COMPILE=i686-w64-mingw32-" exited with 2.

Since _S_IREAD and _S_IWRITE are specified in the Windows documentation you referenced, I'm not sure why they're undefined. Any ideas?

oliverschmidt · 2018-08-17T18:35:12Z

src/sim65/paravirt.c

@@ -169,12 +175,19 @@ static void PVOpen (CPURegs* Regs)
 {
    char Path[1024];
    int OFlag = O_INITIAL;
-    unsigned RetVal, I = 0;
+    unsigned RetVal, I = 0, OMode = 0;


I'd like to see OMode being defined as a mode_t.

oliverschmidt · 2018-08-17T18:36:59Z

src/sim65/paravirt.c

+        /* If the caller did not supply the mode argument,
+        ** use a reasonable default.
+        */
+        Mode = 0400 | 0200;


I'd like to see this use the constants S_IREAD | S_IWRITE.

oliverschmidt · 2018-08-17T18:39:37Z

libsrc/common/_fopen.s

@@ -15,6 +15,7 @@
        .include        "errno.inc"
        .include        "fcntl.inc"
        .include        "_file.inc"
+        .include        "stat.inc"


I think we should leave _fopen.s alone and rely on the default handling you provided in paravirt.c. It's not desirable to make every cc65 program using stdio larger "for nothing".

oliverschmidt · 2018-08-17T19:44:51Z

include/sys/stat.h

+** Must match the values in asminc/stat.inc and src/sim65/paravirt.c
+*/
+
+#define S_IREAD  0400


I'm thinking about these values...

Their type: cc65 doesn't define a mode_t. But it says int __fastcall__ creat (const char* name, unsigned mode); so mode is "known" to be unsigned.

Their values: I don't think it's beneficial to use UNIX values here. It sort of implies that things work like on UNIX - which of course is wrong. If you look at

#define O_RDONLY 0x01 #define O_WRONLY 0x02 #define O_RDWR 0x03 #define O_CREAT 0x10 #define O_TRUNC 0x20 #define O_APPEND 0x40 #define O_EXCL 0x80

then these are non-UNIX values too. And finally: If there should be one day an 8-bit target actually supporting the mode it would be beneficial to only use 8-bit values allowing the 6502 open/creat code to ignore the high byte - just like with the flags above which are only 8-bit values although the data type is int.
So I would just go for

#define S_IREAD 0x01 #define S_WRITE 0x02

oliverschmidt · 2018-08-17T19:57:28Z

Since _S_IREAD and _S_IWRITE are specified in the Windows documentation you referenced, I'm not sure why they're undefined. Any ideas?

Neither with Visual C++ nor with MinGW there should be a reason to use the _... variants. I see

#define S_IREAD _S_IREAD
#define S_IWRITE _S_IWRITE

in all headers I can get hold of.

ppelleti · 2018-08-17T20:14:03Z

Neither with Visual C++ nor with MinGW there should be a reason to use the _... variants.

I only tried that because the Windows build was failing because S_IREAD and S_IWRITE were undefined. (And if they were defined, it wouldn't be taking the ifndef S_IREAD case anyway.)

So I'm still not clear why neither S_IREAD nor _S_IREAD is being defined. I tried including sys/types.h and sys/stat.h, since the Windows documentation for _open listed them as "optional headers", but that didn't help any.

It isn't necessary, since paravirt.c has a default if the mode isn't pushed onto the stack.

(As requested in the PR.)

ppelleti · 2018-08-17T20:53:59Z

I'd like to see OMode being defined as a mode_t.

Good idea. Done!

I'd like to see this use the constants S_IREAD | S_IWRITE.

These are the target's S_IREAD and S_IWRITE, which are (now) different than the host's. Currently, I don't believe sim65 includes any 6502 headers. (And if it did, they would conflict with the host's headers.) Any suggestions for how to best handle this?

I think we should leave _fopen.s alone and rely on the default handling you provided in paravirt.c. It's not desirable to make every cc65 program using stdio larger "for nothing".

Done!

I don't think it's beneficial to use UNIX values here. It sort of implies that things work like on UNIX - which of course is wrong.

OK. I slightly disagree, since open is inherently a POSIX function. And unlike the O_RDONLY etc. constants, which are always specified symbolically, it's fairly common to specify the mode as an octal number.

However, I see your point about making 8-bit implementations easier, and I've gone ahead and done as you've requested.

And oddly, Travis is passing now, although I don't see how any of the changes I made would have affected the issue with S_IREAD and S_IWRITE not being defined on Windows.

ppelleti · 2018-08-17T20:56:53Z

By the way, any idea why clicking on the "See review" button doesn't do anything? I've never used that feature of GitHub before, so I'm not sure how it's supposed to work. (Luckily, GitHub sent me your comments in an email, so I'm just going off of that.)

ppelleti · 2018-08-17T21:21:18Z

By the way, any idea why clicking on the "See review" button doesn't do anything?

Looks like Privacy Badger was causing this problem. I disabled Privacy Badger and was able to click "See Review" successfully.

oliverschmidt · 2018-08-17T21:27:20Z

These are the target's S_IREAD and S_IWRITE, which are (now) different than the host's. Currently, I don't believe sim65 includes any 6502 headers. (And if it did, they would conflict with the host's headers.) Any suggestions for how to best handle this?

You are of course right. This is about manipulating Mode, not OMode. For Mode we use the right magic numbers just like we do for Flags.

Implements this suggestion: #719 (comment)

ppelleti added 2 commits August 17, 2018 10:43

Allow "mode" argument to open() to be passed from 6502 code.

16fff8f

Implements this suggestion: cc65#719 (comment)

sim65: If mode argument is omitted, use a reasonable default.

38f3c36

sim65: Build was failing on Windows, too.

01838db

Microsoft loves putting underscores in front of everything!

sim65: Blindly guessing to get Travis build to pass.

21d7e71

oliverschmidt requested changes Aug 17, 2018

View reviewed changes

ppelleti added 3 commits August 17, 2018 13:17

sim65: Use mode_t for OMode.

8bd933b

Don't pass mode argument to open() from _fopen().

d35ae11

It isn't necessary, since paravirt.c has a default if the mode isn't pushed onto the stack.

Use non-POSIX values for S_IREAD and S_IWRITE.

d615b23

(As requested in the PR.)

oliverschmidt approved these changes Aug 17, 2018

View reviewed changes

oliverschmidt merged commit e549e23 into cc65:master Aug 17, 2018

oliverschmidt pushed a commit that referenced this pull request Aug 17, 2018

Allow "mode" argument to open() to be passed from 6502 code.

aba320e

Implements this suggestion: #719 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix permissions for files created by sim65. #719

Fix permissions for files created by sim65. #719

ppelleti commented Aug 17, 2018

oliverschmidt commented Aug 17, 2018

ppelleti commented Aug 17, 2018

ppelleti commented Aug 17, 2018

oliverschmidt commented Aug 17, 2018

ppelleti commented Aug 17, 2018

oliverschmidt Aug 17, 2018

oliverschmidt Aug 17, 2018

oliverschmidt Aug 17, 2018

oliverschmidt Aug 17, 2018

oliverschmidt commented Aug 17, 2018

ppelleti commented Aug 17, 2018

ppelleti commented Aug 17, 2018

ppelleti commented Aug 17, 2018

ppelleti commented Aug 17, 2018

oliverschmidt commented Aug 17, 2018 •

edited by greg-king5

Fix permissions for files created by sim65. #719

Fix permissions for files created by sim65. #719

Conversation

ppelleti commented Aug 17, 2018

oliverschmidt commented Aug 17, 2018

ppelleti commented Aug 17, 2018

ppelleti commented Aug 17, 2018

oliverschmidt commented Aug 17, 2018

ppelleti commented Aug 17, 2018

oliverschmidt Aug 17, 2018

Choose a reason for hiding this comment

oliverschmidt Aug 17, 2018

Choose a reason for hiding this comment

oliverschmidt Aug 17, 2018

Choose a reason for hiding this comment

oliverschmidt Aug 17, 2018

Choose a reason for hiding this comment

oliverschmidt commented Aug 17, 2018

ppelleti commented Aug 17, 2018

ppelleti commented Aug 17, 2018

ppelleti commented Aug 17, 2018

ppelleti commented Aug 17, 2018

oliverschmidt commented Aug 17, 2018 • edited by greg-king5

oliverschmidt commented Aug 17, 2018 •

edited by greg-king5