Opened 17 years ago

Last modified 13 years ago

#751 new defect

NetBSD-specific code in stream/vcd_read_nbsd.h has problems

Reported by: svs@… Owned by: reimar
Priority: normal Component: streaming
Version: HEAD Severity: normal
Keywords: Cc:
Blocked By: Blocking:
Reproduced by developer: no Analyzed by developer: no

Description

a) Sector address conversion routines do not take into account
150-sector offset (MSF address 0:0.0 is LBA -150); thus, wrong locations
on disc are read. Usual failure mode: the disc will be played without
video. Surprisingly, some discs will still play correctly.

b) The "error" field of struct scsireq is unused by NetBSD kernel and
may contain random data.

Attachments (2)

patch-netbsd-vcd (1.1 KB ) - added by svs@… 17 years ago.
proposed patch
751.patch (1.0 KB ) - added by svs@… 16 years ago.
proposed fix

Download all attachments as: .zip

Change History (18)

by svs@…, 17 years ago

Attachment: patch-netbsd-vcd added

proposed patch

comment:1 by svs@…, 17 years ago

comment:2 by r_togni@…, 17 years ago

Component: demuxerstreaming

Patch forwarded to the mplayer-dev-eng mailinglist.
I have no clue about NetBSD and I can't test the patch.

comment:3 by svs@…, 17 years ago

I have posted this patch to devel list earlier, but got no usable response. See http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/2006-October/046670.html.

comment:4 by svs@…, 16 years ago

Should I post a review request to -devel again?

comment:5 by reimar, 16 years ago

Owner: changed from r_togni@… to Reimar.Doeffinger@…

The code no longer exists in that form, it has been merged with the FreeBSD code.
The 150 sector offset has been fixed, and error field has never caused problems.
Note that "The "error" field of struct scsireq is unused by NetBSD kernel" and "may contain random data" contradict each other, if it is really unused by the kernel, the kernel would not change it so it will keep its initialization value of 0.
If the kernel fills in random data, I'd be really worried, a kernel should never leak unspecified data to user programs.

comment:6 by reimar, 16 years ago

Resolution: worksforme
Status: newclosed

The current code works for me, reopen if it causes problems or you have "proof" that it could cause problems.

comment:7 by svs@…, 16 years ago

Resolution: worksforme
Status: closedreopened

New code causes memory corruption (only on NetBSD), because

sc.datalen = 2328 is larger than VCD_SECTOR_DATA (2324).

comment:8 by reimar, 16 years ago

That part of the "new" code was like that always (esp. also long before you submitted your patch).
So does sc.datalen = VCD_SECTOR_DATA work or how is this supposed to be done on NetBSD?

comment:9 by svs@…, 16 years ago

Owner: changed from Reimar.Doeffinger@… to r_togni@…
Status: reopenednew

It worked before because buffer was larger (VCD_SECTOR_SIZE = 2352). I don't remember why 2328 was required, but will find out.

comment:10 by reimar, 16 years ago

Just FYI, I changed it to sc.datalen = VCD_SECTOR_DATA , if this does not work it must be done differently with an extra buffer and memcpy.
While it did work previously without this, it was still wrong (against the stream API).

comment:11 by svs@…, 16 years ago

MMC-3 spec [1] says:

Only sectors (see Table 350) that have a user data field of 2 324 shall be returned. <...> NOTE: 4 spare bytes are included making the total data length returned 2 328 bytes/sector.

So it seems I'd have to use memcpy after all.

[1] http://www.t10.org/ftp/t10/drafts/mmc3/mmc3r10g.pdf

by svs@…, 16 years ago

Attachment: 751.patch added

proposed fix

comment:12 by svs@…, 16 years ago

attachments.isobsolete: 01

comment:13 by svs@…, 16 years ago

Please review the patch that I posted.

comment:14 by svs@…, 15 years ago

Please review the patch that I posted

comment:15 by svs@…, 15 years ago

The patch in question was included into NetBSD pkgsrc package. Please review it.

comment:16 by compn, 13 years ago

Owner: changed from r_togni@… to reimar
Note: See TracTickets for help on using tickets.