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

Fix Crash on permission failure #2906

Merged
merged 1 commit into from
Mar 7, 2021
Merged

Conversation

Febbe
Copy link
Collaborator

@Febbe Febbe commented Mar 4, 2021

  • replaced fs::equivalent with 2 more robust calls to fs::weakly_canonical
  • catches an error on failure.

@Febbe
Copy link
Collaborator Author

Febbe commented Mar 4, 2021

will merge in 24h if pipeline succeeds and if there are no objections
@Technius @rolandlo

Copy link
Member

@Technius Technius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on where this "permission failure" is coming from?

src/control/jobs/BaseExportJob.cpp Outdated Show resolved Hide resolved
@rolandlo
Copy link
Member

rolandlo commented Mar 5, 2021

Will this fix #2823 (comment)?

@Febbe
Copy link
Collaborator Author

Febbe commented Mar 5, 2021

Will this fix #2823 (comment)?

Very likely. I still don't know when and why this happens. For me, it seems like a permission failure. But the error message (unsupported operation) was not very specific, and it occurred in 90% of all cases. So I just decided to use a similar approach which is a lot more safe, but a bit more expensive. Since it is not in a loop, this is fine.

@createyourpersonalaccount
Copy link
Contributor

@Febbe maybe it has to do with SIP protection? https://support.apple.com/en-us/HT204899
Here's a github issue from a different project that may be related to what was happening: gulrak/filesystem#77

@Technius
Copy link
Member

Technius commented Mar 7, 2021

We actually use that library for file system handling on Mac, but we're too close to our next release to bump up a dependency version and risk regressions. I propose just having this band-aid fix for now and bumping the library version after we're done with the 1.1.0 release.

Edit: we are using the ghc::filesystem version that supposedly includes the fix for the linked issue.

@Febbe
Copy link
Collaborator Author

Febbe commented Mar 7, 2021

It's also a problem on linux mint 20(Ubuntu 20.04), dunno if this happens on mac.
So the gulrak impl is not the cause here.

std::filesystem::equivalent throws when one of both filepaths have the filestatus not_found, despite the fact, that std::filesystem::equivalent also compares the filestatus.

There is also a defect report in the documentation to that: https://cplusplus.github.io/LWG/issue2937

@gaperez64
Copy link

Will this fix #2823 (comment)?

Very likely. I still don't know when and why this happens. For me, it seems like a permission failure. But the error message (unsupported operation) was not very specific, and it occurred in 90% of all cases. So I just decided to use a similar approach which is a lot more safe, but a bit more expensive. Since it is not in a loop, this is fine.

I can confirm that with the change to fs::weakly_canonical I am unable to reproduce the error. To be precise, the following code does not give the error anymore:

#include <iostream>
#include <cstdint>
#include <filesystem>
#include <cassert>

namespace fs = std::filesystem;

int main(int argc, char* argv[])
{
    assert(argc >= 3); 
    // hard link equivalency
    fs::path p1 = argv[1];
    fs::path p2 = argv[2];
    // if (fs::equivalent(p1, p2)) {
    if (fs::weakly_canonical(p1) == fs::weakly_canonical(p2)) {
        std::cout << p1 << " is equivalent to " << p2 << '\n';
        return 1;
    } else {
        std::cout << p1 << " is NOT equivalent to " << p2 << '\n';
        return 0;
    }   
}

 - replaced fs::equivalent with 2 more robust calls to weakly_canonical
 - catched an error on failure.
@Febbe Febbe force-pushed the fix_pdfexport_crash branch from 5aa312a to c643db5 Compare March 7, 2021 16:33
@Febbe Febbe merged commit 538235d into xournalpp:master Mar 7, 2021
@Febbe Febbe deleted the fix_pdfexport_crash branch November 27, 2021 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants