Opened 14 years ago

Closed 14 years ago

#5521 closed enhancement (fixed)

StyledEdit does not check read-only status of a file prior to writing to it.

Reported by: ver Owned by: korli
Priority: low Milestone: R1
Component: Applications/StyledEdit Version: R1/alpha1
Keywords: stylededit save read-only Cc: ver@…
Blocked By: Blocking:
Platform: All

Description

This is easy to demonstrate...

Create a file, set it to read-only by changing the file's mode to exclude any write bits, and then open it in StyledEdit. Change the file contents, and save it. No warning is produced. I don't believe this is expected behaviour as Pe and Vi (for example) both warn prior to writing to a read-only file.

I have included a patch which relies on #5520 to work, but will compile and produce indifferent results otherwise.

Attachments (1)

stylededit_stylededitwindow-iswritable.patch (1.1 KB ) - added by ver 14 years ago.
patch against svn 35718 (mar 2, 2010)

Download all attachments as: .zip

Change History (8)

by ver, 14 years ago

patch against svn 35718 (mar 2, 2010)

comment:1 by ver, 14 years ago

Cc: ver@… added

comment:2 by axeld, 14 years ago

Blocked By: 5520 removed

Besides that #5520 has been marked as invalid, your patch also contains a number of style violations - we do have a coding style and require all of our own sources to comply to it.

Anyway, StyledEdit obviously issues no warning, as writing actually succeeds (since you are the root user), so the file is writable after all. I guess it would make sense to check for the writable bit being set; so while this patch isn't adequate, I'm keeping the bug open.

BTW: Instead of fstat(), there is also a BFile::GetStat() inherited from BStatable.

comment:3 by ver, 14 years ago

Blocked By: 5520 added

(In #5520) Replying to axeld:

These changes make no sense - if a file is not writable for you, you cannot open it read/write anyway, hence the additional check is superfluous.

Why would you set a file mode to r-x if you never check the w? What is the point of IsWritable() otherwise? To tell me I passed THIS flag when I called my BFile constructor? :/

Furthermore, we cannot change original Be API, since we aim for binary compatibility (ie. if we would actually change such a method, we'd need to use symbol versioning in order to make it still work as expected for legacy applications).

Meh, alright, I wasn't aware it would break binary compatibility. So, would the solution be to effectively move this check directly into StyledEdit code, in that case?

comment:4 by axeld, 14 years ago

Blocked By: 5520 removed

(In #5520) > Meh, alright, I wasn't aware it would break binary compatibility.

It breaks compatibility on a functional level - if an application is using IsWritable(), it will not work as expected all of a sudden anymore.

So, would the solution be to effectively move this check directly into StyledEdit code, in that case?

Yes, indeed, I'm afraid that would be the solution.

comment:5 by ver, 14 years ago

Alright, I'm not sure which fstat you are referring to, but I apologise for the formatting. I copied majority of it from elsewhere (Pe as well as earlier in StyledEdit's source) so I assumed it was style-conforming. What would the best approach to fixing this be? Is checking stat flags and producing a warning in the case of a missing writeable bit generally acceptable?

comment:6 by axeld, 14 years ago

I meant the fstat() as mentioned in #5520. And yes, checking the writable bit should be generally acceptable, at least if they indicate you may actually not want to write to the file.

comment:7 by korli, 14 years ago

Resolution: fixed
Status: newclosed

Fixed in hrev36114.

Note: See TracTickets for help on using tickets.