Re: [PATCH]: chmod - do inform about using different mode than requested with SGID/SUID/sticky bits without permissions to change them

[Ondřej Vašík]

Hello,
thanks for review and objections.

Paul Eggert wrote:
> Ondřej Vašík <address@hidden> writes:
> 
> > -      bool changed = (chmod_succeeded
> > -                 && mode_changed (file, old_mode, new_mode));
> > +      bool mode_change = mode_changed (file, old_mode, new_mode);
> > +      bool changed = (chmod_succeeded && mode change);

Sorry, that change was obviously redundant...

> > +
> > +      if (chmod_succeeded && ((old_mode ^ new_mode) & CHMOD_MODE_BITS))
> > +        {
> > +
> > +          /* Changed to another mode than requested */
> 
> This doesn't look right to me.  First, surely there's no need to invoke
> mode_changed if chmod_succeeded is false.  Second, ((old_mode ^
> new_mode) & CHMOD_MODE_BITS) doesn't tell us whether we changed to
> another mode than requested; mode_change tells us that.

About first you are right, that change was accidently added for other
approach and I forgot to remove it.
About second - that's not everytime true - as shown in the example -
SUID, SGID and sticky bits could be ignored in mode changed. That's
documented in info changes. Anyway user is then mistakenly informed that
mode was changed e.g. to 2755, but the real change was to 0755 - and
that's imho wrong.

> > +          struct stat new_stats;
> > +          char perms_requested[12];
> > +          char perms_actual[12];
> > +
> > +          if (stat (file, &new_stats) != 0)
> 
> Third, this means we've invoked 'stat' twice on the file afterwards,
> once here, and once in mode_changed.  We should invoke 'stat' only in
> mode_changed.

You are right, that stat() call easy to reduce. So better solution for
that issue would be to check everything in mode_changed() function. 

Proposed amended patch is attached, should solve all your objections
about the previous patch.

Greetings,
          Ondřej Vašík

chmod-sgid.patch
Description: Text Data

signature.asc
Description: Toto je digitálně podepsaná část zprávy