|
From: Yuval <uv...@de...> - 2006-10-28 23:16:15
|
Changes by: uvman
Update of /cvs/linux-ntfs/ntfsprogs/libntfs
In directory delta357:/tmp/cvs-serv11764/libntfs
Modified Files:
compress.c
Log Message:
memmove() already deals with overlap, is optimized, is ANSI C and only takes one line to call.
Index: compress.c
===================================================================
RCS file: /cvs/linux-ntfs/ntfsprogs/libntfs/compress.c,v
retrieving revision 1.16
retrieving revision 1.17
diff -u -p -r1.16 -r1.17
--- compress.c 26 Oct 2006 19:10:05 -0000 1.16
+++ compress.c 28 Oct 2006 23:15:44 -0000 1.17
@@ -166,7 +166,7 @@ do_next_tag:
tag = *cb++;
/* Parse the eight tokens described by the tag. */
for (token = 0; token < 8; token++, tag >>= 1) {
- u16 lg, pt, length, max_non_overlap;
+ u16 lg, pt, length;
register u16 i;
u8 *dest_back_addr;
@@ -214,27 +214,8 @@ do_next_tag:
/* Verify destination is in range. */
if (dest + length > dest_sb_end)
goto return_overflow;
- /* The number of non-overlapping bytes. */
- max_non_overlap = dest - dest_back_addr;
- if (length <= max_non_overlap) {
- /* The byte sequence doesn't overlap, just copy it. */
- memcpy(dest, dest_back_addr, length);
- /* Advance destination pointer. */
- dest += length;
- } else {
- /*
- * The byte sequence does overlap, copy non-overlapping
- * part and then do a slow byte by byte copy for the
- * overlapping part. Also, advance the destination
- * pointer.
- */
- memcpy(dest, dest_back_addr, max_non_overlap);
- dest += max_non_overlap;
- dest_back_addr += max_non_overlap;
- length -= max_non_overlap;
- while (length--)
- *dest++ = *dest_back_addr++;
- }
+ /* memmove() is safe with overlapping blocks. */
+ memmove(dest, dest_back_addr, length);
/* Advance source position and continue with the next token. */
cb += 2;
}
|
|
From: Anton A. <ai...@ca...> - 2006-10-29 07:55:51
|
This is b0rked, please revert it ASAP. You cannot use memmove().
Please do not meddle with code you do not understand. (Given your log
message you clearly do not understand it...)
Anton
On Sun, 29 Oct 2006, Yuval wrote:
>
> Changes by: uvman
>
> Update of /cvs/linux-ntfs/ntfsprogs/libntfs
> In directory delta357:/tmp/cvs-serv11764/libntfs
>
> Modified Files:
> compress.c
> Log Message:
> memmove() already deals with overlap, is optimized, is ANSI C and only takes one line to call.
>
> Index: compress.c
> ===================================================================
> RCS file: /cvs/linux-ntfs/ntfsprogs/libntfs/compress.c,v
> retrieving revision 1.16
> retrieving revision 1.17
> diff -u -p -r1.16 -r1.17
> --- compress.c 26 Oct 2006 19:10:05 -0000 1.16
> +++ compress.c 28 Oct 2006 23:15:44 -0000 1.17
> @@ -166,7 +166,7 @@ do_next_tag:
> tag = *cb++;
> /* Parse the eight tokens described by the tag. */
> for (token = 0; token < 8; token++, tag >>= 1) {
> - u16 lg, pt, length, max_non_overlap;
> + u16 lg, pt, length;
> register u16 i;
> u8 *dest_back_addr;
>
> @@ -214,27 +214,8 @@ do_next_tag:
> /* Verify destination is in range. */
> if (dest + length > dest_sb_end)
> goto return_overflow;
> - /* The number of non-overlapping bytes. */
> - max_non_overlap = dest - dest_back_addr;
> - if (length <= max_non_overlap) {
> - /* The byte sequence doesn't overlap, just copy it. */
> - memcpy(dest, dest_back_addr, length);
> - /* Advance destination pointer. */
> - dest += length;
> - } else {
> - /*
> - * The byte sequence does overlap, copy non-overlapping
> - * part and then do a slow byte by byte copy for the
> - * overlapping part. Also, advance the destination
> - * pointer.
> - */
> - memcpy(dest, dest_back_addr, max_non_overlap);
> - dest += max_non_overlap;
> - dest_back_addr += max_non_overlap;
> - length -= max_non_overlap;
> - while (length--)
> - *dest++ = *dest_back_addr++;
> - }
> + /* memmove() is safe with overlapping blocks. */
> + memmove(dest, dest_back_addr, length);
> /* Advance source position and continue with the next token. */
> cb += 2;
> }
>
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/
|
|
From: Anton A. <ai...@ca...> - 2006-10-29 09:09:55
|
I should have explained what it is you are not understanding... (-;
memmove() is assembler optimized and does a "4-bytes-at-a-time-copy" and
takes care to set the direction bit in the correct direction depending on
whether src > dest or src < dst. This is fine and good in itself when src
exists and overlaps destination.
The decompression code in NTFS is totally unlike the above. src does NOT
exist! But yes it does overlap with dst. So since src does not exist you
cannot use memmove() otherwise it copies garbage!
The encoding is: copy X times the byte sequence Z thus you have to store
the sequence Z at position Y, Y+1, Y+2, etc. Thus you are only generating
the src as you are writing the destination itself. Thus it can only be
done safely byte-by-byte without special-casing on the size of the
sequence Z. Thus memmove() DOES NOT WORK.
Best regards,
Anton
On Sun, 29 Oct 2006, Anton Altaparmakov wrote:
> This is b0rked, please revert it ASAP. You cannot use memmove().
>
> Please do not meddle with code you do not understand. (Given your log
> message you clearly do not understand it...)
>
> Anton
>
> On Sun, 29 Oct 2006, Yuval wrote:
>
> >
> > Changes by: uvman
> >
> > Update of /cvs/linux-ntfs/ntfsprogs/libntfs
> > In directory delta357:/tmp/cvs-serv11764/libntfs
> >
> > Modified Files:
> > compress.c
> > Log Message:
> > memmove() already deals with overlap, is optimized, is ANSI C and only takes one line to call.
> >
> > Index: compress.c
> > ===================================================================
> > RCS file: /cvs/linux-ntfs/ntfsprogs/libntfs/compress.c,v
> > retrieving revision 1.16
> > retrieving revision 1.17
> > diff -u -p -r1.16 -r1.17
> > --- compress.c 26 Oct 2006 19:10:05 -0000 1.16
> > +++ compress.c 28 Oct 2006 23:15:44 -0000 1.17
> > @@ -166,7 +166,7 @@ do_next_tag:
> > tag = *cb++;
> > /* Parse the eight tokens described by the tag. */
> > for (token = 0; token < 8; token++, tag >>= 1) {
> > - u16 lg, pt, length, max_non_overlap;
> > + u16 lg, pt, length;
> > register u16 i;
> > u8 *dest_back_addr;
> >
> > @@ -214,27 +214,8 @@ do_next_tag:
> > /* Verify destination is in range. */
> > if (dest + length > dest_sb_end)
> > goto return_overflow;
> > - /* The number of non-overlapping bytes. */
> > - max_non_overlap = dest - dest_back_addr;
> > - if (length <= max_non_overlap) {
> > - /* The byte sequence doesn't overlap, just copy it. */
> > - memcpy(dest, dest_back_addr, length);
> > - /* Advance destination pointer. */
> > - dest += length;
> > - } else {
> > - /*
> > - * The byte sequence does overlap, copy non-overlapping
> > - * part and then do a slow byte by byte copy for the
> > - * overlapping part. Also, advance the destination
> > - * pointer.
> > - */
> > - memcpy(dest, dest_back_addr, max_non_overlap);
> > - dest += max_non_overlap;
> > - dest_back_addr += max_non_overlap;
> > - length -= max_non_overlap;
> > - while (length--)
> > - *dest++ = *dest_back_addr++;
> > - }
> > + /* memmove() is safe with overlapping blocks. */
> > + memmove(dest, dest_back_addr, length);
> > /* Advance source position and continue with the next token. */
> > cb += 2;
> > }
> >
>
>
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/
|
|
From: Yuval F. <yu...@gm...> - 2006-10-29 09:13:58
|
On 10/29/06, Anton Altaparmakov wrote: > This is b0rked, please revert it ASAP. You cannot use memmove(). Reverted. > Please do not meddle with code you do not understand. (Given your log > message you clearly do not understand it...) Open source would not work with this attitude. Review is exactly for this purpose. Mistakes can only be made by doing something. I was making sure with the C99 specification that memmove() would deal with this case. Obviously, I have not taken into account bugs in libc. As a safeguard, I called ntfscat with some large compressed files and it worked. I will open a ticket on glibc. p.s. don't use the cvs user as the addressee. I'm getting this mail from the mailing list. -- Yuval Fledel |
