Hi Anatolij
First of all - many thanks for the compliment - you seem to think, that
I'm able to conduct a meaningful communication after an almost 2 year
break ;-) Anyway, I think, it would be best to accept your patches, since
there don't seem to be too many users of this project;-) Just one more
idea:
On Wed, 8 Feb 2012, Anatolij Gustschin wrote:
> Hi Guennadi,
>
> On Wed, 14 Apr 2010 18:14:37 +0200 (CEST)
> Guennadi Liakhovetski <g.liakhovetski@xxxxxx> wrote:
>
> > Looks good in general, some comments below:
>
> Thanks for review and comments, please see my reply below.
>
> > On Fri, 9 Apr 2010, Anatolij Gustschin wrote:
> >
> > > This patch extends PTP gadget to support DeleteObject
> > > operation.
> > >
> > > Signed-off-by: Anatolij Gustschin <agust@xxxxxxx>
> > > ---
> > > ptp.c | 206 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > > 1 files changed, 203 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/ptp.c b/ptp.c
> > > index b887176..87adbd4 100644
> > > --- a/ptp.c
> > > +++ b/ptp.c
[snip]
> > > +{
> > > + struct stat st;
> > > + int ret;
> > > + uid_t euid;
> > > + gid_t egid;
> > > +
> > > + /* access() is unreliable on NFS, we use stat() instead */
> > > + ret = stat(name, &st);
> > > + if (ret < 0) {
> > > + fprintf(stderr, "Cannot stat %s: %s\n", name, strerror(errno));
> > > + *resp_code = PIMA15740_RESP_GENERAL_ERROR;
> > > + return -1;
> > > + }
> > > +
> > > + euid = geteuid();
> > > + if (euid == st.st_uid) {
> > > + if (!(st.st_mode & S_IWUSR)) {
> > > + *resp_code = PIMA15740_RESP_OBJECT_WRITE_PROTECTED;
> > > + return -1;
> > > + }
> > > + goto del;
> > > + }
> > > +
> > > + egid = getegid();
> > > + if (egid == st.st_gid) {
> > > + if (!(st.st_mode & S_IWGRP)) {
> > > + *resp_code = PIMA15740_RESP_OBJECT_WRITE_PROTECTED;
> > > + return -1;
> > > + }
> > > + goto del;
> > > + }
> > > +
> > > + if (!(st.st_mode & S_IWOTH)) {
> > > + *resp_code = PIMA15740_RESP_OBJECT_WRITE_PROTECTED;
> > > + return -1;
> > > + }
> >
> > Isn't there an easier and less error-prone way to verify? Maybe just try
> > to delete, if errno == EPERM - return write-protected? Any specific reason
> > for doing it this way? Would this also cover SELinux, ACL,... Notice, I
> > have no idea about either of them, please, explain.
>
> Trying to delete using unlink() will remove the file. But we may not
> delete read-only files and must return Object_WriteProtected response
> code in such cases. Therefore I'm checking for effective uid/gid write
> permissions of the file.
>
> > > +
> > > +del:
> > > + ret = unlink(name);
> > > + if (ret) {
> > > + fprintf(stderr, "Cannot delete %s: %s\n",
> > > + name, strerror(errno));
> > > + *resp_code = PIMA15740_RESP_GENERAL_ERROR;
> >
> > If you accept my proposal to just try to unlink, you'd check for EACCESS /
> > EPERM here, and then set PIMA15740_RESP_OBJECT_WRITE_PROTECTED response
> > code, otherwise PIMA15740_RESP_GENERAL_ERROR.
>
> No, this won't work. EPERM is set if the file system doesn't allow
> unlinking of files. EACCESS is set if write access to the directory
> containing the file is not allowed for the process's effective UID, or
> if one of the directories in the path to the file didn't allow search
> permission. If we want to allow deletion of files in the image directory
> neither of these conditions must be true, anyway. unlink() successfully
> deletes the file even if this file doesn't grant write permissions.
> unlink() is allowed to delete the file even if it is owned by another
> user. Having write permission to the directory containing the file is
> enough to delete the file.
You're right. Still, I would prefer to use a different approach if
possible. What if we try to truncate(2) the file instead? That should fail
without write permissions for the file, right?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
