it seems that udf_readatoffset() function does not properly handle certain offset, size combinations: offset%bsize != 0 and size in ](bsize-offset%bsize) + n*bsize, bsize + n*bsize] where n = 0, 1, 2, ... or with graphic illustration: current offset v |--------| - data that we want to read |******|&| - '*' are for good data, '&' for junk in memory |--------|--------| - data on disk |^^^^^^^^| - this is what would be read with current code |^^^^^^^^|^^^^^^^^| - this is what should be read ^ ^ ^ sector (or logical block) boundaries this happens because calculations of number of sectors to read do not take into account the fact that an additional sector may be needed because of current offset alignment. Fix: number of requested bytes passed to udf_readlblks should be increased by (offset & udfmp->bmask) bytes. For the same reasons max_size calculated in udf_bmap_internal() should be decreased by the same value, so that we do not read beyond extent end. How-To-Repeat: mount udf disk and perform something similar to large enough file: tmp$ dd if=/mnt/dvdrom/oddity/export.level-7.2005-02-01.dump.gz of=ttt bs=2000 21+1 records in 21+1 records out 42472 bytes transferred in 0.226800 secs (187266 bytes/sec) tmp$ diff /mnt/dvdrom/oddity/export.level-7.2005-02-01.dump.gz ttt Binary files /mnt/dvdrom/oddity/export.level-7.2005-02-01.dump.gz and ttt differ tmp$ dd if=/mnt/dvdrom/oddity/export.level-7.2005-02-01.dump.gz of=ttt bs=2048 20+1 records in 20+1 records out 42472 bytes transferred in 0.038471 secs (1104001 bytes/sec) tmp$ diff /mnt/dvdrom/oddity/export.level-7.2005-02-01.dump.gz ttt tmp$ file should be the same in both cases
If meaning of max_size is interpreted as maximum number of contiguous bytes that can be read starting from a given offset rather than starting from a beginning of a calculated sector number then a patch could be like the following. (Please note that currently max_size contains number of bytes in an extent to which current offset belongs, which is total nonsense since any code that calls udf_bmap_internal() has no notion of extents) --- udf_vnops.c.orig Mon Feb 7 22:59:34 2005 +++ udf_vnops.c Mon Feb 7 23:18:06 2005 @@ -1107,19 +1107,21 @@ *size = max_size; *size = min(*size, MAXBSIZE); - if ((error = udf_readlblks(udfmp, sector, *size, bp))) { + if ((error = udf_readlblks(udfmp, sector, *size + (offset & udfmp->bmask), bp))) { printf("warning: udf_readlblks returned error %d\n", error); return (error); } bp1 = *bp; - *data = (uint8_t *)&bp1->b_data[offset % udfmp->bsize]; + *data = (uint8_t *)&bp1->b_data[offset & udfmp->bmask]; return (0); } /* * Translate a file offset into a logical block and then into a physical * block. + * max_size - maximum number of bytes that can be read starting from given + * offset, not beginning of calculated sector number */ static int udf_bmap_internal(struct udf_node *node, off_t offset, daddr_t *sector, uint32_t *max_size) @@ -1172,7 +1174,7 @@ lsector = (offset >> udfmp->bshift) + ((struct short_ad *)(icb))->pos; - *max_size = GETICBLEN(short_ad, icb); + *max_size = icblen - offset; break; case 1: @@ -1196,7 +1198,7 @@ lsector = (offset >> udfmp->bshift) + ((struct long_ad *)(icb))->loc.lb_num; - *max_size = GETICBLEN(long_ad, icb); + *max_size = icblen - offset; break; case 3:
If meaning of max_size is interpreted as maximum number of contiguous bytes that can be read starting from a given offset rather than starting from a beginning of a calculated sector number then a patch could be like the following. (Please note that currently max_size contains number of bytes in an extent to which current offset belongs, which is total nonsense since any code that calls udf_bmap_internal() has no notion of extents) --- udf_vnops.c.orig Mon Feb 7 22:59:34 2005 +++ udf_vnops.c Mon Feb 7 23:18:06 2005 @@ -1107,19 +1107,21 @@ *size = max_size; *size = min(*size, MAXBSIZE); - if ((error = udf_readlblks(udfmp, sector, *size, bp))) { + if ((error = udf_readlblks(udfmp, sector, *size + (offset & udfmp->bmask), bp))) { printf("warning: udf_readlblks returned error %d\n", error); return (error); } bp1 = *bp; - *data = (uint8_t *)&bp1->b_data[offset % udfmp->bsize]; + *data = (uint8_t *)&bp1->b_data[offset & udfmp->bmask]; return (0); } /* * Translate a file offset into a logical block and then into a physical * block. + * max_size - maximum number of bytes that can be read starting from given + * offset, not beginning of calculated sector number */ static int udf_bmap_internal(struct udf_node *node, off_t offset, daddr_t *sector, uint32_t *max_size) @@ -1172,7 +1174,7 @@ lsector = (offset >> udfmp->bshift) + ((struct short_ad *)(icb))->pos; - *max_size = GETICBLEN(short_ad, icb); + *max_size = icblen - offset; break; case 1: @@ -1196,7 +1198,7 @@ lsector = (offset >> udfmp->bshift) + ((struct long_ad *)(icb))->loc.lb_num; - *max_size = GETICBLEN(long_ad, icb); + *max_size = icblen - offset; break; case 3: _______________________________________________ freebsd-bugs@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-bugs To unsubscribe, send any mail to "freebsd-bugs-unsubscribe@freebsd.org"
updated patch -- Andriy Gapon
State Changed From-To: open->feedback Is this still a problem with a recent 6.x or -current?
State Changed From-To: feedback->open Feedback received.
This problem is still present in 6.1. Updated patch is attached. -- Andriy Gapon
Responsible Changed From-To: freebsd-bugs->remko I will look at this
remko 2007-06-11 20:14:44 UTC FreeBSD src repository Modified files: sys/fs/udf udf_vnops.c Log: Correct corrupt read when the read starts at a non-aligned offset. PR: kern/77234 MFC After: 1 week Approved by: imp (mentor) Requested by: many many people Submitted by: Andriy Gapon <avg at icyb dot net dot ua> Revision Changes Path 1.66 +6 -4 src/sys/fs/udf/udf_vnops.c _______________________________________________ cvs-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/cvs-all To unsubscribe, send any mail to "cvs-all-unsubscribe@freebsd.org"
State Changed From-To: open->patched Patched in -CURRENT, MFC in a little.
remko 2007-06-26 06:59:24 UTC FreeBSD src repository Modified files: (Branch: RELENG_6) sys/fs/udf udf_vnops.c Log: MFC v 1.66 udf_vnops.c Correct corrupt read when the read starts at a non-aligned offset. PR: kern/77234 Approved by: imp (mentor) Requested by: many many people Submitted by: Andriy Gapon <avg at icyb dot net dot ua> Approved by: imp (implicit, mentor) Revision Changes Path 1.58.2.4 +6 -4 src/sys/fs/udf/udf_vnops.c _______________________________________________ cvs-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/cvs-all To unsubscribe, send any mail to "cvs-all-unsubscribe@freebsd.org"
remko 2007-06-26 07:00:39 UTC FreeBSD src repository Modified files: (Branch: RELENG_5) sys/fs/udf udf_vnops.c Log: MFC v 1.66 udf_vnops.c Correct corrupt read when the read starts at a non-aligned offset. PR: kern/77234 Approved by: imp (mentor) Requested by: many many people Submitted by: Andriy Gapon <avg at icyb dot net dot ua> Approved by: imp (implicit, mentor) Revision Changes Path 1.37.2.3 +6 -4 src/sys/fs/udf/udf_vnops.c _______________________________________________ cvs-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/cvs-all To unsubscribe, send any mail to "cvs-all-unsubscribe@freebsd.org"
State Changed From-To: patched->closed The fix had been imported into -CURRENT and a few secs ago into RELENG_5 and RELENG_6, thanks for the submission and the patience!