utime/futimes can only be used on files that you own

[phated]

This was pointed out in gulpjs/gulp#1376 and it is documented in the manpage of utime (http://man7.org/linux/man-pages/man2/utimes.2.html). We should check the file owner before trying to call futimes on it.

@piranna @erikkemperman

[piranna]

sh_t... I know it's not my fault, but I must to admit I felt guilty... :-P Yeah, I'll check the owner before trying to change the utimes, but as you have said on the other issue, this should *not_ be a problem since the files should be created with the correct owner... :-/

[ilazaridis]

Thank you. 👍

[phated]

@piranna it seems that we inherit the owner and stuff from the src files (or so it is documented).

[piranna]

@piranna it seems that we inherit the owner and stuff from the src files (or so it is documented).

Via the uid and gid fields, isn't it? Makes sense... Anyway this check should be done to prevent failures like the one on the gulp issue, isn't it?

[phated]

@piranna yeah, we need to check the owner to avoid that issue.

[piranna]

By the way, why the change from utime to futimes? Have they fixed the sub-second precision only on the fd* functions?

[phated]

@piranna yep, because it uses futimes underneath which has sub-second precision on filesystems that support it.

[erikkemperman]

@piranna There was talk here about making fs.utimes use the utimes system call in stead of utime. As the manpage linked above show, one supports sub-second precision and the other doesn't. So yeah, that's why I proposed that change.

I think this ownership issue is orthogonal to futimes vs utimes though.

[piranna]

I think this ownership issue is orthogonal to utimes vs utime though.

Yeah, definitely, I was just curious about that thing. Absolutely +1 to use utimes :-) Only I was a bit surprised about the usage of file description instead of path functions, no more.

[erikkemperman]

It would be good if node would use the utimes syscall consistently. But that issue is already a year old and has no follow-up as far as I can tell. And even it were fixed today, it would take some time before a lot of node users actually benefit from it. So for the moment we're going to have to deal with filedescriptors where it would otherwise not be necessary.

[piranna]

I've found the change was easy:

if(stat.uid !== process.getuid() || !(stat.mode & S_IWUSR)) {
  return cb();
}

Problem is that the mtime tests are failling because we are only setting the mtime and atime fields there:

    var expectedFile = new File({
      base: inputBase,
      cwd: __dirname,
      path: inputPath,
      contents: expectedContents,
      stat: {
        mtime: expectedMtime
      }
    });

We have here two options, add the missing fields on stat for the tests (uid and mode), or add them on the File object as it was proposed before and as how recent Node.js versions doing. What do you think?

[phated]

The check could also check for null/undefined because if we aren't setting it, the owner will be the user running the process.

[piranna]

Good idea! I'll do that :-)

[piranna]

Ok, I've done a pull-request to fix this :-)

[erikkemperman]

@piranna @phated @ounos
I'm not sure I understand this issue... I assumed that when ounos wrote

I am not the owner of the file, but i am in the same group. File permissions 777.

he's talking about a pre-existing file which he wants dest to overwrite. At least, that's the situation in which I get an EPERM error out of futimes (or for that matter utimes in the old situation).

But the submitted PR is checking the stat on the source file, not the destination... So if I apply the proposed patch the issue, as I interpreted it, persists and is still throwing EPERM. What am I missing here?

It seems to me that fs.(f)utimes is being too strict. We're being allowed to overwrite a file but not change its atime and mtime. I am certainly able to touch the file, for instance, as the same user whose gulp process gets an error out of fs.futimes.

[piranna]

@erikkemperman, fs.(f)utimes is too strict because you are changing the times of the file, so you are making it "lie" about when it was created or accessed. In files that you don't own this can be a security issue, because you could be able to read a confidential file and later change its access time so nobody would know you've read it, definitely a bad issue. That's the reason why you can be able to touch it when you are not the owner but have read permission, but you need to be the owner and have write permission over it to change its times.

Regarding to this thing and due to you post about the usage of the stat of the original file, I think you are right about the use case of updating current files instead of creating new ones. Creating new ones will (always?) be done with the uid (and gid, maybe?) of the current process, so it doesn't matter the ones of the original file except when modifying them, so maybe a better alternative would be to just try the call to fs.(f)utimes inside a try-catch and if it throw an EPERM error (or maybe in all the exceptions?) ignore it.

What do you think?

[erikkemperman]

That's the reason why you can be able to touch it when you are not the owner but have read permission, but you need to be the owner and have write permission over it to change its times.

Hm, my first thought here was: how is changing atime/mtime via touch any different from changing them via (f)utimes, from the point of view of the fs?

But now I think I get what you mean, touch with no arguments (set atime/mtime to current time) might require different privileges than touch -A <time> (set atime/mtime to an arbitrary time). And the latter is what we're trying to do with our futimes call.

I did not know about that, but yes I see how the difference might be relevant in terms of security implications.

Interestingly, I find different manpages on mac and linux, to the point where I think they're saying contradictory things about the case where times is not NULL and the process has the appropriate privileges but is not the owner or super-user.

Linux:

If times is NULL, then the access and modification times of the file
are set to the current time.
Changing timestamps is permitted when: either the process has
appropriate privileges, or the effective user ID equals the user ID
of the file, or times is NULL and the process has write permission
for the file.

Mac:

If times is NULL, the access and modification times are set to the cur-
rent time. The caller must be the owner of the file, have permission to
write the file, or be the super-user.
If times is non-NULL, it is assumed to point to an array of two timeval
structures. The access time is set to the value of the first element,
and the modification time is set to the value of the second element. The
caller must be the owner of the file or be the super-user.

If I parse this correctly, and my interpretation of @ounos's issue is right, then it wouldn't have happened on linux? What does windows say about all this?

In any event, I'd agree that instead of trying to predict if it might happen, it would be better to just attempt the call and catch this particular error -- that is, if we insist on suppressing it, which I'm not sure is the best course of action tbh.

Suppose the destination file is an intermediate file, which serves as source in a subsequent pipeline. If that second pipe uses something like gulp-newer things will still go wrong, only quietly so.

Personally I think I'd like to know about this failing, because it can lead to problems which would be annoying to track down. So I might argue against suppressing the symptom whilst not addressing the illness. Does that make sense?

[erikkemperman]

The plot thickens. When this EPERM error happens in fs.futimes, we'll have already overwritten the contents. So the file will have its last-modified time updated (but not its last-access time). So the scenario I worried about, with something like gulp-newer, might turn out fine.

$ touch output.txt 
$ ls -lc output.txt 
-rwxrwxrwx  1 root  staff  5 Nov  7 13:44 output.txt
$ ls -lu output.txt 
-rwxrwxrwx  1 root  staff  5 Nov  7 13:44 output.txt
$ gulp access
[13:45:11] Starting 'access'...
[13:45:11] 'access' errored after 33 ms
[13:45:11] Error: EPERM: operation not permitted, futime
    at Error (native)
$ ls -lc output.txt 
-rwxrwxrwx  1 root  staff  5 Nov  7 13:45 output.txt
$ ls -lu output.txt 
-rwxrwxrwx  1 root  staff  5 Nov  7 13:44 output.txt

[piranna]

I find different manpages on mac and linux

MacOS X is based on BSD so it's normal there're differences, but they are mostly harmless.

What does windows say about all this?

Don't see the evil, don't listen the evil, don't tell the evil ;-)

[erikkemperman]

Much as I'd like to, we probably can't ignore the evil entirely.

[ilazaridis]

@erikkemperman I am talking about a file that the first time I call dest is created and any other time is overwritten.
I am surprised that this issue comes up now. I mean the scenario here is really simple (and I guess common): a mercurial repository accessed by a team, re-generating assets (css/js) automatically on update using gulp.

[erikkemperman]

@ounos I guess I'm still not getting it :-) If the file is originally created by dest, how did it wind up being owned by someone else -- do you explicitly set stat.uid or something? Or isn't it the overwrite, after all, that gives you the EPERM error?

[ilazaridis]

@erikkemperman ok, let me explain the workflow here:
There is a mercurial project, me and my team work on. Using a hg update hook, I call gulp which combines less files and generates a css file. As you can understand, gulp can be triggered by any team member (when they push their changes). Maybe I have misunderstood sth and I should find a different way.
The point is that we have, lets say, a multiuser environment with access to the same file via gulp.
It's definitely the overwrite, I played around with gulp and file ownership.

[erikkemperman]

@ounos
Ah, okay, so the less files are owned by whomever checked out the repository. And assuming that the css files aren't versioned (because they shouldn't be) they will be owned by whomever ran gulp for the first time.

And then the other users in the same group ought to be able to re-run gulp and overwrite those css files. This should work because you made sure of mode 0777. And it does, as far as the contents of the file goes -- except for the time attributes, because setting those throws an error.

Does that sound about right? Which OS/filesystem is this?

I'm not sure how common it is to share a local working copy like that. But I suppose you could sidestep this whole issue by inventing a dedicated project-user and having the group do su project before checking out code and (re)running gulp.

[ilazaridis]

Ah, okay, so the less files are owned by whomever checked out the repository. And assuming that the css files aren't versioned (because they shouldn't be) they will be owned by whomever ran gulp for the first time.
And then the other users in the same group ought to be able to re-run gulp and overwrite those css files. This should work because you made sure of mode 0777. And it does, as far as the contents of the file goes -- except for the time attributes, because setting those throws an error.
Does that sound about right? Which OS/filesystem is this?

css file is generated on the web server, when a changeset is pushed to it (this way we can check changes on a dev web server on the fly). Except that, everything else is right. It's GNU/Linux (OpenSUSE), I am not sure about the filesystem, but I guess typical ext3 or ext4.
But, now I realise that the css file is generated correctly, right ? The problem is only with the file metadata ? I thought that the file was not overwritten at all.

I'm not sure how common it is to share a local working copy like that. But I suppose you could sidestep this whole issue by inventing a dedicated project-user and having the group do su project before checking out code and (re)running gulp.

As I wrote above, it's not a local copy, it's a css file generated on a remote machine. Yes, ofc I could do sth like that, but to be honest I am not excited with the idea.

I am sorry, if I confused you and thanks for your time.

[piranna]

I think you should remove the CSS files altogether from the repo, and just
generate them when publishing to the web server on its own local copy,
mostly as you are doing on your local machine. This is the way how works
all the PaaS that I know like Heroku or 5Apps or algo GitHub Pages that
allow to publish by using git commands.
El 07/11/2015 19:43, "Ioannis Lazaridis" [email protected]
escribió:

Ah, okay, so the less files are owned by whomever checked out the
repository. And assuming that the css files aren't versioned (because they
shouldn't be) they will be owned by whomever ran gulp for the first time.
And then the other users in the same group ought to be able to re-run gulp
and overwrite those css files. This should work because you made sure of
mode 0777. And it does, as far as the contents of the file goes -- except
for the time attributes, because setting those throws an error.
Does that sound about right? Which OS/filesystem is this?

css file is generated on the mercurial remote repo, when a changeset is
pushed to it (this way we can check changes on a dev web server on the
fly). Except that, everything else is right. It's GNU/Linux (OpenSUSE), I
am not sure about the filesystem, but I guess typical ext3 or ext4.
But, now I realise that the css files is generated correctly, right ? The
problem is only with the file metadata ? I thought that the file was not
generated/overwritten at all.

I'm not sure how common it is to share a local working copy like that. But
I suppose you could sidestep this whole issue by inventing a dedicated
project-user and having the group do su project before checking out code
and (re)running gulp.

As I wrote above, it's not a local copy, it's a css file generated on a
remote machine. Yes, ofc I could do sth like that, but to be honest I am
not excited with the idea.

—
Reply to this email directly or view it on GitHub
#127 (comment).

[ilazaridis]

@piranna css file is not included in the repo. As you said I just generate it when pushing changes to the web server. I updated my previous comment. My fault.

[piranna]

👍 sorry, I misunderstood it. Are you having the error on the CSS files on the remote machine, or in local?

[ilazaridis]

@erikkemperman this will work for now, but If add js file generation too, it means that on first error (css file) gulp will die and exit, meaning js file won't be generated (or what comes first), right ?
Buh, I probably should find another solution here.

[erikkemperman]

@ounos you're using gulp 4, right? I think that if you run tasks in parallel then an error in one won't interfere with the others. But only because they'll already have been started and there's no abort mechanism afaik.

I probably sound like a broken record, sorry about that, but the workaround I suggested still strikes me as rather painless, and it ought to preempt this whole mess. Like I said above, I think this error is not necessarily something vinyl-fs should be sweeping under the rug. But don't worry, it's not up to me :-)

[phated]

@erikkemperman I don't think this is something that someone needs to work around. The mtime/atime stuff was recently added for an edge case and we weren't breaking before that. If you go look at the manpages again for utime, it confirms that EPERM happens when the owner of the file is not the one calling the method. In vfs, we use the original owner of the src file to create the new file. As far as I can tell, @ounos is seeing on the production environment that the CSS files being generated as the user that created the LESS files and then the user (probably some automated process) running the gulpfile isn't that user that those CSS files were created as. If we compare the user to the one on the source files, everything should begin to work again.

CC @piranna

[phated]

@erikkemperman @piranna we should probably use https://www.npmjs.com/package/fs-access to check access before attempting to set utimes.

[phated]

^ although, we'd probably have to help with sindresorhus/fs-access#1 😛

[phated]

I can't actually find a call to fs.chown() so maybe we aren't creating files with the same user that created the src files...

[piranna]

On Linux, chown requires to be used by root due to similar security issues
as the ones of utimes ("me?!? Oh no, look! That file saying willing to
destroy all humans is of Fry, not me Bender, the friend of the humans!!"
:-P)
El 08/11/2015 19:40, "Blaine Bublitz" [email protected] escribió:

I can't actually find a call to fs.chown() so maybe we aren't creating
files with the same user that created the src files...

—
Reply to this email directly or view it on GitHub
#127 (comment).

[erikkemperman]

@phated As I understand it the less files are owned by the user that checked out or last updated that file from vcs. The destination css will be owned by the user that ran gulp when it is first created -- not necessarily the owner of the source, although in the case of a hg hook it likely is.

I can't actually find a call to fs.chown() so maybe we aren't creating files with the same user that created the src files...

Right, I think new files are owned by the gulp user, not the src owner. Subsequent attempts by different users running gulp to overwrite the css then trigger this error, and I confirmed that will still be the case after something like the proposed patch were applied. Because it doesn't test ownership of the destination, nor would it be allowed to change that ownership in this circumstance.

So if updating the time attributes on disk is really only an optional bonus and dest having different behavior in this particular case of shared destination files is not considered problematic... Yep, this error should be suppressed. Then I think the test needs work, fs-access looks handy. Or else just attempt the call and catch the EPERM.

I can't find it just now but someone pointed out that pipes attached to dest should always get the same metadata, exactly as if that file were just srcd. If this error is hidden that won't be true, unless time attributes are exempted from this principle. Though I suppose times could even be "reverse- synced" to reflect the fs. Ugh.

[piranna]

The more we disscus this, the more I think it should not be fixed... :-/
El 08/11/2015 21:18, "Erik Kemperman" [email protected] escribió:

@phated https://github.com/phated As I understand it the less files are
owned by the user that checked out or last updated that file from vcs. The
destination css will be owned by the user that ran gulp when it is first
created -- not necessarily the owner of the source, although in the case of
a hg hook it likely is.

I can't actually find a call to fs.chown() so maybe we aren't creating
files with the same user that created the src files...

Right, I think new files are owned by the gulp user, not the src owner.
Subsequent attempts by different users running gulp to overwrite the css
then trigger this error, and I confirmed that will still be the case after
something like the proposed patch were applied. Because it doesn't test
ownership of the destination, nor would it be allowed to change that
ownership in this circumstance.

So if updating the time attributes on disk is really only an optional
bonus and dest having different behavior in this particular case of
shared destination files is not considered problematic... Yep, this error
should be suppressed. Then I think the test needs work, fs-access looks
handy. Or else just attempt the call and catch the EPERM.

I can't find it just now but someone pointed out that pipes attached to
dest should always get the same metadata, exactly as if that file were
just srcd. If this error is hidden that won't be true, unless time
attributes are exempted from this principle. Though I suppose times could
even be "reverse- synced" to reflect the fs. Ugh.

—
Reply to this email directly or view it on GitHub
#127 (comment).

[erikkemperman]

If you go look at the manpages again for utime, it confirms that EPERM happens when the owner of the file is not the one calling the method

Ah yes indeed, the errors section is clear where the general description (imho) isn't. So uniform across Mac/Linux then. Don't speak the evil :-)

[erikkemperman]

Final thought on this -- I spent way too many words on it already :-)

So the utimes/futimes call is only there because of this issue? Maybe it is causing more headaches than it cures...

[piranna]

So the utimes/futimes call is only there because of this issue? Maybe it
is causing more headaches than it cures...

Yes, utimes is there due to that issue, that maked tar files get badly
uncompressed. I understand the problem here with the file permissions is
due to a bad usage of gulp (or at least not being used in its expected way)
and not a problem with the data on the vinyl pipeline, so I would close
this pull-request and left things as they are at this moment.

[erikkemperman]

so I would close this pull-request and left things as they are at this moment.

@piranna Yes that was my feeling too. Until @phated pointed out that this particular ownership issue never came up in the situation before (f)utimes was added... This is because setting mtime/atime to arbitrary stamps requires more privileges than setting mtime to "now", as happens (implicitly) in overwriting the file. And come to think of it, the issue I had with milliseconds earlier would also not have come up in the old situation.

[piranna]

Oooookey... now I see what's the problem here: we have two valid and somewhat contradictory use cases:

• create new files, that should be created with the given mtime and atime (my use case)
• update current files, that should set the current time instead of the given one (@ounos use case)

I've re-read the man page of futimes and the EPERM error only happens when setting the utimes of a file that we don't own, no more. This only could happen for current files, so I've updated the pull-request to ignore it, so it will allow to set the utimes in newly created files (since we own them) and will not raise error when updating current files.

This has only a drawback, that's the utimes will be updated on current files that we own so they would not appear as modified, but don't think it would be too much a problem. I've been looking on the source code for any other indicator like the open flag, but files are always open with w or wx so it's not possible to identify.

[erikkemperman]

@piranna I have no skin in this game, as they say, but as a user I would probably be annoyed to find different behaviour for write and overwrite. Both use cases seem to me to have fairly trivial workarounds, but that's easy for me to say because I'm not the one having to apply them. Anyway, for what it's worth, I might now lean toward yanking the (f)utimes call entirely, in all cases.

[piranna]

Well, at this moment we have already a different behaviour for write and overwrite when setting the file mode, since Node.js fs.open() ignore it on overwrite :-D :

mode sets the file mode (permission and sticky bits), but only if the file was created. It defaults to 0666, readable and writeable.

[erikkemperman]

True, no chmod is done for extant files. And attempting it would fail in the case of a different owner, just like futimes would.

Even so, wouldn't it make sense to remove the whole futimes call from vinyl-fs? Sticking to the old way seems safer, @ounos might just be one of several use-cases that stumble over this?

A separate plugin gulp-touch or something like that could handle your use-case, i.e. forcing atime and mtime upon a file. Either from the src file metadata, or given as options to the plugin. That plugin would of course require ownership. Writing it ought to be easy.

[piranna]

In my case it's not so easy because I'm not using vinyl-fs directly, but instead it's a dependency of decompress-tar, at the same time a dependency of decompress, of download and finally from download-manager... too much levels to fiddle around. I could try to call to futimes() on decompress-tar, but seems not easy since it delegates all the work with files to vinyl... :-/

[erikkemperman]

Ah, I see. I still think that if futimes is called by vfs, the default response to an error should be to throw it. Maybe an option to dest to ignore it; at least that would be conscious decision by the caller.

Like I said though it doesn't really matter for my little use case, my merged PR takes care of the issue I had with milliseconds.

I'm going to sign off here, sorry about ludicrous verbosity :-)

[phated]

@erikkemperman it's important for us to discuss these things so I'm glad you've been involved on this issue.

The ethos of vinyl-fs is that this library should be doing all the filesystem operations (it is against gulp plugin guidelines to use the fs module directly. I think we need to figure out the proper way to handle this in dest. The gulp-touch plugin you suggested got me thinking about this a bunch and what that plugin would do is set the atime/mtime on the vinyl object and then we would have to set it on the file.

[erikkemperman]

@phated Perhaps this is really just one instance (futimes) of a broader question (chmod, chown). If so it might warrant a separate issue and broader discussion.

But I can't resist an attempt to summarize what I think all of the above amounts to.

The fact that futimes got into master, and that you were searching for a chown somewhere, suggests to me that ideally dest would write all (set) stat attributes of the vinyl object to the fs. The problem is that these operations require additional privileges, compared to the principal act of (over)writing a file.

This issue of @ounos and the earlier one of @piranna demonstrate that different use-cases have different expectations / wishes / demands about what dest does if it manages to write the file, but not its metadata.

piranna's case broke before the (f)utimes was merged because the atime/mtime were not written to disk, ounus' case broke because of insufficient privileges for (f)utimes, even though he doesn't care about the atime/mtime on disk (or rather is willing to accept this compromise for multi-user case).

I might have extrapolated too much from discussion elsewhere, but I like the thought that when the pipeline doesn't end with dest, the vinyl object it pushes through would ideally reflect all attributes from the fs. In other words, there should be no difference between piping onto dest or creating a new src on the file that dest wrote -- not just in terms of file contents but also metadata like uid, mode, mtime.

This would imply that if writing a piece of metadata fails, and we don't want to consider that a "hard" error, we might want to update that bit on the vinyl object from disk instead of vice versa. That might be radical though, and I can't decide if this is philosophically sound enough to justify its technical ugliness :-)

[Delagen]

+1 on windows gulp-bump does not update package.json

[yocontra]

If we aren't able to resolve this soon, I think the best bet is to revert all of the stat changes that introduced breaking changes until we are able to do it properly.

[phated]

This has finally been resolved by #151 and published as 2.3.2. As per the new .dest() documentation:

Once the file is written to disk, an attempt is made to determine if the stat.mode, stat.mtime and stat.atime of the vinyl File object differ from the file on the filesystem. If they differ and the running process owns the file, the corresponding filesystem metadata is updated. If they don't differ or the process doesn't own the file, the attempt is skipped silently. This functionality is disabled on Windows operating systems or any other OS that doesn't support process.getuid or process.geteuid in node. This is due to Windows having very unexpected results through usage of fs.fchmod and fs.futimes.

[phated]

Oh, and please update your installations to get the fixes! Cheers

[erikkemperman]

Thanks @phated !

[piranna]

Yeah, thank you! I'll try to find time to test it :-)
El 26/2/2016 7:19 AM, "Erik Kemperman" [email protected] escribió:

Thanks @phated https://github.com/phated !

—
Reply to this email directly or view it on GitHub
#127 (comment).