Midori Web Browser

Bug #1203240
Comment #35

Comment 35 for bug 1203240

Revision history for this message

In bugs.webkit.org/ #119477, Calvaris (calvaris) wrote on 2017-02-02:

#35

(In reply to comment #26)
> > Another thing is that I think we should ensure the files are created in a different directory for each user and ensure that directory has the proper permissions so that it cannot be access by anyone else other than the user. I think media people watch online can vary a lot from cats to other things. I'd write a function to return the template and ensure it is placed in the proper directory.
>
> This is tricky and, IMHO, not needed.
>
> Having and extra directory under /var/tmp means that we would have to
> maintain (delete) that directory by ourselves. This is problematic because
> we can't use the elegant trick of deleting the file right after its creation
> to delete the directory. If several videos are being played at once by
> different WebProcesses a race condition can happen:
>
> - Process A creates /var/tmp/$USER
> - Process A creates WebKit-Media-123456 there
> - Process B tries to create /var/tmp/$USER (but it's already created, no
> problem)
> - Process A deletes WebKit-Media-123456 there and /var/tmp/$USER
> - Process B creates WebKit-Media-ABCDEF there but... the directory doesn't
> exist anymore. ERROR!
>
> Anyway, the directory itself isn't needed because the new temporary files
> are now deleted right after their creation. This means that no other
> processes can open them anymore, so there shouldn't be any concern regarding
> privacy. I've checked it by myself with a "while true; do ls /var/tmp; done"
> script.

Well, I don't think deleting the directory is needed, it is just 4k on disk.

> > Yep, this will purge the old files. I have a concern though for the files that fall under the GST_WARNING of line 1377 of not being able to unlink. Should we assume that is is very unlikely and forget (and trust distros to purge this from time to time)? Btw, if it is so unlikely, we can tag the decission as UNLIKELY().
>
> Michael would say that WebKit should try hard to clean its own files. I
> don't think distros are going to mess with the contents of the user home
> dir, even for the .cache dir. Thanks for the UNLIKELY() suggestion, btw.

I think I wasn't clear here. Line 1377, if the regular unlink fails (not the purge one, the regular) we are leaking them (under /var/tmp/).

(In reply to comment #26)
> > Another thing is that I think we should ensure the files are created in a different directory for each user and ensure that directory has the proper permissions so that it cannot be access by anyone else other than the user. I think media people watch online can vary a lot from cats to other things. I'd write a function to return the template and ensure it is placed in the proper directory.
> 
> This is tricky and, IMHO, not needed.
> 
> Having and extra directory under /var/tmp means that we would have to
> maintain (delete) that directory by ourselves. This is problematic because
> we can't use the elegant trick of deleting the file right after its creation
> to delete the directory. If several videos are being played at once by
> different WebProcesses a race condition can happen:
> 
> - Process A creates /var/tmp/$USER
> - Process A creates WebKit-Media-123456 there
> - Process B tries to create /var/tmp/$USER (but it's already created, no
> problem)
> - Process A deletes WebKit-Media-123456 there and /var/tmp/$USER
> - Process B creates WebKit-Media-ABCDEF there but... the directory doesn't
> exist anymore. ERROR!
> 
> Anyway, the directory itself isn't needed because the new temporary files
> are now deleted right after their creation. This means that no other
> processes can open them anymore, so there shouldn't be any concern regarding
> privacy. I've checked it by myself with a "while true; do ls /var/tmp; done"
> script.

Well, I don't think deleting the directory is needed, it is just 4k on disk.

> > Yep, this will purge the old files. I have a concern though for the files that fall under the GST_WARNING of line 1377 of not being able to unlink. Should we assume that is is very unlikely and forget (and trust distros to purge this from time to time)? Btw, if it is so unlikely, we can tag the decission as UNLIKELY().
> 
> Michael would say that WebKit should try hard to clean its own files. I
> don't think distros are going to mess with the contents of the user home
> dir, even for the .cache dir. Thanks for the UNLIKELY() suggestion, btw.

I think I wasn't clear here. Line 1377, if the regular unlink fails (not the purge one, the regular) we are leaking them (under /var/tmp/).