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

WIP: use mkdtemp for IPC paths #343

Closed
wants to merge 2 commits into from
Closed

Conversation

jnpkrn
Copy link
Contributor

@jnpkrn jnpkrn commented Mar 22, 2019

work in progress:

  • core implementation
  • code cleanup, doc, valgrind sanitization etc.
  • chmod + chown recursion, also reconsider the permissions
    scheme (single perms guard at the dir + lax approach for items,
    vs. everything strict) and sanity checking
    (data contained within the same parent dir header
    enforcement in client)
  • rmdir at more places as a proper cleanup
  • using gnulib surrogate for mkdtemp where not available natively

This was referenced Mar 22, 2019
Previously, it was mistakenly too lax.  Umask ditto (no need
for preserving executable bit for whatever reason, see also
what POSIX.1-2008 mandates).

Signed-off-by: Jan Pokorný <[email protected]>
@jnpkrn jnpkrn force-pushed the ipc-mkdtemp branch 3 times, most recently from 2473fd1 to b2597a0 Compare March 22, 2019 22:44
TODO:
- code cleanup, doc, valgrind sanitization etc.
- chmod + chown, also reconsider the permissions scheme
- rmdir at more places as a proper cleanup
- using gnulib surrogate for mkdtemp where not available natively

Signed-off-by: Jan Pokorný <[email protected]>
@radosroka
Copy link

Hi @jnpkrn , is this also going to fix #369 ?

@jnpkrn
Copy link
Contributor Author

jnpkrn commented Jan 8, 2020

is this also going to fix #369?

For explanation, this was to demonstrate (mainly to @chrissie-c who
originally set off in the ill-devised direction):

#339 (comment)

@radosroka:

You apparently struggle with phenomenon of directory ownership change
(that you seem would expect not to be mangled in contrast how it would
have been by standard process-to-objects permissions transfer), but
rest assure this PR is definitely not going to help (if anything, it
does not handle that problem at all -- while it was apparent it needs
some sort of assessment should it be eventually picked -- see the TODO
notes:

  • chmod + chown, also reconsider the permissions scheme

That being said, closing this PR so as not to cast any confusions.

@jnpkrn jnpkrn closed this Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants