Hi Aurélien,
Thanks for the review. Please read my responses inline below.
On Wed, Sep 16, 2020 at 4:26 PM Aurélien Aptel <aaptel@xxxxxxxx> wrote:
>
> Shyam Prasad N <nspmangalore@xxxxxxxxx> writes:
> > This is a fix for the scenario of a krb5 user running a "sudo mount".
> > Even if the user has cred cache populated, when the mount is run using
> > sudo, uid switches to 0. So cred cache for the root user will be
> > searched for, unless cruid is specified explicitly. This fix checks
> > for cruid=$SUDO_UID as a fallback option, when the mount fails with
> > ENOKEY.
>
> The idea seems good.
This was Pavel's suggestion. :)
>
> > @@ -2053,7 +2066,24 @@ int main(int argc, char **argv)
> > parsed_info = NULL;
> > fprintf(stderr, "Unable to allocate memory: %s\n",
> > strerror(errno));
> > - return EX_SYSERR;
> > + rc = EX_SYSERR;
> > + goto mount_exit;
> > + }
> > +
> > + reinit_parsed_info =
> > + (struct parsed_mount_info *) malloc(sizeof(*reinit_parsed_info));
> > + if (reinit_parsed_info == NULL) {
> > + fprintf(stderr, "Unable to allocate memory: %s\n",
> > + strerror(errno));
> > + rc = EX_SYSERR;
> > + goto mount_exit;
> > + }
> > +
> > + options = calloc(options_size, 1);
> > + if (!options) {
> > + fprintf(stderr, "Unable to allocate memory.\n");
> > + rc = EX_SYSERR;
> > + goto mount_exit;
>
> This function later forks, so if you allocate before the fork, you need
> to free in parent and in the child.
Good point.
I think I'm doing it right though. I allocate all that I need at the beginning.
And the function always terminates in mount_exit, both for parent and
child. That is where the allocations are freed.
I know the child will have the options buffer unnecessarily allocated,
but isn't the code flow simpler this way?
Please let me know if you see an issue there.
> > @@ -2228,6 +2252,7 @@ mount_retry:
> > if (nextaddress)
> > *nextaddress++ = '\0';
> > }
> > + memset(options, 0, sizeof(*options));
> > goto mount_retry;
>
> Altho not wrong this is a bit misleading as options is a char*. If you
> do a memset do it option_size, or do options[0] = 0;
Got it. Will do.
>
> > case ENODEV:
> > fprintf(stderr,
> > @@ -2250,6 +2275,21 @@ mount_retry:
> > already_uppercased = 1;
> > goto mount_retry;
> > }
>
> Need to reset options again before goto I guess. Maybe reset option
> after the retry label?
Good catch. This code existed before my changes, and I had noted this
bug. But forgot it during my changes. :)
I was actually confused if I should reset after the label, or before the goto.
After the label is an added overhead in the "happy" code path, so went
with this. But it does reduce the chances of missing out a reset.
For now I'll reset options before each goto mount_retry. Please let me
know if you feel the other approach is better.
>
> > }
> > - free(options);
> > - free(orgoptions);
> > - free(mountpoint);
> > + if (reinit_parsed_info)
> > + free(reinit_parsed_info);
> > + if (options)
> > + free(options);
> > + if (orgoptions)
> > + free(orgoptions);
> > + if (mountpoint)
> > + free(mountpoint);
> > return rc;
>
> free(NULL) is defined to be a no-op, you don't need the checks.
Sure. Will do.
>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
--
-Shyam
