Re: [PATCH 4/8] pretty: fix parsing of half-valid "%<" and "%>" placeholders
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Mar 20, 2025, 09:18 UTC
- Message-ID
- <Z9vdS4bxY6spILsc@pks.im>
- In-Reply-To
- <7d6b62006ecaf7db159e8db0c85455ed58027ce6.1742367347.git.martin.agren@gmail.com>
On Wed, Mar 19, 2025 at 08:23:37AM +0100, Martin Ågren wrote:
Show 10 quoted lines
> When we parse a padding directive ("%<" or "%>"), we might populate a
> few of the struct's fields before bailing. This can result in such
> half-parsed information being used to actually introduce some
> padding/truncation.
>
> When parsing a "%<" or "%>", only store the parsed data after parsing
> successfully. The added test would have failed before this commit. It
> also shows how the existing behavior is hardly something someone can
> rely on since the non-consumed modifier ("%<(10,bad)") shows up verbatim
> in the pretty output.Ideally I'd expect us to die when seeing misformatted placeholders like this. This is way less confusing to the user as otherwise things _look_ like they work, but we silently do the wrong thing.
That being said, I have no idea whether we can do such a change now without breaking existing usecases. As you rightfully argue the result already is wrong, but with my proposal we'd completely refuse to do anything. Which I'd argue is a good thing in the end.
> We could let the caller use a temporary struct and only copy the data on > success. Let's instead make our parsing function easy to use correctly > by letting it only touch the output struct in the success case.
s/success/&ful/
Show 32 quoted lines
> While setting up a temporary struct for parsing into, we might as well
> initialize it to a well-defined state. It's unnecessary for the current
> implementation since it always writes to all three fields in a
> successful case, but some future-proofing shouldn't hurt.
>
> Note that the test relies on first using a correct placeholder
> "%<(4,trunc)" where "trunc" (`trunc_right`) lingers in our struct until
> it's then used instead of the invalid "bad". The next commit will teach
> us to clean up any remnants of "%<(4,trunc)" after handling it.
>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> pretty.c | 18 ++++++++++++------
> t/t4205-log-pretty-formats.sh | 6 ++++++
> 2 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/pretty.c b/pretty.c
> index e5e8ef24fa..a4fa052f8b 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1121,6 +1121,11 @@ static size_t parse_padding_placeholder(const char *placeholder,
> const char *ch = placeholder;
> enum flush_type flush_type;
> int to_column = 0;
> + struct padding_args ans = {
> + .flush_type = no_flush,
> + .truncate = trunc_none,
> + .padding = 0,
> + };
>
> switch (*ch++) {
> case '<':I honestly have no idea what `ans` stands for. You could call it `result` to signify that it's what we'll ultimately bubble up to the caller in the successful case.
Patrick