Opened 12 years ago

Last modified 8 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: Analyzed by developer:

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@… 12 years ago.
proposed patch
751.patch (1.0 KB) - added by svs@… 11 years ago.
proposed fix

Download all attachments as: .zip

Change History (18)

Changed 12 years ago by svs@…

proposed patch

comment:1 Changed 12 years ago by svs@…

comment:2 Changed 12 years ago by r_togni@…

  • Component changed from demuxer to streaming

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

comment:3 Changed 12 years ago by svs@…

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 Changed 11 years ago by svs@…

Should I post a review request to -devel again?

comment:5 Changed 11 years ago by reimar

  • 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 Changed 11 years ago by reimar

  • Resolution set to worksforme
  • Status changed from new to closed

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

comment:7 Changed 11 years ago by svs@…

  • Resolution worksforme deleted
  • Status changed from closed to reopened

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

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

comment:8 Changed 11 years ago by reimar

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 Changed 11 years ago by svs@…

  • Owner changed from Reimar.Doeffinger@… to r_togni@…
  • Status changed from reopened to new

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 Changed 11 years ago by reimar

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 Changed 11 years ago by svs@…

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

Changed 11 years ago by svs@…

proposed fix

comment:12 Changed 11 years ago by svs@…

  • attachments.isobsolete changed from 0 to 1

comment:13 Changed 11 years ago by svs@…

Please review the patch that I posted.

comment:14 Changed 10 years ago by svs@…

Please review the patch that I posted

comment:15 Changed 10 years ago by svs@…

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

comment:16 Changed 8 years ago by compn

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