Don't ever claim /dev/null is an execpath. #12516
Conversation
|
This is a cleanup related to #12514 but split out because it might break blaze due to fileset things I can't see. |
benjaminp
force-pushed
the
empty-virtual
branch
2 times, most recently
from
633accd to
507a47e
Compare
| @Nullable private final PathFragment execPath; | ||
|
|
||
| private EmptyActionInput() { | ||
| execPath = null; |
There was a problem hiding this comment.
Instead of setting the field to null and handling that case in all of the methods, it would be easier to just have a private class for the empty marker singleton.
There was a problem hiding this comment.
Yes, that sounds better. I didn't know if this was the only use of EmptyActionInput or if some Google code makes real execpath empty inputs.
There was a problem hiding this comment.
There is one internal use of EmptyActionInput that also passes /dev/null. I suggest keeping this class as it was for now and just create a new VirtualActionInput implementation class for the "null" case. I will separately see if the internal use case can be migrated similarly and if so follow up with a change to delete this class.
There was a problem hiding this comment.
Okay, reverting to the original version of the PR now...
benjaminp
force-pushed
the
empty-virtual
branch
2 times, most recently
from
8ba61cb to
507a47e
Compare
benjaminp
force-pushed
the
empty-virtual
branch
from
507a47e to
8ba61cb
Compare
SpawnInputExpander.EMPTY_FILE claimed an execpath of /dev/null, which could lead to clients trying to do mutating filesystem operations in /dev. In this change, SpawnInputExpander.EMPTY_FILE is replaced with an EmptyVirtualInput with no execpath. This way errant callers will explode before trying filesystem operations on it. Such a EmptyVirtualInput doesn't really meet the contract of being an ActionInput but that was really already true.
benjaminp
force-pushed
the
empty-virtual
branch
from
8ba61cb to
ebe480c
Compare
|
@justinhorvitz sending you the import CL. I'm on triage duty this week and trying to clean the backlog of stale PRs. |
|
Had to roll this back. I'm working on relanding it. 0f626a4 |
SpawnInputExpander.EMPTY_FILE claimed an execpath of /dev/null, which could lead to clients trying to do mutating filesystem operations in /dev. In this change, SpawnInputExpander.EMPTY_FILE is replaced with an EmptyVirtualInput with no execpath. This way errant callers will explode before trying filesystem operations on it. Such a EmptyVirtualInput doesn't really meet the contract of being an ActionInput but that was really already true. Closes #12516. Roll forward of ceec93c, which was rolled back in 0f626a4 RELNOTES: None. PiperOrigin-RevId: 351367453
|
Thank you for sorting that out. |
|
You're welcome. I forgot, however, to munge the re-landing to give the attribution to you. Sorry about that. It's a pity the tooling does not do this automatically. |
SpawnInputExpander.EMPTY_FILE claimed an execpath of /dev/null, which could lead to clients trying to do mutating filesystem operations in /dev. In this change, SpawnInputExpander.EMPTY_FILE is replaced with an EmptyVirtualInput with no execpath. This way errant callers will explode before trying filesystem operations on it. Such a EmptyVirtualInput doesn't really meet the contract of being an ActionInput but that was really already true. Closes bazelbuild#12516. Roll forward of ceec93c, which was rolled back in 0f626a4 RELNOTES: None. PiperOrigin-RevId: 351367453
SpawnInputExpander.EMPTY_FILE claimed an execpath of /dev/null, which could lead to clients trying to do mutating filesystem operations in /dev. In this change, SpawnInputExpander.EMPTY_FILE is replaced with an EmptyVirtualInput with no execpath. This way errant callers will explode before trying filesystem operations on it. Such a EmptyVirtualInput doesn't really meet the contract of being an ActionInput but that was really already true. Closes bazelbuild#12516. Roll forward of ceec93c, which was rolled back in 0f626a4 RELNOTES: None. PiperOrigin-RevId: 351367453
SpawnInputExpander.EMPTY_FILE claimed an execpath of /dev/null, which could lead to clients trying to do mutating filesystem operations in /dev.
In this change, SpawnInputExpander.EMPTY_FILE is replaced with an EmptyVirtualInput with no execpath. This way errant callers will explode before trying filesystem operations on it. Such a EmptyVirtualInput doesn't really meet the contract of being an ActionInput but that was really already true.