Skip to content

Fix r+ permission writeable check #77

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.

Sign up for GitHub

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

Closed
yurganov wants to merge 2 commits into from
Closed

Fix r+ permission writeable check #77

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9a4333
Fix r+ permission writeable check
yurganov Aug 9, 2023
5908dc9
fix check writable for rb+ and r+b
yurganov Aug 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Internal/QueuedWritesFile.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public function __construct(
}

$this->queue = new \SplQueue();
$this->writable = $this->mode[0] !== 'r';
$this->writable = $this->mode[0] !== 'r' || $this->mode == 'r+';
Copy link
Member

@kelunik kelunik Aug 9, 2023

Choose a reason for hiding this comment

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

This likely needs normalization like in other places and can then just check for $this->mode !== 'r'?

See $mode = \str_replace(['b', 't', 'e'], '', $mode); in other places.

Copy link
Author

@yurganov yurganov Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

file/src/functions.php

Line 78 in efeb737

function openFile(string $path, string $mode): File

https://github.com/danog/MadelineProto/blob/0689c16309a8a404f920f5e5543b64ad8a3309b2/src/MTProtoTools/FilesLogic.php#L360

The openFile is amphp function. Maybe I am wrong, but AS I can understand, IF it possible to create new file with amphp library with "r+" permission, so the library will do the rest -> create it, write it, ...

If I am wrong could you please explain to me why. Thanks!

Copy link
Member

@kelunik kelunik Aug 9, 2023

Choose a reason for hiding this comment

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

@yurganov I think you didn't understand correctly: The new check is better than the old one, but still potentially incorrect, e.g. for rb+ (at least I think that works). So it should normalize the mode with the mentioned function and then check just for !== 'r', as that's the only mode that is non-writable.

Copy link
Author

@yurganov yurganov Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @kelunik, absolutely, I overlooked the 'rb' option. I believe it's simpler to validate only for 'r' and 'rb. Please check my commit.

Copy link
Member

@kelunik kelunik Aug 11, 2023

Choose a reason for hiding this comment

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

@yurganov I still think we should remove b, t, and e from the mode, as they're just flags and that can be present in any order IIRC.

$this->position = $this->mode[0] === 'a' ? $this->size : 0;
}

Expand Down