* re: [media] gspca: Fix locking issues related to suspend/resume
@ 2012-07-19 12:13 Dan Carpenter
2012-09-09 21:59 ` Hans de Goede
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2012-07-19 12:13 UTC (permalink / raw)
To: hans.verkuil; +Cc: linux-media
Hello Hans Verkuil,
This is a semi-automatic email about new static checker warnings.
The patch 254902b01d2a: "[media] gspca: Fix locking issues related to
suspend/resume" from May 6, 2012, leads to the following Smatch
complaint:
drivers/media/video/gspca/sq905c.c:176 sq905c_dostream()
warn: variable dereferenced before check 'gspca_dev->dev' (see line 180)
drivers/media/video/gspca/sq905c.c
158 /* Request the header, which tells the size to download */
159 ret = usb_bulk_msg(gspca_dev->dev,
160 usb_rcvbulkpipe(gspca_dev->dev, 0x81),
^^^^^^^^^^^^^^
Derereference inside the calls to usb_bulk_msg() and usb_rcvbulkpipe().
161 buffer, FRAME_HEADER_LEN, &act_len,
162 SQ905C_DATA_TIMEOUT);
163 PDEBUG(D_STREAM,
164 "Got %d bytes out of %d for header",
165 act_len, FRAME_HEADER_LEN);
166 if (ret < 0 || act_len < FRAME_HEADER_LEN)
167 goto quit_stream;
168 /* size is read from 4 bytes starting 0x40, little endian */
169 bytes_left = buffer[0x40]|(buffer[0x41]<<8)|(buffer[0x42]<<16)
170 |(buffer[0x43]<<24);
171 PDEBUG(D_STREAM, "bytes_left = 0x%x", bytes_left);
172 /* We keep the header. It has other information, too. */
173 packet_type = FIRST_PACKET;
174 gspca_frame_add(gspca_dev, packet_type,
175 buffer, FRAME_HEADER_LEN);
176 while (bytes_left > 0 && gspca_dev->dev) {
^^^^^^^^^^^^^^
New check.
177 data_len = bytes_left > SQ905C_MAX_TRANSFER ?
178 SQ905C_MAX_TRANSFER : bytes_left;
179 ret = usb_bulk_msg(gspca_dev->dev,
180 usb_rcvbulkpipe(gspca_dev->dev, 0x81),
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Some more dereferences here.
181 buffer, data_len, &act_len,
182 SQ905C_DATA_TIMEOUT);
183 if (ret < 0 || act_len < data_len)
184 goto quit_stream;
185 PDEBUG(D_STREAM,
186 "Got %d bytes out of %d for frame",
187 data_len, bytes_left);
188 bytes_left -= data_len;
189 if (bytes_left == 0)
190 packet_type = LAST_PACKET;
191 else
192 packet_type = INTER_PACKET;
193 gspca_frame_add(gspca_dev, packet_type,
194 buffer, data_len);
195 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [media] gspca: Fix locking issues related to suspend/resume
2012-07-19 12:13 [media] gspca: Fix locking issues related to suspend/resume Dan Carpenter
@ 2012-09-09 21:59 ` Hans de Goede
0 siblings, 0 replies; 2+ messages in thread
From: Hans de Goede @ 2012-09-09 21:59 UTC (permalink / raw)
To: Dan Carpenter; +Cc: hans.verkuil, linux-media
Hi,
Thanks for the report, it turns out that the checking for gspca_dev->dev,
rather then gspca_dev->present (which is a mod I made on top of
Hans Verkuil's orginal patch), turns out to be a bad idea in general
as its racy, this race has been fixed by this commit:
http://git.linuxtv.org/hgoede/gspca.git/commitdiff/a454f0811950742475b76dbf5ac10876e48ffaaa
Which should also appease the static checker warnings.
Thanks & Regards,
Hans
On 07/19/2012 02:13 PM, Dan Carpenter wrote:
> Hello Hans Verkuil,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 254902b01d2a: "[media] gspca: Fix locking issues related to
> suspend/resume" from May 6, 2012, leads to the following Smatch
> complaint:
>
> drivers/media/video/gspca/sq905c.c:176 sq905c_dostream()
> warn: variable dereferenced before check 'gspca_dev->dev' (see line 180)
>
> drivers/media/video/gspca/sq905c.c
> 158 /* Request the header, which tells the size to download */
> 159 ret = usb_bulk_msg(gspca_dev->dev,
> 160 usb_rcvbulkpipe(gspca_dev->dev, 0x81),
> ^^^^^^^^^^^^^^
> Derereference inside the calls to usb_bulk_msg() and usb_rcvbulkpipe().
>
> 161 buffer, FRAME_HEADER_LEN, &act_len,
> 162 SQ905C_DATA_TIMEOUT);
> 163 PDEBUG(D_STREAM,
> 164 "Got %d bytes out of %d for header",
> 165 act_len, FRAME_HEADER_LEN);
> 166 if (ret < 0 || act_len < FRAME_HEADER_LEN)
> 167 goto quit_stream;
> 168 /* size is read from 4 bytes starting 0x40, little endian */
> 169 bytes_left = buffer[0x40]|(buffer[0x41]<<8)|(buffer[0x42]<<16)
> 170 |(buffer[0x43]<<24);
> 171 PDEBUG(D_STREAM, "bytes_left = 0x%x", bytes_left);
> 172 /* We keep the header. It has other information, too. */
> 173 packet_type = FIRST_PACKET;
> 174 gspca_frame_add(gspca_dev, packet_type,
> 175 buffer, FRAME_HEADER_LEN);
> 176 while (bytes_left > 0 && gspca_dev->dev) {
> ^^^^^^^^^^^^^^
> New check.
>
> 177 data_len = bytes_left > SQ905C_MAX_TRANSFER ?
> 178 SQ905C_MAX_TRANSFER : bytes_left;
> 179 ret = usb_bulk_msg(gspca_dev->dev,
> 180 usb_rcvbulkpipe(gspca_dev->dev, 0x81),
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Some more dereferences here.
>
> 181 buffer, data_len, &act_len,
> 182 SQ905C_DATA_TIMEOUT);
> 183 if (ret < 0 || act_len < data_len)
> 184 goto quit_stream;
> 185 PDEBUG(D_STREAM,
> 186 "Got %d bytes out of %d for frame",
> 187 data_len, bytes_left);
> 188 bytes_left -= data_len;
> 189 if (bytes_left == 0)
> 190 packet_type = LAST_PACKET;
> 191 else
> 192 packet_type = INTER_PACKET;
> 193 gspca_frame_add(gspca_dev, packet_type,
> 194 buffer, data_len);
> 195 }
>
> regards,
> dan carpenter
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2012-09-09 21:58 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-19 12:13 [media] gspca: Fix locking issues related to suspend/resume Dan Carpenter
2012-09-09 21:59 ` Hans de Goede
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.