Comment # 17
on bug 8912
from Guy Harris
(In reply to comment #14)
> Some notes about the following part of the patch for wiretap/vwr.c in
> vwr_get_fpga_version():
>
> if ((rec_size > vVW510021_W_STATS_LEN) && (fpga_version == 1000)) {
> /* Check the version of the FPGA */
> if (header[8] == 48)
> fpga_version = S3_W_FPGA;
> }
>
> if ((rec_size > vVW510021_W_STATS_LEN) && (fpga_version == 1000)) {
> /* Check the version of the FPGA */
> if (header[8] == 48)
> fpga_version = S3_W_FPGA;
> }
>
> 1. The code is duplicated. Can the duplicated code be removed or was the
> second
> if (...) intended to be something different ?
I've removed the duplication.
> 2. More importantly: with the addition of this test, at least 1 capture file
> I have is incorrectly identified as a vwr capture file
> (and then a crash occurs (see below) when the file is read for
> dissection).
>
> Looking at the code in vwr.c: vwr_get_fpga_version(): this test seems
> a bit weak.
> Essentially: Test for one of hex 21/31/C1/8B/FE in the 0th byte
> and hex 30 in the 8th byte of a 16 byte 'header'.
>
> Looking a bit more deeply at the code: A search is done through the
> file looking for certain patterns and skipping stuff (headers/data) which
> don't match the pattern. Would it be possible to do some initial
> validation
> of each "header" (e.g., checking for valid 'cmd' values) so that there
> would
> be less likelihood of having to read a lot of data from the file before
> deciding that it's not a vwr file.
I'll look at strengthening the heuristics.
> 3. The crash occurred in 'vwr_read_rec_data_wlan' in the following code:
>
> if ( rec_size < ((int)common_fields.vw_msdu_length +
> (int)vwr->STATS_LEN) )
> /*something's been truncated, DUMP AS-IS*/
> memcpy(&data_ptr[bytes_written], &rec[mpdu_offset],
> common_fields.vw_msdu_length);
>
> I think the problem is copying something greater than 'rec_size' is ng.
I've added more sanity checks to the code in the trunk. (I reorganized the
code a fair bit to make it a bit clearer what was going on so I could figure
out
> (I'll see if I can share the capture file I have which causes the crash).
If you still have it, see what happens on the trunk.
You are receiving this mail because:
- You are watching all bug changes.
