Bug 15776

(In reply to comment #0)
> Created an attachment (id=16270) [details]
> 0001-Fix-a-crash-if-xorg.conf-doesn-t-have-a-Files-sectio.patch
> 
> The crash happens when opening the "Expert Mode" interface
> in xorgcfg.
> 
> I think this patch for git master is included in the tarball
> in https://bugs.freedesktop.org/show_bug.cgi?id=14730, and
> (also think) I posted a non git patch some time ago just
> the mailing list.
> 
> Please also apply to git master.
> 

wouldn't it be easier to just have a 

value = (file && file->file_logfile) ? file->file_logfile : "";

instead of allocating the empty storage? file isn't used anywhere else in this function.

(In reply to comment #1)
> > Please also apply to git master.
> > 
> 
> wouldn't it be easier to just have a 
> 
> value = (file && file->file_logfile) ? file->file_logfile : "";
> 
> instead of allocating the empty storage? file isn't used anywhere else in this
> function.

  I think I need to check it more carefully for git master, as 
configuration for rgb path and log file was removed (maybe due
to xorg.conf being read after those values being required...,
would also need to check more carefully).
  But if all fields are empty, having XF86Config->conf_files as
NULL should just not create an empty Files section when saving the
configuration file (otherwise there may exist some other code not
checking for a NULL pointer elsewhere).

  But yes, it should just work, but it has been quite a lot of time
since I wrote that code, that I don't remember all details :-)

(In reply to comment #2)
>   But if all fields are empty, having XF86Config->conf_files as
> NULL should just not create an empty Files section when saving the
> configuration file 

you lost me there.

> (otherwise there may exist some other code not checking for a NULL pointer elsewhere).

if that happens, we fix it where it happens. seems a better solution than allocing memory just to not trip over bugs.


If you post an updated patch, I'll be happy to merge it.
 

Created attachment 16284 [details] [review]
0001-Fix-a-crash-if-xorg.conf-doesn-t-have-a-Files-sectio.patch

  This is an alternate patch that creates the XF86Config->conf_files
on demand, i.e. only if it is required, so that an empty Files section
will not be created by just opening the "Expert Mode" interface
and saving the generated xorg.conf. But I think that should not
really be a problem...

  Another alternative, to keep a simpler code and prevent similar
problems (that on a overview don't exist), could be to ensure
all XF86Config fields are intialized in config.c:StartConfig() but
that would create empty sections in xorg.conf for those sections
without parameters/options.

  A bit off topic (but maybe not really :-), xorgcfg also needs bitmaps
that are usually installed in devel packages by distros, it also uses
several bitmaps that are in xorg/data/bitmaps and at least one that is
installed with the bitmap app.

(In reply to comment #4)
> Created an attachment (id=16284) [details]
> 0001-Fix-a-crash-if-xorg.conf-doesn-t-have-a-Files-sectio.patch
> 
>   This is an alternate patch that creates the XF86Config->conf_files
> on demand, i.e. only if it is required, so that an empty Files section
> will not be created by just opening the "Expert Mode" interface
> and saving the generated xorg.conf. But I think that should not
> really be a problem...

Pushed as 01c61f3d972fc2f4e5bb536dd00d8b6bbeb0fb3d. Thanks.