On Wed, Nov 29, 2023 at 10:11:26AM +0100, Maxime Ripard wrote: > Hi Ville, > > On Tue, Nov 28, 2023 at 03:49:08PM +0200, Ville Syrjälä wrote: > > On Tue, Nov 28, 2023 at 02:29:40PM +0100, Maxime Ripard wrote: > > > On Tue, Nov 28, 2023 at 02:54:02PM +0200, Jani Nikula wrote: > > > > On Tue, 28 Nov 2023, Maxime Ripard <mripard@xxxxxxxxxx> wrote: > > > > > All the drm_connector_init variants take at least a pointer to the > > > > > device, connector and hooks implementation. > > > > > > > > > > However, none of them check their value before dereferencing those > > > > > pointers which can lead to a NULL-pointer dereference if the author > > > > > isn't careful. > > > > > > > > Arguably oopsing on the spot is preferrable when this can't be caused by > > > > user input. It's always a mistake that should be caught early during > > > > development. > > > > > > > > Not everyone checks the return value of drm_connector_init and friends, > > > > so those cases will lead to more mysterious bugs later. And probably > > > > oopses as well. > > > > > > So maybe we can do both then, with something like > > > > > > if (WARN_ON(!dev)) > > > return -EINVAL > > > > > > if (drm_WARN_ON(dev, !connector || !funcs)) > > > return -EINVAL; > > > > > > I'd still like to check for this, so we can have proper testing, and we > > > already check for those pointers in some places (like funcs in > > > drm_connector_init), so if we don't cover everything we're inconsistent. > > > > People will invariably cargo-cult this kind of stuff absolutely > > everywhere and then all your functions will have tons of dead > > code to check their arguments. > > And that's a bad thing because... ? > > Also, are you really saying that checking that your arguments make sense > is cargo-cult? > > We're already doing it in some parts of KMS, so we have to be > consistent, and the answer to "most drivers don't check the error" > cannot be "let's just give on error checking then". > > > I'd prefer not to go there usually. > > > > Should we perhaps start to use the (arguably hideous) > > - void f(struct foo *bar) > > + void f(struct foo bar[static 1]) > > syntax to tell the compiler we don't accept NULL pointers? > > > > Hmm. Apparently that has the same problem as using any > > other kind of array syntax in the prototype. That is, > > the compiler demands to know the definition of 'struct foo' > > even though we're passing in effectively a pointer. Sigh. > > Honestly, I don't care as long as it's something we can unit-test to > make sure we make it consistent. We can't unit test a complete kernel > crash. Why do you want to put utterly broken code into a unit test? -- Ville Syrjälä Intel
