Re: [PATCH] Make core.sharedRepository more generic (version 2)
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 15, 2008, 00:08 UTC
- Message-ID
- <7v3apo7zfg.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <20080412195754.GA15091@zakalwe.fi>
Heikki Orsila <heikki.orsila@iki.fi> writes:
Show 13 quoted lines
> diff --git a/builtin-init-db.c b/builtin-init-db.c > index 2854868..8c63295 100644 > --- a/builtin-init-db.c > +++ b/builtin-init-db.c > @@ -400,9 +400,9 @@ int cmd_init_db(int argc, const char **argv, const char *prefix) > char buf[10]; > /* We do not spell "group" and such, so that > * the configuration can be read by older version > - * of git. > + * of git. Note, we use octal numbers. > */ > - sprintf(buf, "%d", shared_repository); > + sprintf(buf, "0%o", shared_repository);
Unconditionally doing this makes the resulting repository unusable by git 1.5.5 and older, even when the user wanted to use the bog standard "git init --shared". You can limit the extent of damage if you continue writing PERM_GROUP and PERM_EVERYBODY out as 1 and 2, and use the new octal notation only when the user used the settings allowed only with new git.
Show 24 quoted lines
> @@ -438,11 +440,46 @@ int git_config_perm(const char *var, const char *value)
> !strcmp(value, "world") ||
> !strcmp(value, "everybody"))
> return PERM_EVERYBODY;
> +
> + /* Parse octal numbers */
> + i = strtol(value, &endptr, 8);
> + if (*endptr != 0) {
> + /* Not an octal number. Maybe true/false? */
> + if (git_config_bool(var, value))
> + return PERM_GROUP;
> + else
> + return PERM_UMASK;
> + }
> +
> + /* Handle compatibility cases */
> + switch (i) {
> + case PERM_UMASK: /* 0 */
> + return PERM_UMASK;
> + case OLD_PERM_GROUP: /* 1 */
> + return PERM_GROUP;
> + case OLD_PERM_EVERYBODY: /* 2 */
> + return PERM_EVERYBODY;
> + }This is valid only because forcing "chmod 0", "chmod 1", nor "chmod 2" would not make any sense. We might want to explain that in comment.
Show 11 quoted lines
> + /* A filemode value was given: 0xxx */
> +
> + if ((i & 0600) != 0600)
> + die("Problem with core.sharedRepository filemode value"
> + " (0%.3o).\nThe owner of files must always have "
> + "read and write permissions.", i);
> +
> + if (i & 0002)
> + warning("core.sharedRepository filemode (0%.3o) is a "
> + "security threat.\nMasking off write "
> + "permission for others\n", i);I am not sure about this.
If the user explicitly asked for world-writable, I think we should allow it. "is a" is too strong a statement to make without knowing how the access to the repository is arranged. In a setting where there is nothing but a restricted access over ssh, and "update-paranoid" hook in place, it may not be a threat at all, and you are forbidding hosting services to use such an access control model.
> + /* > + * Mask filemode value. Others can not get write permission. > + * x flags for directories are handled separately. > + */
Whitespace breakages.
> + return i & 0664;
> } > - return git_config_bool(var, value); > + return PERM_GROUP;
At this point we know value is NULL so always returning PERM_GROUP probably makes sense. But if you did
if (!value) return PERM_GROUP
upfront, you can lose one level of indentation from the major parts of this function (this is 70% style and 30% readability comment).