Re: [PATCH] Kconfig: add warning about permission of config file

Re: [PATCH] Kconfig: add warning about permission of config file

[Hiromu Yakura]

Hello,
thanks for replying.

On Mon, May 23, 2011 at 13:58, Arnaud Lacombe <lacombar@gmail.com> wrote:
> Do you have a precise way to reproduce this, in particular which
> target is involved ? I tried to `chmod 555' the kernel root directory,
> re-ran `conf' (through the `defconfig' target) and `mconf' (manually
> for this one, as check-lxdialog.sh fails when invoked though make).
> The former failed with:
> 
> *** Error during writing of the configuration.
> 
> gmake[1]: *** [defconfig] Error 1
> gmake: *** [defconfig] Error 2
In my environment, this patch is working properly.
The output is as follows:

hiromu@hiromu-MacBook:/usr/src/linux-2.6$ ls -ld .
dr-xr-sr-x 25 hiromu hiromu 4096 May 24 21:41 .
hiromu@hiromu-MacBook:/usr/src/linux-2.6$ make defconfig
*** Default configuration is based on 'x86_64_defconfig'
*** Permission denied to write the configuration.

make[1]: *** [defconfig] Error 1
make: *** [defconfig] Error 2
hiromu@hiromu-MacBook:/usr/src/linux-2.6$  

> So check _are_ being done whether or not the configuration can be
> written, but there might be a corner-case not checked, in that case,
> this specific path should be fixed.
I think it is OK if you have the write permission to the directory
which be saved the configuration.

> > Cc: Roman Zippel <zippel@linux-m68k.org>
> > Cc: Michal Marek <mmarek@suse.cz>
> > Cc: Arnaud Lacombe <lacombar@gmail.com>
> >
> > Signed-off-by: Hiromu Yakura <hiromu1996@gmail.com>
> > ---
> >  scripts/kconfig/conf.c     |    6 ++++++
> >  scripts/kconfig/confdata.c |   17 +++++++++++++++++
> >  scripts/kconfig/gconf.c    |    4 ++++
> >  scripts/kconfig/lkc.h      |    1 +
> >  scripts/kconfig/mconf.c    |    4 ++++
> >  scripts/kconfig/nconf.c    |    4 ++++
> >  scripts/kconfig/qconf.cc   |    4 ++++
> >  7 files changed, 40 insertions(+), 0 deletions(-)
> >
> > diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> > index 006ad81..d93e351 100644
> > --- a/scripts/kconfig/conf.c
> > +++ b/scripts/kconfig/conf.c
> > @@ -466,6 +466,12 @@ int main(int ac, char **av)
> >        bindtextdomain(PACKAGE, LOCALEDIR);
> >        textdomain(PACKAGE);
> >
> > +       if (conf_check_permission()) {
> > +               fprintf(stderr,
> > +                       "*** Permission denied to write the configuration.\n\n");
> > +               exit(1);
> > +       }
> > +
> >        while ((opt = getopt_long(ac, av, "", long_opts, NULL)) != -1) {
> >                input_mode = (enum input_mode)opt;
> >                switch (opt) {
> > diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> > index 61c35bf..3de1fbe 100644
> > --- a/scripts/kconfig/confdata.c
> > +++ b/scripts/kconfig/confdata.c
> > @@ -7,6 +7,7 @@
> >  #include <ctype.h>
> >  #include <errno.h>
> >  #include <fcntl.h>
> > +#include <libgen.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > @@ -1051,3 +1052,19 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
> >                        set_all_choice_values(csym);
> >        }
> >  }
> > +
> > +int conf_check_permission(void)
> > +{
> > +       int ret, retval = 0;
> > +       const char *name;
> > +       char *dir;
> > +
> > +       name = conf_get_configname();
> > +       dir = dirname((char *)name);
> > +
> > +       ret = access(dir, W_OK);
> > +       if (ret < 0)
> > +               retval = -errno;
> > +
> > +       return retval;
> > +}
> > diff --git a/scripts/kconfig/gconf.c b/scripts/kconfig/gconf.c
> > index 4558961..3567a23 100644
> > --- a/scripts/kconfig/gconf.c
> > +++ b/scripts/kconfig/gconf.c
> > @@ -1510,6 +1510,10 @@ int main(int ac, char *av[])
> >        bind_textdomain_codeset(PACKAGE, "UTF-8");
> >        textdomain(PACKAGE);
> >
> > +       if (conf_check_permission())
> > +               fprintf(stderr,
> > +                       "Warning: Permission denied to write the configuration.\n");
> > +
> >        /* GTK stuffs */
> >        gtk_set_locale();
> >        gtk_init(&ac, &av);
> > diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
> > index febf0c9..4d20841 100644
> > --- a/scripts/kconfig/lkc.h
> > +++ b/scripts/kconfig/lkc.h
> > @@ -91,6 +91,7 @@ char *conf_get_default_confname(void);
> >  void sym_set_change_count(int count);
> >  void sym_add_change_count(int count);
> >  void conf_set_all_new_symbols(enum conf_def_mode mode);
> > +int conf_check_permission(void);
> >
> >  /* confdata.c and expr.c */
> >  static inline void xfwrite(const void *str, size_t len, size_t count, FILE *out)
> > diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
> > index d433c7a..c820e05 100644
> > --- a/scripts/kconfig/mconf.c
> > +++ b/scripts/kconfig/mconf.c
> > @@ -803,6 +803,10 @@ int main(int ac, char **av)
> >        bindtextdomain(PACKAGE, LOCALEDIR);
> >        textdomain(PACKAGE);
> >
> > +       if (conf_check_permission())
> > +               fprintf(stderr,
> > +                       "Warning: Permission denied to write the configuration.\n");
> > +
> >        conf_parse(av[1]);
> >        conf_read(NULL);
> >
> > diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c
> > index db56377..1cea031 100644
> > --- a/scripts/kconfig/nconf.c
> > +++ b/scripts/kconfig/nconf.c
> > @@ -1491,6 +1491,10 @@ int main(int ac, char **av)
> >        bindtextdomain(PACKAGE, LOCALEDIR);
> >        textdomain(PACKAGE);
> >
> > +       if (conf_check_permission())
> > +               fprintf(stderr,
> > +                       "Warning: Permission denied to write the configuration.\n");
> > +
> >        conf_parse(av[1]);
> >        conf_read(NULL);
> >
> > diff --git a/scripts/kconfig/qconf.cc b/scripts/kconfig/qconf.cc
> > index 06dd2e3..7dca7ac 100644
> > --- a/scripts/kconfig/qconf.cc
> > +++ b/scripts/kconfig/qconf.cc
> > @@ -1746,6 +1746,10 @@ int main(int ac, char** av)
> >        bindtextdomain(PACKAGE, LOCALEDIR);
> >        textdomain(PACKAGE);
> >
> > +       if (conf_check_permission())
> > +               fprintf(stderr,
> > +                       "Warning: Permission denied to write the configuration.\n");
> > +
> >  #ifndef LKC_DIRECT_LINK
> >        kconfig_load();
> >  #endif
> > --
> > 1.7.4.1
> >
> >
> >
> >

Re: [PATCH] Kconfig: add warning about permission of config file

[Michal Marek]

On 24.5.2011 15:06, Hiromu Yakura wrote:
> Hello,
> thanks for replying.
>
> On Mon, May 23, 2011 at 13:58, Arnaud Lacombe<lacombar@gmail.com>  wrote:
>> Do you have a precise way to reproduce this, in particular which
>> target is involved ? I tried to `chmod 555' the kernel root directory,
>> re-ran `conf' (through the `defconfig' target) and `mconf' (manually
>> for this one, as check-lxdialog.sh fails when invoked though make).
>> The former failed with:
>>
>> *** Error during writing of the configuration.
>>
>> gmake[1]: *** [defconfig] Error 1
>> gmake: *** [defconfig] Error 2
> In my environment, this patch is working properly.
> The output is as follows:
>
> hiromu@hiromu-MacBook:/usr/src/linux-2.6$ ls -ld .
> dr-xr-sr-x 25 hiromu hiromu 4096 May 24 21:41 .
> hiromu@hiromu-MacBook:/usr/src/linux-2.6$ make defconfig
> *** Default configuration is based on 'x86_64_defconfig'
> *** Permission denied to write the configuration.

Arnaud's point is that your patch should not be necessary at all, 
because kconfig already checks the return value of the fopen() call in 
conf_write() and prints the above message if it fails. So do you have a 
testcase where make <...>config without your patch returns without 
error, but the configuration is not written?

Michal

Re: [PATCH] Kconfig: add warning about permission of config file

[Hiromu Yakura]

Hello,

On Fri, May 24, 2011 at 22:23, Michal Marek <mmarek@suse.cz> wrote:
> Arnaud's point is that your patch should not be necessary at all, 
> because kconfig already checks the return value of the fopen() call in 
> conf_write() and prints the above message if it fails. So do you have a 
> testcase where make <...>config without your patch returns without 
> error, but the configuration is not written?
Sorry for misunderstanding.
Indeed, make *config which use 'conf' (e.g. oldconfig, defconfig...)
raise error.
And only xconfig and gconfig don't write the configuration without
error.

However, make *config using 'conf' hasn't function of 'Save as'.
In other words, if you don't set the write permission to the directory,
you would lose changes of the configuration.
I think that it is necessary to check permission at all so that kconfig
warn of the possibility of losing changes.

Thanks

Re: [PATCH] Kconfig: add warning about permission of config file

[Michal Marek]

On 24.5.2011 16:26, Hiromu Yakura wrote:
> Hello,
>
> On Fri, May 24, 2011 at 22:23, Michal Marek<mmarek@suse.cz>  wrote:
>> Arnaud's point is that your patch should not be necessary at all,
>> because kconfig already checks the return value of the fopen() call in
>> conf_write() and prints the above message if it fails. So do you have a
>> testcase where make<...>config without your patch returns without
>> error, but the configuration is not written?
> Sorry for misunderstanding.
> Indeed, make *config which use 'conf' (e.g. oldconfig, defconfig...)
> raise error.
> And only xconfig and gconfig don't write the configuration without
> error.

I see, qconf lacks a check for the return value of conf_write() in 
ConfigMainWindow::closeEvent(), gconf does check the return value, but 
only displays it in the bottom box of the main window instead of a 
message box. Neither of them return failure in the error case. These 
bugs should be indeed fixed. But I don't like the directory permission 
check, it only handles one case, but does not handle permission on the 
.config file itself with KCONFIG_OVERWRITECONFIG=1, ENOSPC and so on.

Michal

Re: [PATCH] Kconfig: add warning about permission of config file

[Arnaud Lacombe]

Hi,

On Tue, May 24, 2011 at 11:01 AM, Michal Marek <mmarek@suse.cz> wrote:
> On 24.5.2011 16:26, Hiromu Yakura wrote:
>>
>> Hello,
>>
>> On Fri, May 24, 2011 at 22:23, Michal Marek<mmarek@suse.cz>  wrote:
>>>
>>> Arnaud's point is that your patch should not be necessary at all,
>>> because kconfig already checks the return value of the fopen() call in
>>> conf_write() and prints the above message if it fails. So do you have a
>>> testcase where make<...>config without your patch returns without
>>> error, but the configuration is not written?
>>
>> Sorry for misunderstanding.
>> Indeed, make *config which use 'conf' (e.g. oldconfig, defconfig...)
>> raise error.
>> And only xconfig and gconfig don't write the configuration without
>> error.
>
> I see, qconf lacks a check for the return value of conf_write() in
> ConfigMainWindow::closeEvent(), gconf does check the return value, but only
> displays it in the bottom box of the main window instead of a message box.
> Neither of them return failure in the error case. These bugs should be
> indeed fixed.
>
agree.

> But I don't like the directory permission check, it only
> handles one case, but does not handle permission on the .config file itself
> with KCONFIG_OVERWRITECONFIG=1, ENOSPC and so on.
>
seconded.

 - Arnaud

Re: [PATCH] Kconfig: add warning about permission of config file

[Sam Ravnborg]

> >>>
> >>> Arnaud's point is that your patch should not be necessary at all,
> >>> because kconfig already checks the return value of the fopen() call in
> >>> conf_write() and prints the above message if it fails. So do you have a
> >>> testcase where make<...>config without your patch returns without
> >>> error, but the configuration is not written?
> >>
> >> Sorry for misunderstanding.
> >> Indeed, make *config which use 'conf' (e.g. oldconfig, defconfig...)
> >> raise error.
> >> And only xconfig and gconfig don't write the configuration without
> >> error.
> >
> > I see, qconf lacks a check for the return value of conf_write() in
> > ConfigMainWindow::closeEvent(), gconf does check the return value, but only
> > displays it in the bottom box of the main window instead of a message box.
> > Neither of them return failure in the error case. These bugs should be
> > indeed fixed.
> >
> agree.
The only thing that a write-access check up-front could be useful for would be
to launch a front-end in ReadOnly mode.
And this is not supported by any of them today IIRC.

	Sam

Re: [PATCH] Kconfig: add warning about permission of config file

[Hiromu Yakura]

On Tue, May 24, 2011, at 0:50, Arnaud Lacombe <lacombar@gmail.com>
wrote:
> On Tue, May 24, 2011 at 11:01 AM, Michal Marek <mmarek@suse.cz> wrote:
> > I see, qconf lacks a check for the return value of conf_write() in
> > ConfigMainWindow::closeEvent(), gconf does check the return value, but only
> > displays it in the bottom box of the main window instead of a message box.
> > Neither of them return failure in the error case. These bugs should be
> > indeed fixed.
> >
> agree.
> 
> > But I don't like the directory permission check, it only
> > handles one case, but does not handle permission on the .config file itself
> > with KCONFIG_OVERWRITECONFIG=1, ENOSPC and so on.
> >
> seconded.
I'm sorry for forgetting to handle a case which was set KCONFIG_OVERWRITECONFIG.
So I rewrote the patch and attach it.

Thanks for your advice.

Signed-off-by: Hiromu Yakura <hiromu1996@gmail.com>
---
 scripts/kconfig/conf.c     |    6 ++++++
 scripts/kconfig/confdata.c |   24 ++++++++++++++++++++++++
 scripts/kconfig/gconf.c    |    4 ++++
 scripts/kconfig/lkc.h      |    1 +
 scripts/kconfig/mconf.c    |    4 ++++
 scripts/kconfig/nconf.c    |    4 ++++
 scripts/kconfig/qconf.cc   |    4 ++++
 7 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 006ad81..d93e351 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -466,6 +466,12 @@ int main(int ac, char **av)
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE);
 
+	if (conf_check_permission()) {
+		fprintf(stderr,
+			"*** Permission denied to write the configuration.\n\n");
+		exit(1);
+	}
+
 	while ((opt = getopt_long(ac, av, "", long_opts, NULL)) != -1) {
 		input_mode = (enum input_mode)opt;
 		switch (opt) {
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 61c35bf..2070ac0 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -7,6 +7,7 @@
 #include <ctype.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <libgen.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -1051,3 +1052,26 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
 			set_all_choice_values(csym);
 	}
 }
+
+int conf_check_permission(void)
+{
+	int ret, retval = 0;
+	const char *name;
+	char *dir, *env;
+
+	name = conf_get_configname();
+
+	env = getenv("KCONFIG_OVERWRITECONFIG");
+	if (!env || !*env) {
+		dir = dirname((char *)name);
+		ret = access(dir, W_OK);
+		if (ret < 0)
+			retval = -errno;
+	} else {
+		ret = access(name, W_OK);
+		if (ret < 0)
+			retval = -errno;
+	}
+
+	return retval;
+}
diff --git a/scripts/kconfig/gconf.c b/scripts/kconfig/gconf.c
index 4558961..3567a23 100644
--- a/scripts/kconfig/gconf.c
+++ b/scripts/kconfig/gconf.c
@@ -1510,6 +1510,10 @@ int main(int ac, char *av[])
 	bind_textdomain_codeset(PACKAGE, "UTF-8");
 	textdomain(PACKAGE);
 
+	if (conf_check_permission())
+		fprintf(stderr,
+			"Warning: Permission denied to write the configuration.\n");
+
 	/* GTK stuffs */
 	gtk_set_locale();
 	gtk_init(&ac, &av);
diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
index febf0c9..4d20841 100644
--- a/scripts/kconfig/lkc.h
+++ b/scripts/kconfig/lkc.h
@@ -91,6 +91,7 @@ char *conf_get_default_confname(void);
 void sym_set_change_count(int count);
 void sym_add_change_count(int count);
 void conf_set_all_new_symbols(enum conf_def_mode mode);
+int conf_check_permission(void);
 
 /* confdata.c and expr.c */
 static inline void xfwrite(const void *str, size_t len, size_t count, FILE *out)
diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
index d433c7a..c820e05 100644
--- a/scripts/kconfig/mconf.c
+++ b/scripts/kconfig/mconf.c
@@ -803,6 +803,10 @@ int main(int ac, char **av)
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE);
 
+	if (conf_check_permission())
+		fprintf(stderr,
+			"Warning: Permission denied to write the configuration.\n");
+
 	conf_parse(av[1]);
 	conf_read(NULL);
 
diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c
index db56377..1cea031 100644
--- a/scripts/kconfig/nconf.c
+++ b/scripts/kconfig/nconf.c
@@ -1491,6 +1491,10 @@ int main(int ac, char **av)
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE);
 
+	if (conf_check_permission())
+		fprintf(stderr,
+			"Warning: Permission denied to write the configuration.\n");
+
 	conf_parse(av[1]);
 	conf_read(NULL);
 
diff --git a/scripts/kconfig/qconf.cc b/scripts/kconfig/qconf.cc
index 06dd2e3..7dca7ac 100644
--- a/scripts/kconfig/qconf.cc
+++ b/scripts/kconfig/qconf.cc
@@ -1746,6 +1746,10 @@ int main(int ac, char** av)
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE);
 
+	if (conf_check_permission())
+		fprintf(stderr,
+			"Warning: Permission denied to write the configuration.\n");
+
 #ifndef LKC_DIRECT_LINK
 	kconfig_load();
 #endif
-- 
1.7.4.1

Re: [PATCH] Kconfig: add warning about permission of config file

[Arnaud Lacombe]

Hi,

On Tue, May 24, 2011 at 1:38 PM, Hiromu Yakura <hiromu1996@gmail.com> wrote:
> On Tue, May 24, 2011, at 0:50, Arnaud Lacombe <lacombar@gmail.com>
> wrote:
>> On Tue, May 24, 2011 at 11:01 AM, Michal Marek <mmarek@suse.cz> wrote:
>> > I see, qconf lacks a check for the return value of conf_write() in
>> > ConfigMainWindow::closeEvent(), gconf does check the return value, but only
>> > displays it in the bottom box of the main window instead of a message box.
>> > Neither of them return failure in the error case. These bugs should be
>> > indeed fixed.
>> >
>> agree.
>>
>> > But I don't like the directory permission check, it only
>> > handles one case, but does not handle permission on the .config file itself
>> > with KCONFIG_OVERWRITECONFIG=1, ENOSPC and so on.
>> >
>> seconded.
> I'm sorry for forgetting to handle a case which was set KCONFIG_OVERWRITECONFIG.
> So I rewrote the patch and attach it.
>
> Thanks for your advice.
>
> Signed-off-by: Hiromu Yakura <hiromu1996@gmail.com>
>
Let me re-state: your patch does not handle all the case where
conf_write() may fails, and I do not think we want to preemptively
check for all errors open(2) may return.

 - Arnaud

Re: [PATCH] Kconfig: add warning about permission of config file

[Hiromu Yakura]

On Tue, May 24, 2011, at 2:59, Arnaud Lacombe <lacombar@gmail.com>
wrote:
> Hi,
> 
> On Tue, May 24, 2011 at 1:38 PM, Hiromu Yakura <hiromu1996@gmail.com> wrote:
> > On Tue, May 24, 2011, at 0:50, Arnaud Lacombe <lacombar@gmail.com>
> > wrote:
> >> On Tue, May 24, 2011 at 11:01 AM, Michal Marek <mmarek@suse.cz> wrote:
> >> > I see, qconf lacks a check for the return value of conf_write() in
> >> > ConfigMainWindow::closeEvent(), gconf does check the return value, but only
> >> > displays it in the bottom box of the main window instead of a message box.
> >> > Neither of them return failure in the error case. These bugs should be
> >> > indeed fixed.
> >> >
> >> agree.
> >>
> >> > But I don't like the directory permission check, it only
> >> > handles one case, but does not handle permission on the .config file itself
> >> > with KCONFIG_OVERWRITECONFIG=1, ENOSPC and so on.
> >> >
> >> seconded.
> > I'm sorry for forgetting to handle a case which was set KCONFIG_OVERWRITECONFIG.
> > So I rewrote the patch and attach it.
> >
> > Thanks for your advice.
> >
> > Signed-off-by: Hiromu Yakura <hiromu1996@gmail.com>
> >
> Let me re-state: your patch does not handle all the case where
> conf_write() may fails, and I do not think we want to preemptively
> check for all errors open(2) may return.
conf_write() is called after the configure changed.
So I don't think we should handle the failed case of conf_write()
because the purpose of this patch is not to losing changes.

Re: [PATCH] Kconfig: add warning about permission of config file

[Arnaud Lacombe]

Hi,

On Tue, May 24, 2011 at 4:46 PM, Hiromu Yakura <hiromu1996@gmail.com> wrote:
> [...]
> conf_write() is called after the configure changed.
> So I don't think we should handle the failed case of conf_write()
> because the purpose of this patch is not to losing changes.
>
Then your patch is incomplete. only open(2) and write(2) can fail in
more than 30 different ways, you check 1.

 - Arnaud

ps: I am not even addressing the race-condition introduced by your patch...