Bug 47705 - Using O_CLOEXEC
Summary: Using O_CLOEXEC
Status: RESOLVED FIXED
Alias: None
Product: fontconfig
Classification: Unclassified
Component: library (show other bugs)
Version: 2_1
Hardware: Other All
: medium enhancement
Assignee: Akira TAGOH
QA Contact: Behdad Esfahbod
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-22 03:58 UTC by Petr Gajdos
Modified: 2013-01-08 06:45 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Using O_CLOEXEC in 2.9.0. (2.09 KB, patch)
2012-03-22 03:58 UTC, Petr Gajdos
Details | Splinter Review

Description Petr Gajdos 2012-03-22 03:58:00 UTC
Created attachment 58855 [details] [review]
Using O_CLOEXEC in 2.9.0.

Shouldn't fontconfig use O_CLOEXEC when opening files?

(btw: thanks for releasing 2.9.0!)
Comment 1 Petr Gajdos 2012-03-22 04:04:14 UTC
(This patch created Christian Rodríguez.)
Comment 2 Behdad Esfahbod 2012-03-22 08:03:21 UTC
Comment on attachment 58855 [details] [review]
Using O_CLOEXEC in 2.9.0.

Review of attachment 58855 [details] [review]:
-----------------------------------------------------------------

::: src/fcatomic.c.orig
@@ +113,1 @@
>      fd = mkstemp ((char *) atomic->tmp);

Maybe use the fcntl() equivalent in this case?

@@ +115,3 @@
>      if (fd < 0)
>  	return FcFalse;
> +    f = fdopen (fd, "we");

Do other systems just ignore the 'e' modifier transparently?  Infact, since this is fdopen as opposed to fopen, I think you don't need the modifier, since the fd is already open.

::: src/fccache.c.orig
@@ +216,4 @@
>      if (FcStat (cache_file, file_stat) < 0)
>          return -1;
>  #endif
> +    fd = open((char *) cache_file, O_RDONLY | O_BINARY | O_CLOEXEC);

Since O_CLOEXEC is a Linux extension, you need ifdef around this I guess?  And fall back to the fcntl() equivalent?  Looks like we need an internal FcOpen :).

@@ +963,4 @@
>      if (!FcAtomicLock (atomic))
>  	goto bail3;
>  
> +    fd = open((char *)FcAtomicNewFile (atomic), O_RDWR | O_CREAT | O_BINARY | O_CLOEXEC, 0666);

Same here.
Comment 3 Petr Gajdos 2012-03-23 01:20:59 UTC
I'll try to improve this patch ;-].
Comment 4 Akira TAGOH 2012-12-06 11:25:33 UTC
just for proposed fix:
  http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=bz47705

Hmm, FWIW git-bz seems not working here since a while ago but anyway.
Comment 5 Akira TAGOH 2013-01-08 06:45:43 UTC
committed the changes into git master.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.