Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: ZheNing Hu <adlternative@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com>,
	Git List <git@vger.kernel.org>,
	Christian Couder <christian.couder@gmail.com>
Subject: Re: [PATCH v3] [GSOC]trailer: pass arg as positional parameter
Date: Fri, 26 Mar 2021 21:29:46 +0800
Message-ID: <CAOLTT8TMFjvH1TzZ9qRtW1v7aDYypdajJxRKpmZ0TaG_xx5H0w@mail.gmail.com> (raw)
In-Reply-To: <xmqqwntuuav7.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> 于2021年3月26日周五 上午6:28写道:
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > Original implementation of `trailer.<token>.command` use
>
> uses
>
> > `strbuf_replace` to replace $ARG in command with the <value>
> > of the trailer, but it have a problem: `strbuf_replace`
>
> has
>
> > replace the $ARG only once, If the user's trailer command
>
> replaces the $ARG only once.
>

Okay... singular and plural problems.

> > have used more than one $ARG, the remaining replacement will
> > fail.
>
> "will fail" is quite vague.  It is just left unreplaced, and if the
> user expects all of them to be replaced, then the expectation and
> reality would not match, but all of that you have already said by
> "replaces the $ARG only once.", so I think this sentence should be
> removed.
>

Indeed so.

> > If directly modify the implementation of the original
> > `trailer.<token>.command`, The user’s previous `'$ARG'` in
> > trailer command will not be replaced.
>
> That statement does not make much sense.  Depending on the way how
> the implementation is "directly" modified, you can fix the "replaces
> only once" problem without introducing such a problem.  Just look
> for '$ARG' in the string and replace all of them, not just the first
> one.  It's not too difficult.
>
> This confusion primarily comes from the fact that you forgot to
> explain the other problem you are fixing, I think.  Even though the
> trailer.<token>.command documentation implies that the user is
> expected to give a shell script or some sort as the command and the
> use of $ARG makes it look like a shell variable, the original
> implementation does not treat it as a shell variable at all.  And
> the textual replacement is done without making sure the value being
> replaced has characters with special meaning in the shell language.
>

Yes! The accidental injection problem caused should have been the focus
of my explanation.

> So existing .command may incorrectly use $ARG inside a single-quote
> pair and expect it to be replaced to a string inside a single-quote
> pair.  A malformed, or worse, malicious, value would escape out of
> the single-quote pair (remember, the '; rm -fr .' example?) and
> execute arbitrary code in an unexpected way.  The (ungrammatical)
> "if directly modify the implementation" refers to a potential way to
> fix these two problems at the same time by doing the $ARG thing
> without using textual replacement, namely, exporting the value as an
> environment variable ARG.  If that approach was taken, then, $ARG
> enclosed in a single-quote pair will no longer be replaced, which
> makes it a backward incompatible change.
>

Oh, I remeber it: terrible shell injection. Specifically, it looks like this:

$ git config trailer.sign.command "git log --author='\$ARG'"
$ git interpret-trailers --trailer "sign = adl' && rm -rf ./repo/'"

now I know that should be "backward incompatible change" as you said.

> But without describing what solution you are talking about, your
> three-line description does not make much sense.
>
> > So now add new config
> > "trailer.<token>.cmd", pass trailer's value as positional
> > parameter 1 to the user's command, the user can use $1 as
> > trailer's value, to implement original variable replacement.
> >
> > If the user has these two configuration: "trailer.<token>.cmd"
> > and "trailer.<token>.command", "cmd" will execute and "command"
> > will not executed.
> >
> > Original `trailer.<token>.command` can still be used until git
> > completely abandoned it.
> >
> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
>
> Let's rewrite it completely.
>
>         The `trailer.<token>.command` configuration variable
>         specifies a command (run via the shall, so it does not have
>         to be a single name of or path to the command, but can be a
>         shell script), and the first occurrence of substring $ARG is
>         replaced with the value given to the `interpret-trailer`
>         command for the token.  This has two downsides:
>
>         * The use of $ARG in the mechanism misleads the users that
>           the value is passed in the shell variable, and tempt them
>           to use $ARG more than once, but that would not work, as
>           the second and subsequent $ARG are not replaced.
>
>         * Because $ARG is textually replaced without regard to the
>           shell language syntax, even '$ARG' (inside a single-quote
>           pair), which a user would expect to stay intact, would be
>           replaced, and worse, if the value had an unmatching single
>           quote (imagine a name like "O'Connor", substituted into
>           NAME='$ARG' to make it NAME='O'Connor), it would result in
>           a broken command that is not syntactically correct (or
>           worse).
>
>         Introduce a new `trailer.<token>.cmd` configuration that
>         takes higher precedence to deprecate and eventually remove
>         `trailer.<token>.command`, which passes the value as a
>         parameter to the command.  Instead of "$ARG", the users will
>         refer to the value as positional argument, $1, in their
>         scripts.
>
> I tried to cover everything we need to tell the reviewers about this
> change with the above.  Did I miss anything?

Nothing to blame, feature of the old command, 2 problems, 1 solution,
this is what the log should look like.

>
> > +trailer.<token>.cmd::
> > +     Similar to 'trailer.<token>.command'. But the difference is that
> > +     `$1` is used in the command to replace the value of the trailer
> > +     instead of the original `$ARG`, which means that we can pass the
> > +     trailer value multiple times in the command.
>
> We eventually want to deprecate the .command, so we'd prefer not to
> rely on its description too much (e.g. try to find a way to say what
> you want to say without "instead of the original `$ARG`").
>

Oh! here I can’t rely on the documentation of the old `.command`, otherwise it’s
not easy to delete the old documentation.

>         The command specified by this configuration variable is run
>         with a single parameter, which is the <value> part of an
>         existing trailer with the same <token>.  The output from the
>         command is then used as the value for the <token> in the
>         resulting trailer.
>
> would be the replacement for the part that talks about $ARG in the
> description of trailer.<token>.command.
>
> The original description for `trailer.<token>.command` is so jumbled
> (not your failure at all) that I had a hard time to understand what
> it is trying to say (e.g. what does "as if a special <token>=<value>
> argument were added at the beginning of the command line" mean?  Is
> it making a one-shot export of environment variable to run the
> command???), so the above may need further adjustment.  Christian?
> Care to help out?
>
> > +     E.g. `git config trailer.sign.cmd "test -n \"$1\" && echo \"$1\" || true "`.
>
> An example is good.  There is a whole EXAMPLES section in this
> manual page, and the one that uses $ARG may be a good candidate to
> look at and change to use .cmd (instead of .command).
>

Okay, I will modify the paragraphs containing `.command` in these examples.

> > +     If the user has these two configuration: "trailer.<token>.cmd"
> > +     and "trailer.<token>.command", "cmd" will be executed and "command"
> > +     will not be executed.
>
>         When both .cmd and .command are given for the same <token>,
>         .cmd is used and .command is ignored.
>
> > diff --git a/trailer.c b/trailer.c
> > index be4e9726421c..634d3f1ff04a 100644
> > --- a/trailer.c
> > +++ b/trailer.c
> > @@ -14,6 +14,7 @@ struct conf_info {
> >       char *name;
> >       char *key;
> >       char *command;
> > +     char *cmd;
> >       enum trailer_where where;
> >       enum trailer_if_exists if_exists;
> >       enum trailer_if_missing if_missing;
> > @@ -127,6 +128,7 @@ static void free_arg_item(struct arg_item *item)
> >       free(item->conf.name);
> >       free(item->conf.key);
> >       free(item->conf.command);
> > +     free(item->conf.cmd);
> >       free(item->token);
> >       free(item->value);
> >       free(item);
> > @@ -216,18 +218,24 @@ static int check_if_different(struct trailer_item *in_tok,
> >       return 1;
> >  }
> >
> > -static char *apply_command(const char *command, const char *arg)
> > +static char *apply_command(const char *command, const char *cmd_, const char *arg)
> >  {
> >       struct strbuf cmd = STRBUF_INIT;
> >       struct strbuf buf = STRBUF_INIT;
> >       struct child_process cp = CHILD_PROCESS_INIT;
> >       char *result;
> >
> > -     strbuf_addstr(&cmd, command);
> > -     if (arg)
> > -             strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
> > -
> > -     strvec_push(&cp.args, cmd.buf);
> > +     if (cmd_) {
> > +             strbuf_addstr(&cmd, cmd_);
> > +             strvec_push(&cp.args, cmd.buf);
> > +             if (arg)
> > +                     strvec_push(&cp.args, arg);
> > +     } else if (command) {
> > +             strbuf_addstr(&cmd, command);
> > +             strvec_push(&cp.args, cmd.buf);
> > +             if (arg)
> > +                     strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
> > +     }
>
> OK.  it is clear cmd_ takes precedence this way.
>
> Later (not as part of this patch, but a few releases down the road),
> we may want to add a warning() about using a deprecated feature when
> "else if (command)" block is taken.
>

Fine, I will keep this version of "cmd priority execution" for now.

> > @@ -247,7 +255,7 @@ static char *apply_command(const char *command, const char *arg)
> >
> >  static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok)
> >  {
> > -     if (arg_tok->conf.command) {
> > +     if (arg_tok->conf.command || arg_tok->conf.cmd) {
> >               const char *arg;
> >               if (arg_tok->value && arg_tok->value[0]) {
> >                       arg = arg_tok->value;
> > @@ -257,7 +265,7 @@ static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg
> >                       else
> >                               arg = xstrdup("");
> >               }
> > -             arg_tok->value = apply_command(arg_tok->conf.command, arg);
> > +             arg_tok->value = apply_command(arg_tok->conf.command, arg_tok->conf.cmd, arg);
>
> It might be cleaner to just pass arg_tok->conf to apply_command()
> and hide "cmd takes precedence over command" as an implementation
> detail of that helper function.
>
> The implementation looks as good as the original "command" with that
> change at this point.  Documentation may need a bit more polishing.
>

you're right.

> Thanks.

Thanks, Junio.
You and the people in the git community are very enthusiastic,
You have patiently explained these small mistakes that I made,
and taught me a lot of problems that I didn't notice.

Grateful.

--
ZheNing Hu

  reply index

Thread overview: 101+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-23 14:53 [PATCH] [GSOC]trailer: change $ARG to environment variable ZheNing Hu via GitGitGadget
2021-03-24 15:42 ` [PATCH v2] [GSOC]trailer: pass arg as positional parameter ZheNing Hu via GitGitGadget
2021-03-24 20:18   ` Junio C Hamano
2021-03-25  1:43     ` ZheNing Hu
2021-03-25 11:53   ` [PATCH v3] " ZheNing Hu via GitGitGadget
2021-03-25 22:28     ` Junio C Hamano
2021-03-26 13:29       ` ZheNing Hu [this message]
2021-03-26 16:13     ` [PATCH v4] " ZheNing Hu via GitGitGadget
2021-03-27 18:04       ` Junio C Hamano
2021-03-27 19:53         ` Christian Couder
2021-03-28 10:46           ` ZheNing Hu
2021-03-29  9:04             ` Christian Couder
2021-03-29 13:43               ` ZheNing Hu
2021-03-30  8:45                 ` Christian Couder
2021-03-30 11:22                   ` ZheNing Hu
2021-03-30 15:07                     ` ZheNing Hu
2021-03-30 17:14                       ` Junio C Hamano
2021-03-31  5:14                         ` ZheNing Hu
2021-03-31 18:19                           ` Junio C Hamano
2021-03-31 18:29                             ` Junio C Hamano
2021-04-01  3:56                               ` ZheNing Hu
2021-04-01 19:49                                 ` Junio C Hamano
2021-04-02  2:08                                   ` ZheNing Hu
2021-04-01  3:39                             ` ZheNing Hu
2021-03-31 10:05       ` [PATCH v5 0/2] " ZheNing Hu via GitGitGadget
2021-03-31 10:05         ` [PATCH v5 1/2] [GSOC] run-command: add shell_no_implicit_args option ZheNing Hu via GitGitGadget
2021-04-01  7:22           ` Christian Couder
2021-04-01  9:58             ` ZheNing Hu
2021-03-31 10:05         ` [PATCH v5 2/2] [GSOC]trailer: pass arg as positional parameter ZheNing Hu via GitGitGadget
2021-04-01  7:28         ` [PATCH v5 0/2] " Christian Couder
2021-04-01 10:02           ` ZheNing Hu
2021-04-02 13:26         ` [PATCH v6] [GSOC] trailer: add new trailer.<token>.cmd config option ZheNing Hu via GitGitGadget
2021-04-02 20:48           ` Junio C Hamano
2021-04-03  5:08             ` ZheNing Hu
2021-04-04  5:34               ` Junio C Hamano
2021-04-03  5:51             ` Christian Couder
2021-04-04 23:26               ` Junio C Hamano
2021-04-06  3:47                 ` Christian Couder
2021-04-06  3:52                   ` Christian Couder
2021-04-06  5:16                     ` ZheNing Hu
2021-04-06  5:34                       ` Junio C Hamano
2021-04-06  5:37                       ` Junio C Hamano
2021-04-04  5:43             ` ZheNing Hu
2021-04-04  8:52               ` Christian Couder
2021-04-04  9:53                 ` ZheNing Hu
2021-04-02 23:44           ` Junio C Hamano
2021-04-03  3:22             ` ZheNing Hu
2021-04-03  4:31               ` Junio C Hamano
2021-04-03  5:15                 ` ZheNing Hu
2021-04-04 13:11           ` [PATCH v7] " ZheNing Hu via GitGitGadget
2021-04-06 16:23             ` Christian Couder
2021-04-07  4:51               ` ZheNing Hu
2021-04-09 13:37             ` [PATCH v8 0/2] [GSOC] trailer: add new .cmd " ZheNing Hu via GitGitGadget
2021-04-09 13:37               ` [PATCH v8 1/2] [GSOC] docs: correct descript of trailer.<token>.command ZheNing Hu via GitGitGadget
2021-04-09 19:02                 ` Christian Couder
2021-04-10 13:40                   ` ZheNing Hu
2021-04-09 13:37               ` [PATCH v8 2/2] [GSOC] trailer: add new .cmd config option ZheNing Hu via GitGitGadget
2021-04-09 20:18                 ` Christian Couder
2021-04-10 14:09                   ` ZheNing Hu
2021-04-09 19:59               ` [PATCH v8 0/2] " Christian Couder
2021-04-12 16:39               ` [PATCH v9 " ZheNing Hu via GitGitGadget
2021-04-12 16:39                 ` [PATCH v9 1/2] [GSOC] docs: correct descript of trailer.<token>.command ZheNing Hu via GitGitGadget
2021-04-12 20:42                   ` Junio C Hamano
2021-04-16 12:03                     ` Christian Couder
2021-04-17  1:54                       ` Junio C Hamano
2021-04-12 16:39                 ` [PATCH v9 2/2] [GSOC] trailer: add new .cmd config option ZheNing Hu via GitGitGadget
2021-04-12 20:51                   ` Junio C Hamano
2021-04-13  7:33                     ` Christian Couder
2021-04-13 12:02                       ` ZheNing Hu
2021-04-13 19:18                         ` Junio C Hamano
2021-04-14 13:27                           ` ZheNing Hu
2021-04-14 20:33                             ` Junio C Hamano
2021-04-15 15:32                               ` ZheNing Hu
2021-04-15 17:41                                 ` Junio C Hamano
2021-04-16 12:54                               ` Christian Couder
2021-04-13 18:14                       ` Junio C Hamano
2021-04-16  8:47                 ` [PATCH v10 0/2] " ZheNing Hu via GitGitGadget
2021-04-16  8:47                   ` [PATCH v10 1/2] [GSOC] docs: correct descript of trailer.<token>.command ZheNing Hu via GitGitGadget
2021-04-16 19:11                     ` Junio C Hamano
2021-04-16  8:47                   ` [PATCH v10 2/2] [GSOC] trailer: add new .cmd config option ZheNing Hu via GitGitGadget
2021-04-16 19:13                     ` Junio C Hamano
2021-04-16 19:21                     ` Junio C Hamano
2021-04-16 19:25                       ` Junio C Hamano
2021-04-17  2:58                         ` Junio C Hamano
2021-04-17  3:36                           ` Junio C Hamano
2021-04-17  7:41                             ` ZheNing Hu
2021-04-17  8:11                               ` Junio C Hamano
2021-04-17 15:13                   ` [PATCH v11 0/2] " ZheNing Hu via GitGitGadget
2021-04-17 15:13                     ` [PATCH v11 1/2] [GSOC] docs: correct description of .command ZheNing Hu via GitGitGadget
2021-04-17 15:13                     ` [PATCH v11 2/2] [GSOC] trailer: add new .cmd config option ZheNing Hu via GitGitGadget
2021-04-17 22:26                     ` [PATCH v11 0/2] " Junio C Hamano
2021-04-18  7:47                       ` ZheNing Hu
2021-04-21  0:09                         ` Junio C Hamano
2021-04-21  5:47                           ` ZheNing Hu
2021-04-21 23:40                             ` Junio C Hamano
2021-04-22  9:20                               ` ZheNing Hu
2021-04-27  6:49                                 ` Junio C Hamano
2021-04-27 12:24                                   ` ZheNing Hu
2021-05-03 15:41                     ` [PATCH v12 " ZheNing Hu via GitGitGadget
2021-05-03 15:41                       ` [PATCH v12 1/2] [GSOC] docs: correct descript of trailer.<token>.command ZheNing Hu via GitGitGadget
2021-05-03 15:41                       ` [PATCH v12 2/2] [GSOC] trailer: add new .cmd config option ZheNing Hu via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAOLTT8TMFjvH1TzZ9qRtW1v7aDYypdajJxRKpmZ0TaG_xx5H0w@mail.gmail.com \
    --to=adlternative@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git