Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Opening attachments with commands which use a subshell are racy? #276

Closed
romanofski opened this issue Mar 30, 2019 · 4 comments
Closed

Opening attachments with commands which use a subshell are racy? #276

romanofski opened this issue Mar 30, 2019 · 4 comments
Assignees
Labels
Milestone

Comments

@romanofski
Copy link
Member

Describe the bug
I've seen instances in which opening attachments with firefox end up with an error: the file can not be opened because it can not be found. This seems to be happening at random, although opening the same attachment a second time seems to work (I have not determined what the success rate is).

One thing however which sticks out is that the typical firefox program on Linux is a bash script. Purebred runs the invocation of Firefox in a shell itself. Commands like elinks seem to not have any trouble. My suspicion is, that the shell itself returns and therefore causes Purebred to close and remove the temporary file.

To Reproduce
Steps to reproduce the behavior:

  1. Open an e-mail.
  2. See list of attachments
  3. Pick an HTML attachment and open it with firefox (should be a shell script)
  4. See error

Expected behavior
No random failures due to the removal of the temporary file.

Screenshots
none

Additional context
Not sure what a good solution here is other than not remove the temporary file when the sub process has exited.

@romanofski romanofski added the bug label Mar 30, 2019
@romanofski romanofski added this to the 1.0 milestone Mar 30, 2019
@frasertweedale
Copy link
Member

Yep, for sure this is the child process exiting after signalling some other process (i.e. the already-running firefox instance) to open the file.

We need a knob that says whether or not to delete the file after child process returns.

@romanofski
Copy link
Member Author

After merging #275 I think fixing this item could also add a bit of cleanup for our open and pipe actions. I envision using a data type which can do a bracket style side effect. I haven't thought it through how exactly it will look like and how to incorporate the recent work on #269 but I hope it can be used to cleanup the duplication with invokeEditor, pipeCommand and openCommand.

@romanofski romanofski self-assigned this Apr 1, 2019
@romanofski
Copy link
Member Author

I've investigated the problem a bit further and stumbled over a script which employs inotify to watch when the file is closed by the child process. I suppose just watching if the file is closed will cause the same problem, but knowing about inotify I suppose an action could register a handler upon opening the file which removes it on a subsequent close.

The down side of this is, that it only works on Linux (or systems which support inotify) and therefore would restrict our portability (if that's even a thing).

I guess keeping the temporary file for now is the way to go and thinking about further fine grained management could be a thing if the need arises.

@frasertweedale
Copy link
Member

Yikes, let's not go there. We just have a two-state knob, delete-after-child-termintes or not. The "not" case comes with the obvious caveat, and we move on.

romanofski added a commit that referenced this issue Apr 9, 2019
This removes quite a bit of old code and introduces a function which takes care
of most of the boring bits when handling attachments. It uses a bracket style
pattern to either clean up or not the created temporary file, depending what
type of command we're using (e.g. open vs. pipe).

Fixes #276
romanofski added a commit that referenced this issue Apr 13, 2019
This removes quite a bit of old code and introduces a function which takes care
of most of the boring bits when handling attachments. It uses a bracket style
pattern to either clean up or not the created temporary file, depending what
type of command we're using (e.g. open vs. pipe).

Fixes #276
romanofski added a commit that referenced this issue Apr 20, 2019
This removes quite a bit of old code and introduces a function which takes care
of most of the boring bits when handling attachments. It uses a bracket style
pattern to either clean up or not the created temporary file, depending what
type of command we're using (e.g. open vs. pipe).

Fixes #276
romanofski added a commit that referenced this issue Apr 20, 2019
This removes old cold of dealing with temporary files when opening and piping
attachments. The patch introduces new data types for handling resources in order
to be flexible enought to delete the temporary file or not after process exit.

Furthermore, the patch removes any passing of commands to the shell (e.g.
opening or piping). That was a very convenient way of passing additional
arguments to the command which is not possible any more. However just passing
arbitrary commands to the shell is inherently insecure and we better avoid it
were possible.

Related #276
romanofski added a commit that referenced this issue Apr 20, 2019
When opening attachments with mailcap entries, it is sometimes possible that the
actual binary just sends a message to the main program and exits. Deleting the
temporary file upon process exit would cause the main program upon opening the
temporary file to fail since the file would have been deleted by that time.
Sometimes opening the file from the main program just works, because the time
difference between exiting and opening is really quick, but mostly it doesn't.

Instead, make the mailcap entry flexible so that depending on the command we
either keep or delete the temporary file.

Fixes #276
romanofski added a commit that referenced this issue Apr 20, 2019
This removes old cold of dealing with temporary files when opening and piping
attachments. The patch introduces new data types for handling resources in order
to be flexible enought to delete the temporary file or not after process exit.

Furthermore, the patch removes any passing of commands to the shell (e.g.
opening or piping). That was a very convenient way of passing additional
arguments to the command which is not possible any more. However just passing
arbitrary commands to the shell is inherently insecure and we better avoid it
were possible.

Related #276
romanofski added a commit that referenced this issue Apr 20, 2019
When opening attachments with mailcap entries, it is sometimes possible that the
actual binary just sends a message to the main program and exits. Deleting the
temporary file upon process exit would cause the main program upon opening the
temporary file to fail since the file would have been deleted by that time.
Sometimes opening the file from the main program just works, because the time
difference between exiting and opening is really quick, but mostly it doesn't.

Instead, make the mailcap entry flexible so that depending on the command we
either keep or delete the temporary file.

Fixes #276
romanofski added a commit that referenced this issue Apr 20, 2019
This removes old cold of dealing with temporary files when opening and piping
attachments. The patch introduces new data types for handling resources in order
to be flexible enought to delete the temporary file or not after process exit.

Furthermore, the patch removes any passing of commands to the shell (e.g.
opening or piping). That was a very convenient way of passing additional
arguments to the command which is not possible any more. However just passing
arbitrary commands to the shell is inherently insecure and we better avoid it
were possible.

Related #276
romanofski added a commit that referenced this issue Apr 20, 2019
When opening attachments with mailcap entries, it is sometimes possible that the
actual binary just sends a message to the main program and exits. Deleting the
temporary file upon process exit would cause the main program upon opening the
temporary file to fail since the file would have been deleted by that time.
Sometimes opening the file from the main program just works, because the time
difference between exiting and opening is really quick, but mostly it doesn't.

Instead, make the mailcap entry flexible so that depending on the command we
either keep or delete the temporary file.

Fixes #276
romanofski added a commit that referenced this issue Apr 20, 2019
This removes old cold of dealing with temporary files when opening and piping
attachments. The patch introduces new data types for handling resources in order
to be flexible enought to delete the temporary file or not after process exit.

Furthermore, the patch removes any passing of commands to the shell (e.g.
opening or piping). That was a very convenient way of passing additional
arguments to the command which is not possible any more. However just passing
arbitrary commands to the shell is inherently insecure and we better avoid it
were possible.

Related #276
romanofski added a commit that referenced this issue Apr 20, 2019
When opening attachments with mailcap entries, it is sometimes possible that the
actual binary just sends a message to the main program and exits. Deleting the
temporary file upon process exit would cause the main program upon opening the
temporary file to fail since the file would have been deleted by that time.
Sometimes opening the file from the main program just works, because the time
difference between exiting and opening is really quick, but mostly it doesn't.

Instead, make the mailcap entry flexible so that depending on the command we
either keep or delete the temporary file.

Fixes #276
romanofski added a commit that referenced this issue Apr 20, 2019
This removes old cold of dealing with temporary files when opening and piping
attachments. The patch introduces new data types for handling resources in order
to be flexible enought to delete the temporary file or not after process exit.

Furthermore, the patch removes any passing of commands to the shell (e.g.
opening or piping). That was a very convenient way of passing additional
arguments to the command which is not possible any more. However just passing
arbitrary commands to the shell is inherently insecure and we better avoid it
were possible.

Related #276
romanofski added a commit that referenced this issue Apr 20, 2019
When opening attachments with mailcap entries, it is sometimes possible that the
actual binary just sends a message to the main program and exits. Deleting the
temporary file upon process exit would cause the main program upon opening the
temporary file to fail since the file would have been deleted by that time.
Sometimes opening the file from the main program just works, because the time
difference between exiting and opening is really quick, but mostly it doesn't.

Instead, make the mailcap entry flexible so that depending on the command we
either keep or delete the temporary file.

Fixes #276
romanofski added a commit that referenced this issue Apr 20, 2019
This removes old cold of dealing with temporary files when opening and piping
attachments. The patch introduces new data types for handling resources in order
to be flexible enought to delete the temporary file or not after process exit.

Furthermore, the patch removes any passing of commands to the shell (e.g.
opening or piping). That was a very convenient way of passing additional
arguments to the command which is not possible any more. However just passing
arbitrary commands to the shell is inherently insecure and we better avoid it
were possible.

Related #276
romanofski added a commit that referenced this issue Apr 20, 2019
When opening attachments with mailcap entries, it is sometimes possible that the
actual binary just sends a message to the main program and exits. Deleting the
temporary file upon process exit would cause the main program upon opening the
temporary file to fail since the file would have been deleted by that time.
Sometimes opening the file from the main program just works, because the time
difference between exiting and opening is really quick, but mostly it doesn't.

Instead, make the mailcap entry flexible so that depending on the command we
either keep or delete the temporary file.

Fixes #276
romanofski added a commit that referenced this issue Apr 20, 2019
This removes old cold of dealing with temporary files when opening and piping
attachments. The patch introduces new data types for handling resources in order
to be flexible enought to delete the temporary file or not after process exit.

Furthermore, the patch removes any passing of commands to the shell (e.g.
opening or piping). That was a very convenient way of passing additional
arguments to the command which is not possible any more. However just passing
arbitrary commands to the shell is inherently insecure and we better avoid it
were possible.

Related #276
romanofski added a commit that referenced this issue Apr 20, 2019
When opening attachments with mailcap entries, it is sometimes possible that the
actual binary just sends a message to the main program and exits. Deleting the
temporary file upon process exit would cause the main program upon opening the
temporary file to fail since the file would have been deleted by that time.
Sometimes opening the file from the main program just works, because the time
difference between exiting and opening is really quick, but mostly it doesn't.

Instead, make the mailcap entry flexible so that depending on the command we
either keep or delete the temporary file.

Fixes #276
romanofski added a commit that referenced this issue Apr 24, 2019
This removes old cold of dealing with temporary files when opening and piping
attachments. The patch introduces new data types for handling resources in order
to be flexible enought to delete the temporary file or not after process exit.

Furthermore, the patch removes any passing of commands to the shell (e.g.
opening or piping). That was a very convenient way of passing additional
arguments to the command which is not possible any more. However just passing
arbitrary commands to the shell is inherently insecure and we better avoid it
were possible.

Related #276
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants