Re: [PATCH] avrcp: Fix wrong pointer check

On Mon, Feb 03, 2014 at 01:20:11AM -0800, Luiz Augusto von Dentz wrote:

Hi Andrei,

On Sun, Feb 2, 2014 at 11:51 PM, Andrei Emeltchenko
[off-list ref] wrote:

quoted

Hi Luiz,

On Sun, Feb 02, 2014 at 08:03:34AM -0800, Luiz Augusto von Dentz wrote:

quoted

Hi Andrei,

On Fri, Jan 31, 2014 at 12:33 AM, Andrei Emeltchenko
[off-list ref] wrote:

quoted

From: Andrei Emeltchenko <redacted>

There is wrong assumption that handler might be NULL while it is a
pointer to a struct table so check instead for struct members. This
fixes accessing wrong memory.
---
 profiles/audio/avrcp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index df88138..5030ce1 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c

@@ -1673,7 +1673,7 @@ static size_t handle_vendordep_pdu(struct avctp *conn, uint8_t transaction,
                        break;
        }

-       if (!handler || handler->code != *code) {
+       if (!handler->code || handler->code != *code) {

The code checks if  session->control_handlers is initialized and Im
not sure what is the invalid memory access you are talking about since
handle->code is no a pointer, I do agree that we should probably drop
the second check for the handler in the lines bellow.

handler is a pointer, and it points to

static const struct control_pdu_handler control_handlers[]
table with the last element:

...
                { },
};
...

So checking for !handler is pointless.

Right, because checking for pointer is pointless, yes Im being
sarcastic here... Now lets be clear, you are changing a check of a
pointer to a value and claiming it fixes invalid accesses which does
not make any sense, what could make sense is to check if
handler->pdu_id == pdu->pdu_id since that what we check when we lookup
for a handle.

No, we break if handler->pdu_id == pdu->pdu_id one line above ;)

Best regards 
Andrei Emeltchenko 

-- 
Luiz Augusto von Dentz