Thank you very much for your reply. Do you mean if I track the argument to the
callers, I should be able to figure out that the pin is always valid? I think I
have two questions for this approach. First, does it mean that the branch
returning NULL is practically dead code? Second, the driver code, as it appears
to me, does not have well-defined entry points like the main function. So if I
manage to track the arguments to a function whose caller is not explicit (e.g.,
it's address is taken in a structure), should I still assume that the pin is
valid when this function is used?
Thanks,
Shaobo
On 2019/3/19 2:32, Jani Nikula wrote:
On Mon, 18 Mar 2019, Shaobo He <shaobo@xxxxxxxxxxx> wrote:
I see. In light of this commit, is it a better solution than adding NULL-checks
is to replace the if branch conditioned by `WARN_ON` with simply `WARN` like the
following,
struct i2c_adapter *intel_gmbus_get_adapter(struct drm_i915_private *dev_priv,
unsigned int pin)
{
WARN(!intel_gmbus_is_valid_pin(dev_priv, pin), "Invalid pin: %d\n", pin);
return &dev_priv->gmbus[pin].adapter;
}
So all of this discussion is hypothetical in the sense that it really
should never happen. You can track down the args passed to
intel_gmbus_get_adapter(), while the static analyzer is unable to do
that.
BR,
Jani.
Shaobo
On 3/18/19 5:53 PM, Rodrigo Vivi wrote:
On Mon, Mar 18, 2019 at 05:39:48PM -0600, Shaobo He wrote:
Hi Rodrigo,
Sorry I'm a bit lost here. May I ask where the `WARN` is?
along with the return NULL
struct i2c_adapter *intel_gmbus_get_adapte()
if (WARN_ON(!intel_gmbus_is_valid_pin(dev_priv, pin)))
return NULL;
Thanks,
Shaobo
On 3/18/19 5:26 PM, Rodrigo Vivi wrote:
Hi Shaobo,
n Mon, Mar 18, 2019 at 05:01:10PM -0600, Shaobo He wrote:
Hello everyone,
My name is Shaobo He and I am a graduate student at University of Utah. I am
using a static analysis tool to search for null pointer dereferences and
came across a potentially invalid memory access in the file
drivers/gpu/drm/i915/intel_crt.c: in function `intel_crt_detect_ddc`,
function `intel_gmbus_get_adapter` can return a NULL pointer which is
if this happens we've done a terrible job on defining the platform...
dereferenced by the call to `drm_get_edid` or `intel_gmbus_is_forced_bit`.
but it seems you are right... this will reach i2c_transfer in the end
and it will break everything after we gave the Warning...
It seems that the return value of `intel_gmbus_get_adapter` is never
NULL-checked. If so, it would be better to replace the branch to return a
NULL pointer with something like `BUG_ON`.
what about just adding if (!i2c) return false
instead of BUG.
We already have the WARN to debug if this case ever happens.
Thanks,
Rodrigo.
Please let me know if it makes sense. I am looking forward to your reply.
Best,
Shaobo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
