On Tue, Sep 19, 2023 at 4:16 PM Christian Brauner <brauner@xxxxxxxxxx> wrote: > You're changing how the code currently works which is written in a way > that ensures all fields are initialized to zero. The fact that currently > nothing looks at private is irrelevant. Two callers were previously using a designated intiializer which implicitly zero-initializes unmentioned fields; but that was not intentional, but accidental. It is true that these two call sites are changed, now omitting the implicit (and unintended) initializer. If you consider this a problem, I'll re-add it to those two callers. But adding the "private" initializer to the new function would also change how the code currently works - fot the other callers. It's only relevant if the "private" field is part of some API contract. If that API contract is undocumented, we should add documentation - what is it? I'll write documentation. > Following your argument below this might very easily be the cause for > another CVE when something starts looking at this. When something starts looking at this, the API contract changes, and this one function needs to be adjusted. Without this function, there is not one function, but an arbitrary number of redundant copies of this initializing code which all need to be fixed. As I said, only two copies of those currently do initialize "private", the others do not. Therefore, my patch is strictly an improvement, and is safer against those alleged future CVEs, which is the very goal of my patch. > Wouldn't it make more sense to have the pipe_buf_init() initialize the > whole thing and for the place where it leaves buf->private untouched you > can just do: Would it? I don't think so, but if you insist, I'll add it. I prefer to leave "private" uninitialized unless there is a reason to initialize it, for the reason I stated in my previous reply.
