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

Return err in copy() fstat cb, because stat could be undefined or null #38

Merged
merged 1 commit into from
Oct 24, 2013
Merged

Conversation

ulikoehler
Copy link
Contributor

I had a problem with FlyLaTeX, where some weird arguments (notexistent files etc) were passed to copy() --> err is ENOENT, but the copy() code failed to properly terminate the lstat callback when err is not false-ish, resulting in the whole application terminating with this stacktrace:

/var/lib/nodejs/node-apps/flylatex/node_modules/fs-extra/lib/copy.js:41
    if (stats.isDirectory()) {

This PR fixes this by not only calling callback(err) but instead returning it. I didn't check other functions or files for similar problems. Please feel free to comment if you think this should be fixed otherwise or see possible side-effects with this fix.

@jprichardson
Copy link
Owner

Would it be possible for you to add a regression test? i.e. a test demonstrating a fail condition before the patch, and then when your patch is applied, the test passes?

@ulikoehler
Copy link
Contributor Author

Sure! I'll submit another pull request to be able to see the difference easily.

@jprichardson
Copy link
Owner

Excellent! Thanks. I should be able to review it this evening or by tomorrow morning at the latest.

@ulikoehler
Copy link
Contributor Author

Sure, take your time. I suppose flylatex won't upgrade to the newest fs-extra version anyway, so there's no need to hurry!

When I have some time, I'll check if there could be a similar error in functions other than copy().

@ulikoehler
Copy link
Contributor Author

It seems like this was the only place where the code didn't return when err is not false-ish.

@jprichardson
Copy link
Owner

Cool, good to know :) Thanks again.

jprichardson added a commit that referenced this pull request Oct 24, 2013
Return err in copy() fstat cb, because stat could be undefined or null
@jprichardson jprichardson merged commit cfdf243 into jprichardson:master Oct 24, 2013
@ulikoehler
Copy link
Contributor Author

Thanks for merging ;-) Sorry if the report was a bit confusing, I was really tired at that point, after debugging FlyLaTeX for hours :)

ovr pushed a commit to ovr/node-fs-extra that referenced this pull request Nov 12, 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