I was just looking at the patches included with debian's vdr package, and there were two patches included that I had never seen before. I did not write these patches, but I just wanted to post them to see what everyone's thoughts/reactions are about them (if they should be added to core vdr or not).
Best Regards, C.
diff -ru vdr-1.3.27-eepg-orig/dvbspu.c vdr-1.3.27-eepg/dvbspu.c --- vdr-1.3.27-orig/dvbspu.c 2005-05-07 04:13:48.000000000 -0700 +++ vdr-1.3.27/dvbspu.c 2005-06-29 16:20:58.000000000 -0700 @@ -155,7 +155,7 @@ setMin(minsize[colorid].x1, xp); setMin(minsize[colorid].y1, yp); setMax(minsize[colorid].x2, xp + len - 1); - setMax(minsize[colorid].y2, yp + len - 1); + setMax(minsize[colorid].y2, yp); }
static uint8_t getBits(uint8_t * &data, uint8_t & bitf) @@ -336,6 +336,20 @@ return size; }
+static bool OsdMatchesArea(cOsd *osd, tArea &area) +{ + cBitmap *bmp = osd->GetBitmap(0); + if (!bmp) + return false; + if (bmp->Bpp() < area.bpp) + return false; + if (bmp->X0() > area.x1 || bmp->Y0() > area.y1) + return false; + if ((bmp->X0() + bmp->Width()) <= area.x2 || (bmp->Y0() + bmp->Height()) <= area.y2) + return false; + return true; +} + void cDvbSpuDecoder::Draw(void) { if (!spubmp) { @@ -366,12 +380,16 @@ }
if (bg || fg) { + int x2 = areaSize.x2; + while ((x2 - areaSize.x1 + 1) & 0x03) + x2++; + tArea Area = { areaSize.x1, areaSize.y1, x2, areaSize.y2, (fg && bg) ? 4 : 2 }; + if (osd && !OsdMatchesArea(osd, Area)) { + delete osd; + osd = NULL; + } if (osd == NULL) { osd = cOsdProvider::NewOsd(0, 0); - int x2 = areaSize.x2; - while ((x2 - areaSize.x1 + 1) & 0x03) - x2++; - tArea Area = { areaSize.x1, areaSize.y1, x2, areaSize.y2, (fg && bg) ? 4 : 2 }; osd->SetAreas(&Area, 1); }
diff -ru vdr-1.3.27-eepg-orig/dvbdevice.c vdr-1.3.27-eepg/dvbdevice.c --- vdr-1.3.27-orig/dvbdevice.c 2005-06-19 08:21:07.000000000 -0700 +++ vdr-1.3.27/dvbdevice.c 2005-06-29 15:56:57.000000000 -0700 @@ -699,7 +699,9 @@ Quality = 100;
isyslog("grabbing to %s (%s %d %d %d)", FileName, Jpeg ? "JPEG" : "PNM", Quality, vm.width, vm.height); - FILE *f = fopen(FileName, "wb"); + int fd = open(FileName, O_CREAT | O_EXCL | O_TRUNC | O_RDWR, 00640); + if (fd > -1) { + FILE *f = fdopen(fd, "wb"); if (f) { if (Jpeg) { // write JPEG file: @@ -735,6 +737,7 @@ } fclose(f); } + } else { LOG_ERROR_STR(FileName); result |= 1;
C.Y.M wrote:
- setMax(minsize[colorid].y2, yp + len - 1);
- setMax(minsize[colorid].y2, yp);
This looks like a minor performance bug fix, as the written area is a horizontal line, not a box.
+static bool OsdMatchesArea(cOsd *osd, tArea &area)
This (plus remainings of patch) re-allocates the osd if the new area doesnt fit into the old osd. This may be a bug, if this actually occurs.
As far as I understand it, the dvbspu.c translates subpictures to osd bitmaps, though I dont really know who actually uses this. Maybe DVD plugin?
The second patch is a security patch, described here: http://www.debian.org/security/2005/dsa-656
FILE *f = fopen(FileName, "wb");
int fd = open(FileName, O_CREAT | O_EXCL | O_TRUNC | O_RDWR, 00640);if (fd > -1) {FILE *f = fdopen(fd, "wb");
This seems to force creating a new GRAB file with (00640 & ~umask) access rights, while fopen always uses (00666 & ~umask). Additionally, this version fails if the GRAB file already exists. (vdradmin-am wont work with this, as the file is pre-allocated by vdradmin-am. ;) )
I dont agree with this fix, because (1) insecure SVDRP access is IMHO a security hole in any case, (2) if VDR runs properly as restricted user, there shouldnt be any critical files with write access, and (3) though the patched version cannot overwrite existing files, it still can create new files anywhere, and thats IMHO not much better.
Cheers,
Udo
I demand that Udo Richter may or may not have written...
C.Y.M wrote:
- setMax(minsize[colorid].y2, yp + len - 1);
- setMax(minsize[colorid].y2, yp);
This looks like a minor performance bug fix, as the written area is a horizontal line, not a box.
+static bool OsdMatchesArea(cOsd *osd, tArea &area)
This (plus remainings of patch) re-allocates the osd if the new area doesnt fit into the old osd. This may be a bug, if this actually occurs.
As far as I understand it, the dvbspu.c translates subpictures to osd bitmaps, though I dont really know who actually uses this. Maybe DVD plugin?
Not sure, but it's one of Reinhard Nissl's patches.
The second patch is a security patch, described here: http://www.debian.org/security/2005/dsa-656
FILE *f = fopen(FileName, "wb");
int fd = open(FileName, O_CREAT | O_EXCL | O_TRUNC | O_RDWR, 00640);if (fd > -1) {FILE *f = fdopen(fd, "wb");
This seems to force creating a new GRAB file with (00640 & ~umask) access rights, while fopen always uses (00666 & ~umask). Additionally, this version fails if the GRAB file already exists. (vdradmin-am wont work with this, as the file is pre-allocated by vdradmin-am. ;) )
I don't agree with this fix,
</AOL>.
because (1) insecure SVDRP access is IMHO a security hole in any case,
True. (Klaus?)
(2) if VDR runs properly as restricted user, there shouldn't be any critical files with write access,
True, *but* it's still possible to overwrite files which are, quite properly, owned by vdr.
and (3) though the patched version cannot overwrite existing files, it still can create new files anywhere, and thats IMHO not much better.
Agreed again. My VDR builds have used a similar patch (attached) which restricts where these files can be written for some time now; vdradmin shouldn't have a problem with it.
vdr-xine users will find a commented-out O_EXCL in xineLib.c - you should uncomment this and replace it with O_NOFOLLOW. (My package already has this patch; the official Debian package will too.)
(We still need a send-snap-as-base64 version. Both vdr and vdr-xine will require modification for this; when I last looked at this, I came to the conclusion that a file _handle_ needs to be passed to the snapshot-creation code.)
-
C.Y.M -
Darren Salt -
Udo Richter