Re: log files and permissions

Itagaki Takahiro wrote:

I checked "log_file_mode GUC" patch, and found a couple of Windows-specific
and translation issues.

Thank you for the review. Attached patch attempts to fix these issues.

* fchmod() is not available on some platforms, including Windows.
fh = fopen(filename, mode);
setvbuf(fh, NULL, LBF_MODE, 0);
fchmod(fileno(fh), Log_file_mode);

I've changed that to using chmod(), that should be available everywhere and
is already used in several places in Postgres code.

* How does the file mode work on Windows?
If it doesn't work, we should explain it in docs.

Indeed it seems that chmod() doesn't actually do anything useful on Windows.
So I've added a documentation note about it and put an #ifndef WIN32 around
the chmod() call.

It might look a duplication of codes, but I think this form is better
because we can reuse the existing translation catalogs.
if (am_rotating)
ereport(FATAL, ... "could not create log file ...);
else
ereport(LOG, ... "could not open new log file ...);

Fixed.

regards,
Martin

On Mon, Jul 12, 2010 at 7:36 PM, Martin Pihlak <martin.pihlak@gmail.com> wrote:

Itagaki Takahiro wrote:

I checked "log_file_mode GUC" patch, and found a couple of Windows-specific
and translation issues.

Thank you for the review. Attached patch attempts to fix these issues.

+	if (!*value || *endptr || file_mode < 0 || file_mode > 0777)
+	{
+		ereport(GUC_complaint_elevel(source),
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("invalid value for parameter \"log_file_mode\"")));

The sticky bit cannot be set via log_file_mode. Is this intentional?

+#ifndef WIN32
+		chmod(filename, Log_file_mode);
+#endif

Don't we need to check the return value of chmod()?

+const char *assign_log_file_mode(const char *value,
+							bool doit, GucSource source);
+const char *show_log_file_mode(void);

You forgot to write the show_log_file_mode()? I was not able to find that
in the patch.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

I think the patch is almost ready for committer except the following
three issues:

2010/7/13 Fujii Masao <masao.fujii@gmail.com>:

+     if (!*value || *endptr || file_mode < 0 || file_mode > 0777)

The sticky bit cannot be set via log_file_mode. Is this intentional?

We should also check the value not to be something like 0699.
How about checking it with (file_mode & ~0666) != 0 ?

+#ifndef WIN32
+             chmod(filename, Log_file_mode);
+#endif

Don't we need to check the return value of chmod()?

I prefer umask() rather than chmod() here.

+const char *show_log_file_mode(void);

You forgot to write the show_log_file_mode()? I was not able to find that
in the patch.

I want show_log_file_mode to print the setting value in octal format.

--
Itagaki Takahiro

Itagaki Takahiro wrote:

I think the patch is almost ready for committer except the following
three issues:

2010/7/13 Fujii Masao <masao.fujii@gmail.com>:

+ if (!*value || *endptr || file_mode < 0 || file_mode > 0777)

The sticky bit cannot be set via log_file_mode. Is this intentional?

Yes -- I don't think there is a use case for sticky or setuid bits on log
files, even allowing execute is questionable.

We should also check the value not to be something like 0699.
How about checking it with (file_mode & ~0666) != 0 ?

Aha, that would ensure that the execute bit is not specified. Works for me.
The 0699 and other invalid octal values are caught by strtol()

+#ifndef WIN32
+             chmod(filename, Log_file_mode);
+#endif

Don't we need to check the return value of chmod()?

I prefer umask() rather than chmod() here.

Converted to umask()

+const char *show_log_file_mode(void);

You forgot to write the show_log_file_mode()? I was not able to find that
in the patch.

I want show_log_file_mode to print the setting value in octal format.

I've now (re)added the show_log_file_mode(). It used to be there, but then
at some point I decided to display the value "as-is".

While going through it, I moved the _setmode() call for win32 to logfile_open().

regards,
Martin

Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:

...
We should also check the value not to be something like 0699.
How about checking it with (file_mode & ~0666) != 0 ?
...
I want show_log_file_mode to print the setting value in octal format.

It seems like a whole lot of lily-gilding is going on here. Just make
it work like unix_socket_permissions already does. That's been there
for years and nobody has complained about it.

regards, tom lane

Tom Lane wrote:

Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:

...
We should also check the value not to be something like 0699.
How about checking it with (file_mode & ~0666) != 0 ?
...
I want show_log_file_mode to print the setting value in octal format.

It seems like a whole lot of lily-gilding is going on here. Just make
it work like unix_socket_permissions already does. That's been there
for years and nobody has complained about it.

Thanks, somehow I missed that we can already specify octal integers
as GUC-s. I now converted the log_file_mode to integer and dropped
the assign_log_file_mode function.

regards,
Martin

Martin Pihlak <martin.pihlak@gmail.com> writes:

Thanks, somehow I missed that we can already specify octal integers
as GUC-s. I now converted the log_file_mode to integer and dropped
the assign_log_file_mode function.

Applied with a few corrections. The noncosmetic changes were:

* prevent Log_file_mode from disabling S_IWUSR permissions --- we had
better be able to write the files no matter what.

* save and restore errno across ereport() call; needed since some
callers look at errno after a failure.

* make unix_socket_permissions print its value in octal, for consistency
with log_file_mode.

BTW, I'm not 100% convinced that having the octal show-functions is
a good idea, mainly because they aren't consistent with the other
columns in pg_settings:

regression=# select * from pg_settings where name = 'log_file_mode';
     name      | setting | unit |               category               |        
        short_desc                |                                             
                                                  extra_desc                    
                                                                            | co
ntext | vartype | source  | min_val | max_val | enumvals | boot_val | reset_val 
| sourcefile | sourceline 
---------------+---------+------+--------------------------------------+--------
----------------------------------+---------------------------------------------
--------------------------------------------------------------------------------
----------------------------------------------------------------------------+---
------+---------+---------+---------+---------+----------+----------+-----------
+------------+------------
 log_file_mode | 0600    |      | Reporting and Logging / Where to Log | Sets th
e file permissions for log files. | The parameter value is expected to be a nume
ric mode specification in the form accepted by the chmod and umask system calls.
 (To use the customary octal format the number must start with a 0 (zero).) | si
ghup  | integer | default | 0       | 511     |          | 384      | 384       
|            |           
(1 row)

I guess this is not strictly incorrect, as long as you understand what
the leading '0' means per C conventions, but it looks a bit weird.
However, we're not going to be able to improve on this without a lot more
hackery than I think it's worth.

regards, tom lane