Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In .c, open() with O_CREAT (or O_TMPFILE) requires the 3rd arg: mode #2512

Closed
ghost opened this issue May 17, 2019 · 7 comments
Closed

In .c, open() with O_CREAT (or O_TMPFILE) requires the 3rd arg: mode #2512

ghost opened this issue May 17, 2019 · 7 comments

Comments

@ghost
Copy link

ghost commented May 17, 2019

Could Ale maybe warn or err, if it detects any use of open() (ie. man 2 open) with O_CREAT or O_TMPFILE flags (ie. second argument) which doesn't also have a third argument mode_t mode which is required when using those flags.

bad:
int fd=open("/path/somenewfile.log", O_WRONLY | O_CREAT | O_TRUNC | O_APPEND);

good:
int fd=open("/path/somenewfile.log", O_WRONLY | O_CREAT | O_TRUNC | O_APPEND, S_IWUSR | S_IRUSR | S_IRGRP | S_IROTH);

The bad version won't show any errors or warnings, but you will get random mode flags set on that file after executing that open()!

I got bit by this (here), though it was my first time(iirc) using open(); but even so, it might be useful for others, me thinks.

@w0rp
Copy link
Member

w0rp commented May 17, 2019

ALE doesn't check your code, it runs external tools for checking your code. Maybe you could make this suggestion for one of the C linters that ALE runs.

@w0rp w0rp closed this as completed May 17, 2019
@ghost
Copy link
Author

ghost commented May 18, 2019

Great info, thank you!
Turns out that for gcc (not clang also, unfortunately, due to it acting as gcc v4.2 instead of the required(in macros) v4.3), removing -fsyntax-only and adding -D_FORTIFY_SOURCE=2 -O1 is enough to accomplish this!

sample output:

$ gcc -S -x c -iquote /home/user/sandbox/c/open_file_permission_ -std=c11 -Wall -D_FORTIFY_SOURCE=2 -O1 - < /tmp/c.c
<stdin>: In functionmain’:
<stdin>:20:9: warning: unused variablef’ [-Wunused-variable]
In file included from /usr/include/fcntl.h:328,
                 from <stdin>:5:
In functionopen’,
    inlined frommainat <stdin>:32:10:
/usr/include/bits/fcntl2.h:50:4: error: call to__open_missing_modedeclared with attribute error: open with O_CREAT or O_TMPFILE in second argument needs 3 arguments
    __open_missing_mode ();
    ^~~~~~~~~~~~~~~~~~~~~~


$ clang -S -x c -iquote /home/user/sandbox/c/open_file_permission_ -std=c11 -Wall -D_FORTIFY_SOURCE=2 -O1 - < /tmp/c.c
<stdin>:20:9: warning: unused variable 'f' [-Wunused-variable]
  FILE *f=NULL;
        ^
1 warning generated.

Should I look into preparing a PR? or is -fsyntax-only not a thing that can be dropped? EDIT: Well, without -fsyntax-only there's a file ./-.s generated in current dir, apparently because -S is being ignored but can be mitigated with -o /dev/null (and it doesn't remove /dev/null like sudo grub-mkconfig -o /dev/null does!)

EDIT: However though, the above doesn't look good from within Ale(master), it's just a red marker on #include <fcntl.h> saying Error found in header. See :ALEDetail which when run shows:

In file included from /usr/include/fcntl.h:328,                                                                                 
                 from <stdin>:5:
In functionopen’,
    inlined frommainat <stdin>:32:10:
/usr/include/bits/fcntl2.h:50:4: error: call to__open_missing_modedeclared with attribute error: open with O_CREAT or O_TMPFILE in second argument needs 3 arguments
    __open_missing_mode ();

I don't see how I could change and what to actually get Ale to point me to the specific like 32 inside my main file instead! I should probably change something in autoload/ale/handlers/gcc.vim, but first update to Ale master...
Ok this is too hard(for me), maybe someone else could look into implementing this in Ale...

At least the reason it doesn't work with clang is because file /usr/include/sys/cdefs.h contains:

#if __GNUC_PREREQ (4,3) 
# define __warndecl(name, msg) \
  extern void name (void) __attribute__((__warning__ (msg)))
# define __warnattr(msg) __attribute__((__warning__ (msg))) 
# define __errordecl(name, msg) \                                                                                               
  extern void name (void) __attribute__((__error__ (msg)))
#else        
# define __warndecl(name, msg) extern void name (void)
# define __warnattr(msg)
# define __errordecl(name, msg) extern void name (void)
#endif

So this is how it looks like thus far:

diff --git a/ale_linters/c/gcc.vim b/ale_linters/c/gcc.vim
index d965965d..875ea45d 100644
--- a/ale_linters/c/gcc.vim
+++ b/ale_linters/c/gcc.vim
@@ -2,14 +2,16 @@
 " Description: gcc linter for c files
 
 call ale#Set('c_gcc_executable', 'gcc')
-call ale#Set('c_gcc_options', '-std=c11 -Wall')
+call ale#Set('c_gcc_options', '-std=c11 -Wall -D_FORTIFY_SOURCE=2 -O1')
 
 function! ale_linters#c#gcc#GetCommand(buffer, output) abort
     let l:cflags = ale#c#GetCFlags(a:buffer, a:output)
 
     " -iquote with the directory the file is in makes #include work for
     "  headers in the same directory.
-    return '%e -S -x c -fsyntax-only'
+    " -fsyntax-only
+    " TODO: autoload/ale/handlers/gcc.vim
+    return '%e -S -x c -o /dev/null'
     \   . ' -iquote ' . ale#Escape(fnamemodify(bufname(a:buffer), ':p:h'))
     \   . ale#Pad(l:cflags)
     \   . ale#Pad(ale#Var(a:buffer, 'c_gcc_options')) . ' -'

@w0rp
Copy link
Member

w0rp commented May 19, 2019

Are there any dangers in replacing -fsyntax-only with -o /dev/null? Syntastic runs GCC with -fsyntax-only, but I checked, and flycheck does what you're describing here. https://github.com/flycheck/flycheck/blob/master/flycheck.el#L6893

I'll reopen this, as you might be on to something.

@w0rp w0rp reopened this May 19, 2019
@w0rp
Copy link
Member

w0rp commented May 19, 2019

We might be able to update ALE's error parser for C code to recognise the reference to inline code like so:

In function ‘open’,
    inlined from ‘main’ at <stdin>:32:10:
/usr/include/bits/fcntl2.h:50:4: error: call to ‘__open_missing_mode’ declared with attribute error: open with O_CREAT or O_TMPFILE in second argument needs 3 arguments
    __open_missing_mode ();

And then present that as an error in the list. Maybe we could look for lines like inlined from ‘main’ at <stdin>:32:10: followed by lines like /usr/include/bits/fcntl2.h:50:4: error: ....

@ghost
Copy link
Author

ghost commented May 19, 2019

Are there any dangers in replacing -fsyntax-only with -o /dev/null?

The only danger that I immediately thought of is if gcc would be run as root user, then it could remove(unlink(2)) /dev/null before attempting to -o to it. But I have tested this at the time and it doesn't seem to be the case(so gcc is smart enough to not remove it which is probably the case for any non-script program, imho), however I cannot speak for earlier gcc versions, I was using gcc (GCC) 8.3.0.

I know of only sudo grub-mkconfig -o /dev/null that would remove /dev/null unless patched, it does it via mv -f ${grub_cfg}.new ${grub_cfg} (grub-mkconfig (GRUB) 2.02)

I'm actually patching it on the fly every time I run a script update-grub(as root), but here's the patch only:

prevents removal of /dev/null if used as -o
or creating a ${dir}.new file if arg to -o is a ${dir}

$ colordiff -up5 /tmp/grub-mkconfig `which grub-mkconfig`
--- /tmp/grub-mkconfig	2018-11-23 21:42:03.000000000 +0100
+++ /usr/bin/grub-mkconfig	2019-05-12 00:48:30.239956619 +0200
@@ -100,10 +100,30 @@ do
 	;;
     # Explicitly ignore non-option arguments, for compatibility.
     esac
 done
 
+if test -n "$grub_cfg" -a -e "$grub_cfg" -a 1 -eq 1; then
+  #file already exists
+  if ! test -f "$grub_cfg"; then
+    #but not a regular file, eg. /dev/null  (ie. this will avoid removing /dev/null via a later 'mv')
+    echo -n "Destination file '$grub_cfg' already exists but it's not a regular file!" >&2
+    if test -c "$grub_cfg"; then
+      echo " It's a character device!" >&2
+    elif test -d "$grub_cfg"; then
+      echo " It's a directory! So unpatched 'grub-mkconfig' will put inside it a file named '${grub_cfg##*/}.new'" >&2
+    else
+      echo
+    fi
+    if test "$grub_cfg" == "/dev/null" -o -c "$grub_cfg" -o -b "$grub_cfg"; then
+      echo "Don't use '-o /dev/null' but instead just '>/dev/null' with an optionally appended ' 2>&1'" >&2
+      echo "Otherwise non-patched 'grub-mkconfig' will remove&replace /dev/null (via 'mv') with valid grub.cfg contents so you'll have to remove /dev/null and recreate it via 'mknod -m 666 /dev/null c 1 3' (see 'man 4 null')"
+    fi
+    exit 1
+  fi
+fi
+
 if [ "x$EUID" = "x" ] ; then
   EUID=`id -u`
 fi
 
 if [ "$EUID" != 0 ] ; then

I'm personally using the following locally, as a result of my earlier investigation:

diff --git a/ale_linters/c/gcc.vim b/ale_linters/c/gcc.vim
index d965965d..373d881a 100644
--- a/ale_linters/c/gcc.vim
+++ b/ale_linters/c/gcc.vim
@@ -2,14 +2,17 @@
 " Description: gcc linter for c files
 
 call ale#Set('c_gcc_executable', 'gcc')
-call ale#Set('c_gcc_options', '-std=c11 -Wall')
+call ale#Set('c_gcc_options', '-std=c11 -Wall -D_FORTIFY_SOURCE=2 -O1')
 
 function! ale_linters#c#gcc#GetCommand(buffer, output) abort
     let l:cflags = ale#c#GetCFlags(a:buffer, a:output)
 
     " -iquote with the directory the file is in makes #include work for
     "  headers in the same directory.
-    return '%e -S -x c -fsyntax-only'
+    " -fsyntax-only
+    " TODO: autoload/ale/handlers/gcc.vim
+    " see: https://github.com/w0rp/ale/issues/2512
+    return '%e -S -x c -o /dev/null'
     \   . ' -iquote ' . ale#Escape(fnamemodify(bufname(a:buffer), ':p:h'))
     \   . ale#Pad(l:cflags)
     \   . ale#Pad(ale#Var(a:buffer, 'c_gcc_options')) . ' -'

however I didn't really look into what to do to present an error properly, as you just mentioned. (it seems kinda hard to do, at first sight, maybe unless you already know what's going on)
It would be great if someone could implement that, otherwise it might take me an estimate of two days if I really had to do it myself.(but more importantly, I don't really feel like it xD, otherwise it would've been done already haha)

Looking at that flycheck code you linked, it seems there are many other great gcc options that could be used. I'll definitely add some locally :D Thanks!

EDIT: I'm not entirely sure -D_FORTIFY_SOURCE=2 is the better version to use (as opposed to just -D_FORTIFY_SOURCE=1) due to With _FORTIFY_SOURCE set to 2, some more checking is added, but some conforming programs might fail., source. Personally I'll use the =2, but for general public, I'm unsure yet which I would choose for them (ie. I'd have to look into it more).

@w0rp
Copy link
Member

w0rp commented May 20, 2019

I have updated the linters now to use -o /dev/null or -o nul on Windows so more errors are generated, just like flycheck does.

@w0rp
Copy link
Member

w0rp commented May 20, 2019

I'll open another issue for parsing the inlined code errors. I believe Syntastic handles that, and we should too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant