Re: A bug in message_add_payload()

On Wed, 16 Apr 2003, Aref Taidi wrote:

> The problem I had was that message_check_duplicate() called message_free()
> to release message/memory.
>
> message_free() in the following code:
> for (i = ISAKMP_PAYLOAD_SA; i < ISAKMP_PAYLOAD_RESERVED_MIN; i++)
> for (payload = TAILQ_FIRST (&msg->payload[i]); payload; payload =
> next)
> {
> next = TAILQ_NEXT (payload, link);
> free (payload);
> }
>
> was trying to release memory associated with payload. The memory was
> corrupted/non-existent and caused crashes. What made me to look at
> message_add_payload() was that the payload being released was originally
> obtained by message_add_payload() in the realloc() statement as described
> earlier.

This 'payload' data was allocated by the malloc() call prior to the realloc() in message_add_payload(), or in the message_index_payload() function. As far as I recall, and after a quick search through the source, there are no other places where 'struct payload' objects are allocated.

The realloc() only concerned the msg->iov array, which was used and freed a few lines above these lines in message_free(), without errors.

If you get an error here, I suspect the TAILQ handling is faulty somehow. Or we may need to track where memory used in that TAILQ is allocated and freed. AFAICT this looks sane, but perhaps I've missed something here.

>
> I then noticed that memory obtained for msg->iov originally in
> message_alloc() is obtained through calloc(). I then modified my code so
> that realloc() cleared memory before returning the bock - and the problem
> was fixed. So you need to look at the side effects.

How did you fix this? Show me code, please.

As struct iov is defined as (/usr/includes/sys/uio.h)

Do you need help?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related openbsd.org Links advocacy announce bugs ipv6 misc ports ports-changes security-announce smp source-changes tech Request more information about Pantek

struct iovec {

        void    *iov_base;      /* Base address. */
        size_t   iov_len;       /* Length. */

and the realloc's new size here is (iov_len + 1) * sizeof (struct iovec), and both variables of the the newly allocated iovec struct's (i.e. iov_base and iov_len) are filled in (assuming the realloc does not fail), I do not see how clearing those bytes prior to this can possibly have any effect.

In fact, as far as I understand compiler optimizations (not a lot, admittedly), this zeroing would be a good candidate for an optimizer to remove from the generated code.

>From message_add_payload() :

  new_iov

• (struct iovec *)realloc (msg->iov, (msg->iovlen + 1) * sizeof *msg->iov); if (!new_iov) { log_error ("message_add_payload: realloc (%p, %lu) failed", msg->iov, (msg->iovlen + 1) * (unsigned long)sizeof *msg->iov); free (payload_node); return -1; } msg->iov = new_iov; new_iov[msg->iovlen].iov_base = buf; new_iov[msg->iovlen].iov_len = sz; msg->iovlen++;

You're proposing to add something like

  memset (&new_iov[msg->iovlen], 0, sizeof *msg->iov);

right next to the "msg->iov = new_iov;" line ?

Do you need more help?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related openbsd.org Links advocacy announce bugs ipv6 misc ports ports-changes security-announce smp source-changes tech Request more information about Pantek

I'm sorry, I really do not follow you here.

/H

--
Håkan Olsson         (+46) 708 437 337     Carlstedt Research
Unix, Networking, Security      (+46) 31 701 4264        & Technology AB