Re: [PATCH RFCv3 3/3] lib/test_printf: add test cases for '%pD'

On 15/06/2021 09.06, Justin He wrote:

Hi Rasmus

quoted

-----Original Message-----
From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Sent: Saturday, June 12, 2021 5:40 AM
To: Justin He <redacted>; Petr Mladek <pmladek@suse.com>; Steven
Rostedt [off-list ref]; Sergey Senozhatsky
[off-list ref]; Andy Shevchenko
[off-list ref]; Jonathan Corbet [off-list ref];
Alexander Viro [off-list ref]; Linus Torvalds <torvalds@linux-
foundation.org>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>; Eric Biggers
[off-list ref]; Ahmed S. Darwish [off-list ref]; linux-
doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
fsdevel@vger.kernel.org
Subject: Re: [PATCH RFCv3 3/3] lib/test_printf: add test cases for '%pD'

On 11/06/2021 17.59, Jia He wrote:

quoted

After the behaviour of specifier '%pD' is changed to print full path
of struct file, the related test cases are also updated.

Given the string is prepended from the end of the buffer, the check
of "wrote beyond the nul-terminator" should be skipped.

Sorry, that is far from enough justification.

I should probably have split the "wrote beyond nul-terminator" check in two:

One that checks whether any memory beyond the buffer given to
vsnprintf() was touched (including all the padding, but possibly more
for the cases where we pass a known-too-short buffer), symmetric to the
"wrote before buffer" check.

And then another that checks the area between the '\0' and the end of
the given buffer - I suppose that it's fair game for vsnprintf to use
all of that as scratch space, and for that it could be ok to add that
boolean knob.

Sorry, I could have thought sth like "write beyond the buffer" had been checked by
old test cases, but seems not.

It does. Before each (sub)test, we have (assume PAD_SIZE=4, BUF_SIZE=12)


|    <- alloced_buffer ->    |
|  PAD |  test_buffer | PAD  |
| **** | ************ | **** |

Then after snprintf(buf, 10, "pizza") we have

|    <- alloced_buffer ->    |
|  PAD |  test_buffer | PAD  |
| **** | pizza0****** | **** |
A      B       C   D         E

(with 0 being the nul character). Then

        if (memchr_inv(alloced_buffer, FILL_CHAR, PAD_SIZE)) {

checks whether snprint wrote anything between A and B, while

        if (memchr_inv(test_buffer + written + 1, FILL_CHAR, BUF_SIZE +
PAD_SIZE - (written + 1))) {

checks whether there was a write between C and E.

What I'm saying is that I can see it being reasonable for (some helper
inside) snprintf to actually write something beyond C, but certainly
never beyond D. So the "wrote beyond" test could be split up, with the
first half possibly being allowed to be opt-out for certain test cases.

I will split the "wrote beyond nul-terminator" check into 2 parts. One for
Non-%pD case, the other for %pD.

For %pD, it needs to check whether the space beyond test_buffer[] is written

No, that's not the right way to do this. Let me cook up a patch you can
include in your series.

Rasmus