- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 23.5k
 
Fix FileAccess::open returning valid FileAccess when opening a pack file fails #112330
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
base: master
Are you sure you want to change the base?
Conversation
94af86f    to
    1bfc104      
    Compare
  
    | if (err != OK) { | ||
| ret.unref(); | ||
| } | ||
| return ret; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (err != OK) { | |
| ret.unref(); | |
| } | |
| return ret; | |
| if (err == OK) { | |
| return ret; | |
| } | 
It should behave the same as if nullptr is returned, and try to fall back to raw file outside pack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this is correct behavior. If PackedData::get_singleton()->try_open_path returns a FileAccess object, that means that the path is in PackedData and thus the canonical file location is in the pack. If trying to open that file in the pack returned an error, it means that there's something catastrophically wrong with our environment (wrong encryption key, the pck got deleted in the middle of running, etc.) and we should fail here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should at least be consistent, so maybe:
- check if pack has file using 
PackedData::get_singleton()->has_path- if found, try loading and fail if 
try_open_pathreturnnullptror returned file has error set. - if not found, try using filesystem.
 
 - if found, try loading and fail if 
 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or get_error check can be moved to the try_open_path and it can return nullptr if error is set (to avoid double file map lookup).
FileAccess::open currently returns valid FileAccess files if the packed file has an error while opening (e.g. if the encrypted file fails to be decrypted) because it is currently not checking
get_error()before returning. With FileAccessPack in particular, this can lead to buffer overreads due to reading parts of the PCK that aren't valid data.This changes FileAccessPack to return valid errors with
get_error()if there were errors opening the file, and changesFileAccess::opento checkget_error()before returning.