PaulStoffregen / USBHost_t36

USB Host Library for Teensy 3.6 and 4.0
165 stars 85 forks source link

Memory allocation code issue? #60

Closed lukehutch closed 3 years ago

lukehutch commented 3 years ago

This code pattern is used a number of times in memory.cpp for memory allocation and recycling (for Device_t, Pipe_t, Transfer_t, etc.):

Device_t * USBHost::allocate_Device(void)
{
    Device_t *device = free_Device_list;
    if (device) free_Device_list = *(Device_t **)device;
    return device;
}

void USBHost::free_Device(Device_t *device)
{
    *(Device_t **)device = free_Device_list;
    free_Device_list = device;
}

I don't understand what the intent is here. If device has type Device_t *, then *(Device_t **)device can only make sense if the very first field in Device_t is of type Device_t *, i.e. the next field. But next is not the first field in Device_t. (Same for Pipe_t and Transfer_t.)

I think this usually does not cause a problem for Memory_t and Pipe_t, because there is only one of each allocated:

static Device_t memory_Device[1];
static Pipe_t memory_Pipe[1] __attribute__ ((aligned(32)));

However there are 4 Transfer_t structs allocated:

static Transfer_t memory_Transfer[4] __attribute__ ((aligned(32)));

therefore, these methods will corrupt memory, unless I'm misunderstanding the code:

Transfer_t * USBHostPort::allocate_Transfer(void)
{
    Transfer_t *transfer = free_Transfer_list;
    if (transfer) free_Transfer_list = *(Transfer_t **)transfer;
    return transfer;
}

void USBHostPort::free_Transfer(Transfer_t *transfer)
{
    *(Transfer_t **)transfer = free_Transfer_list;
    free_Transfer_list = transfer;
}
lukehutch commented 3 years ago

I guess the memory corruption doesn't matter, since the first field of the struct is only overwritten by a type-coerced pointer when the struct is added to the free list. But still, this is pretty ugly, especially when some of the affected structs already have next fields that could be used...

lukehutch commented 3 years ago

Closing due to comment on forum about this