Re: g_file_write()

[Soeren Sandmann <sandmann daimi au dk>]

"Alexis S. L. Carvalho" <alexis cecm usp br> writes:

> > +static gchar *
> > +write_to_temp_file (const gchar *contents,
> > +		    gsize length,
> 
> <nitpick>
> gssize to be consistent with g_file_replace?
> </nitpick>

I have changed it, but I don't see why it
matters. write_to_temp_file() is not called with negative arguments.

> > +  
> > +  tmp_name = g_strdup_printf (".%s.XXXXXX", template);
> > +
> 
> Again - if the user specifies e.g. the absolute pathname of the file
> (/foo/bar), you'll try to create a temp file with a name like
> "./foo/bar", which will probably fail.
> 
> If you want the temp file to be a dot-file, you'll have to play with
> g_path_get_basename and g_path_get_dirname .

Thanks, it took me a while to realize what you meant. I kept thinking
you were suggesting that creating the file in the same directory was
somehow a problem.

> Maybe also mention that if the file was a symlink, the symlink itself
> will be replaced?

Done.

> 
> And I'm guessing the permissions issue may become a common gotcha with
> this interface.  Testing it here (debian unstable) the new file was
> created with permissions 600, which I guess is the lesser evil from a
> security standpoint, but may cause problems if the file was supposed to
> be readable by more than one user.

I think this is a serious problem. The file should be created with the
umask permissions. You can do this:

#ifndef G_OS_WIN32
 {
   mode_t mode = umask (0);
   umask (mode);

   chmod (filename, mode);
 }
#endif

but then there is a race condition with another thread setting
umask. This can be avoided by doing it in a separate process:

pid = fork();

if (pid == -1)
{
   report error
}
else if (pid == 0)
{
   /* child */
   umask/chmod ...
   exit (errno);
}
else
{
   /* parent */
   int err;

   waitpid(pid, &err, 0);

   if (err != 0)
       ....;
}

Is there a simpler way to do this? 

> > +gboolean
> > +g_file_replace (const gchar *filename, 
> > +		const gchar *contents,
> > +		gssize	     length,
> > +		GError	   **error)
> 
> 
> > +
> > +  display_tmpname = g_filename_display_name (tmp_filename);
> 
> this is not used anymore
> 
> > +  display_filename = g_filename_display_name (filename);
> 
> this is used only in one place in the error path for G_OS_WIN32, so it
> may make sense to move it into the "if (!rename_file())"

Sven already fixed those.


Søren