tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Implement mode_select for atapi tape drives

On Sat, 8 Aug 2009, Marc Balmer wrote:

Why do you use

memset(&atapi_select, 0, atapi_select_len);

instead of

memset(&atapi_select, 0, sizeof(atapi_select));

and zap the u_int atapi_select_len variable?

I responded with:

Basically, this whole routine was copied almost verbatim from the equivalent code in sys/dev/scsipi/st_scsi.c which also calculates the number of bytes to clear. Since it worked, I never looked any further.

There is always the chance that the computed value of atapi_select_len (based on the _actual_ size of the device's page_0) is less than the possible maximum size of page_0. So we could save a few nanoseconds by clearing fewer bytes. In practice I don't expect this to make any real difference.


If I change my patch, I'd want to update the calculation in st_scsi.c as well, to keep them in sync.

Marc countered with:

And on the worse side of possibilities, there is the chance of buffer overflow, if for some the reason the calculated value is bigger than the structs sice. This code should really use sizeof, to be unambigously clear.


Yes, it should be changed there, too.

OK, I can change the memset() calls in both places to use sizeof.

But the calculated value of *_select_len is actually passed to routine scsipi_mode_select() a little bit later on, so we still need the calculated value.

If you're really concerned about a buffer overflow, it would probably make sense to validate the value of st->page_0_size when it is set in the load_quirk() code (for both atapi and scsi).

|   Paul Goyette   | PGP DSS Key fingerprint: |  E-mail addresses:      |
| Customer Service | FA29 0E3B 35AF E8AE 6651 |  paul at   |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at |
| Kernel Developer |                          | pgoyette at  |

Home | Main Index | Thread Index | Old Index