From: Raphael K. da C. <rakuco@FreeBSD.org> - 2012-05-12 20:52:23
|
Hey there. I was working on some code which uses evas_object_image_memfile_set here on FreeBSD, and it was failing. After some investigation, I found out that the problem was in the stat() call in evas_cache_image_request. On Linux, _create_tmpf() open()s a temporary file with O_CREAT, unlink()s it and then changes the actual filename to /proc/<PID>/fds/<GENERATED-FD>, which exists and reads/writes into the file descriptor returned by open(). On non-Linux systems, the filename renaming does not happen (since there's usually no /proc with that structure), so unlink() removes the file and the stat() in evas_cache_image_request() fails. |
From: Raphael K. da C. <rakuco@FreeBSD.org> - 2012-05-16 01:21:25
Attachments:
evas-fix-unlink-call-in-evas_objct_image.diff
|
Raphael Kubo da Costa <rakuco@FreeBSD.org> writes: > I was working on some code which uses evas_object_image_memfile_set here > on FreeBSD, and it was failing. > > After some investigation, I found out that the problem was in the stat() > call in evas_cache_image_request. > > On Linux, _create_tmpf() open()s a temporary file with O_CREAT, > unlink()s it and then changes the actual filename to > /proc/<PID>/fds/<GENERATED-FD>, which exists and reads/writes into the > file descriptor returned by open(). > > On non-Linux systems, the filename renaming does not happen (since > there's usually no /proc with that structure), so unlink() removes the > file and the stat() in evas_cache_image_request() fails. I've attached a patch to try to address this. The idea is to remove the unlink() call in _create_tmpf() and only remove the file in _cleanup_tmpf(). This has consequences for the Linux case as well, since the Linux code might also end up creating a file in /tmp if the attempt to use /dev/shm fails. Since the unlink() is now made unconditionally, the indirection code which made o->tmpf point to the process' /proc entry has also been removed, since we can't remove that. |
From: Lucas De M. <luc...@pr...> - 2012-05-16 13:13:30
|
On Tue, May 15, 2012 at 10:21 PM, Raphael Kubo da Costa <ra...@fr...> wrote: > Raphael Kubo da Costa <rakuco@FreeBSD.org> writes: > >> I was working on some code which uses evas_object_image_memfile_set here >> on FreeBSD, and it was failing. >> >> After some investigation, I found out that the problem was in the stat() >> call in evas_cache_image_request. >> >> On Linux, _create_tmpf() open()s a temporary file with O_CREAT, >> unlink()s it and then changes the actual filename to >> /proc/<PID>/fds/<GENERATED-FD>, which exists and reads/writes into the >> file descriptor returned by open(). >> >> On non-Linux systems, the filename renaming does not happen (since >> there's usually no /proc with that structure), so unlink() removes the >> file and the stat() in evas_cache_image_request() fails. > > I've attached a patch to try to address this. > > The idea is to remove the unlink() call in _create_tmpf() and only > remove the file in _cleanup_tmpf(). This has consequences for the Linux Not looking at the code, but from your description this is the wrong thing to do. The renaming there is probably a hack so code paths that calls stat() to check if the file exists. If you know it exists, you could rename it to "/" for example or any path in <put-here-your-choice-of-OS> that is guaranteed to exist in the meantime. Reading http://lists.freebsd.org/pipermail/freebsd-hackers/2007-November/022227.html it looks like you have an equivalent in FreeBSD in /dev/fd/. > case as well, since the Linux code might also end up creating a file in > /tmp if the attempt to use /dev/shm fails. Since the unlink() is now > made unconditionally, the indirection code which made o->tmpf point to > the process' /proc entry has also been removed, since we can't remove > that. Doing what I said above you wouldn't have this ugly side effect. > > > ------------------------------------------------------------------------------ > Live Security Virtual Conference > Exclusive live event will cover all the ways today's security and > threat landscape has changed and how IT managers can respond. Discussions > will include endpoint security, mobile security and the latest in malware > threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ > _______________________________________________ > enlightenment-devel mailing list > enl...@li... > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > |
From: Raphael K. da C. <rakuco@FreeBSD.org> - 2012-05-16 15:10:12
|
Lucas De Marchi <luc...@pr...> writes: > The renaming there is probably a hack so code paths that calls stat() > to check if the file exists. If you know it exists, you could rename > it to "/" for example or any path in <put-here-your-choice-of-OS> that > is guaranteed to exist in the meantime. Reading > http://lists.freebsd.org/pipermail/freebsd-hackers/2007-November/022227.html > it looks like you have an equivalent in FreeBSD in /dev/fd/. While that would work in theory, it also requires one to have an fdescfs mounted and still isn't very portable. The renaming is indeed a hack -- what I wonder is if it is worth keeping. |
From: Lucas De M. <luc...@pr...> - 2012-05-17 15:07:06
|
On Wed, May 16, 2012 at 12:09 PM, Raphael Kubo da Costa <ra...@fr...> wrote: > Lucas De Marchi <luc...@pr...> writes: > >> The renaming there is probably a hack so code paths that calls stat() >> to check if the file exists. If you know it exists, you could rename >> it to "/" for example or any path in <put-here-your-choice-of-OS> that >> is guaranteed to exist in the meantime. Reading >> http://lists.freebsd.org/pipermail/freebsd-hackers/2007-November/022227.html >> it looks like you have an equivalent in FreeBSD in /dev/fd/. > > While that would work in theory, it also requires one to have an fdescfs > mounted and still isn't very portable. > > The renaming is indeed a hack -- what I wonder is if it is worth > keeping. As you noticed, any code path that calls stat() to verify if the file indeed exists would fail. You'd need to investigate the code paths and refactor them: - If you receive an fd, stat(filename, ...) should not happen -> and the filename should be used for logging purposes and similar stuff. Postponing the removal like you did IMO is a hack to fix another hack. So, either do the same thing as in Linux (rename to something will continue to exist) or change all code paths. Lucas De Marchi |
From: Raphael K. da C. <rakuco@FreeBSD.org> - 2012-05-18 14:24:01
|
Lucas De Marchi <lucas.demarchi <at> profusion.mobi> writes: > > While that would work in theory, it also requires one to have an > > fdescfs mounted and still isn't very portable. The renaming is > > indeed a hack -- what I wonder is if it is worth keeping. > > As > > you noticed, any code path that calls stat() to verify if the file > > > indeed exists would fail. Why? The patch keeps the file created in /dev/shm, /tmp or whatever around for longer than the lifetime of _create_tmpf() precisely to make the stat() not fail. > You'd need to investigate the code paths and refactor them: - If you > receive an fd, stat(filename, ...) should not happen -> and the > filename should be used for logging purposes and similar stuff. That's what I am doing -- depending on how you view it, evas_object_image_memfile_set() is just a hack to make evas_object_image_file_set() work with memory addresses instead of actual files, and it achieves that by creating a temporary file and mmap()ing the memory addresses to that file. The rest of the codepath is just evas_object_image_file_set() being called, so nothing needs to change. > Postponing the removal like you did IMO is a hack to fix another > hack. So, either do the same thing as in Linux (rename to something > will continue to exist) or change all code paths. Erm, that's what my patch does, I think :-) |
From: Raphael K. da C. <rakuco@FreeBSD.org> - 2012-05-21 03:15:40
|
After talking about the previous version of the patch with raster on IRC, I'm sending a more conservative version that does not change the Linux behavior -- /dev/shm is expected to work on Linux, and the non-Linux case works at the expense of leaving some files in /tmp if a crash occurs before _cleanup_tmpf() is called. |
From: Joerg S. <jo...@br...> - 2012-05-21 08:28:39
|
On Mon, May 21, 2012 at 12:15:10AM -0300, Raphael Kubo da Costa wrote: > After talking about the previous version of the patch with raster on > IRC, I'm sending a more conservative version that does not change the > Linux behavior -- /dev/shm is expected to work on Linux, and the > non-Linux case works at the expense of leaving some files in /tmp if a > crash occurs before _cleanup_tmpf() is called. Can you not hard-code /tmp, but try to honor TMPDIR? Joerg |
From: Vincent T. <vin...@gm...> - 2012-05-21 08:50:36
|
On Mon, May 21, 2012 at 10:28 AM, Joerg Sonnenberger <jo...@br...> wrote: > On Mon, May 21, 2012 at 12:15:10AM -0300, Raphael Kubo da Costa wrote: >> After talking about the previous version of the patch with raster on >> IRC, I'm sending a more conservative version that does not change the >> Linux behavior -- /dev/shm is expected to work on Linux, and the >> non-Linux case works at the expense of leaving some files in /tmp if a >> crash occurs before _cleanup_tmpf() is called. > > Can you not hard-code /tmp, but try to honor TMPDIR? TMPDIR does not exist on Windows. TMP and TEMP do Vincent |
From: Raphael K. da C. <rakuco@FreeBSD.org> - 2012-05-23 19:29:25
|
Vincent Torri <vin...@gm...> writes: > On Mon, May 21, 2012 at 10:28 AM, Joerg Sonnenberger > <jo...@br...> wrote: >> On Mon, May 21, 2012 at 12:15:10AM -0300, Raphael Kubo da Costa wrote: >>> After talking about the previous version of the patch with raster on >>> IRC, I'm sending a more conservative version that does not change the >>> Linux behavior -- /dev/shm is expected to work on Linux, and the >>> non-Linux case works at the expense of leaving some files in /tmp if a >>> crash occurs before _cleanup_tmpf() is called. >> >> Can you not hard-code /tmp, but try to honor TMPDIR? > > TMPDIR does not exist on Windows. TMP and TEMP do New attempt attached: we now check TMPDIR, TMP and TEMP (in this order) before falling back to "/tmp". |
From: Carsten H. (T. R. <ra...@ra...> - 2012-05-24 05:07:21
|
On Wed, 23 May 2012 16:28:52 -0300 Raphael Kubo da Costa <rakuco@FreeBSD.org> said: > Vincent Torri <vin...@gm...> writes: > > > On Mon, May 21, 2012 at 10:28 AM, Joerg Sonnenberger > > <jo...@br...> wrote: > >> On Mon, May 21, 2012 at 12:15:10AM -0300, Raphael Kubo da Costa wrote: > >>> After talking about the previous version of the patch with raster on > >>> IRC, I'm sending a more conservative version that does not change the > >>> Linux behavior -- /dev/shm is expected to work on Linux, and the > >>> non-Linux case works at the expense of leaving some files in /tmp if a > >>> crash occurs before _cleanup_tmpf() is called. > >> > >> Can you not hard-code /tmp, but try to honor TMPDIR? > > > > TMPDIR does not exist on Windows. TMP and TEMP do > > New attempt attached: we now check TMPDIR, TMP and TEMP (in this order) > before falling back to "/tmp". beyond using TMPDIR, TMP and TEMP env vars... i dont see how this helps much :) in fact they probably should be there in if (fd < 0). on non-linux it STAR TS with fd being -1 so it goes right it the failure case. for linx this means failure case still works. we.l that and the ifdef around the unlink. i've put in an alternate patch in svn that keeps the tmpdir fallback for evas. -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ra...@ra... |
From: Raphael K. da C. <rakuco@FreeBSD.org> - 2012-05-24 19:35:42
|
Carsten Haitzler (The Rasterman) <ra...@ra...> writes: > beyond using TMPDIR, TMP and TEMP env vars... i dont see how this > helps much :) in fact they probably should be there in if (fd < 0). on > non-linux it STAR TS with fd being -1 so it goes right it the failure > case. for linx this means failure case still works. we.l that and the > ifdef around the unlink. > > i've put in an alternate patch in svn that keeps the tmpdir fallback > for evas. The reason I stopped treating the failure case on Linux is that when we discussed the patch on IRC you said we shouldn't take the case of /dev/shm not being present into account. Anyway, glad to see that this has finally been fixed. Should this be backported, as currently no stable release has that function working on non-Linux platforms? Do I need to write NEWS/ChangeLog items? |
From: Carsten H. (T. R. <ra...@ra...> - 2012-05-24 23:01:30
|
On Thu, 24 May 2012 16:35:23 -0300 Raphael Kubo da Costa <rakuco@FreeBSD.org> said: > Carsten Haitzler (The Rasterman) <ra...@ra...> writes: > > > beyond using TMPDIR, TMP and TEMP env vars... i dont see how this > > helps much :) in fact they probably should be there in if (fd < 0). on > > non-linux it STAR TS with fd being -1 so it goes right it the failure > > case. for linx this means failure case still works. we.l that and the > > ifdef around the unlink. > > > > i've put in an alternate patch in svn that keeps the tmpdir fallback > > for evas. > > The reason I stopped treating the failure case on Linux is that when we > discussed the patch on IRC you said we shouldn't take the case of > /dev/shm not being present into account. Anyway, glad to see that this > has finally been fixed. well /dev/shm not being there is going to be a REALLy rare day in hell, and with cserve blah blah blah - we kind of cant work without it anymore. :) but the code is easier and cleaner to read imho if we keep the failure case for linux anyway. > Should this be backported, as currently no stable release has that > function working on non-Linux platforms? Do I need to write > NEWS/ChangeLog items? good question. i suppose it should be backported. :) -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ra...@ra... |
From: Raphael K. da C. <rakuco@FreeBSD.org> - 2012-05-24 23:30:28
|
Carsten Haitzler (The Rasterman) <ra...@ra...> writes: > On Thu, 24 May 2012 16:35:23 -0300 Raphael Kubo da Costa <rakuco@FreeBSD.org> > said: >> Should this be backported, as currently no stable release has that >> function working on non-Linux platforms? Do I need to write >> NEWS/ChangeLog items? > > good question. i suppose it should be backported. :) I don't have commit superpowers, so I guess you'll have to do it yourself. I think the patch should apply cleanly to all branches (1.0, 1.1 and 1.2). Please let me know if I can help with anything. |
From: Carsten H. (T. R. <ra...@ra...> - 2012-05-30 05:53:55
|
On Thu, 24 May 2012 20:29:57 -0300 Raphael Kubo da Costa <rakuco@FreeBSD.org> said: > Carsten Haitzler (The Rasterman) <ra...@ra...> writes: > > > On Thu, 24 May 2012 16:35:23 -0300 Raphael Kubo da Costa > > <rakuco@FreeBSD.org> said: > >> Should this be backported, as currently no stable release has that > >> function working on non-Linux platforms? Do I need to write > >> NEWS/ChangeLog items? > > > > good question. i suppose it should be backported. :) > > I don't have commit superpowers, so I guess you'll have to do it > yourself. I think the patch should apply cleanly to all branches (1.0, > 1.1 and 1.2). Please let me know if I can help with anything. done. back to 1.2 -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ra...@ra... |