NTFS-3G development discussion and problem submission

Bogdan | 4 Jun 2012 11:34

Picon

[PATCH] Changes for utils.c, ntfswipe.c

Hello everybody.

 I've prepared a small patch for utils.c and ntfswipe.c:
- in utils.c, checking for allocated blocks is fixed to pre-fetch
data on the first run,
- in ntfswipe.c, wiping file tails in enhanced and wiping undelete
data is introduced.

 Additionally, a small correction is added to the layout.h file
(change from floating-point constant to long long), so that there
are no warnings when ntfsck.c is being compiled. You can make this
conditional, there are autoconf tests that check for support of the
"long long" type in the C compiler.

 It would be nice if you could make the "list.h" header a part of
the public API/development package, with "NTFS_" or "ntfs_" prefixes
on the symbols.

 Please reply also to my e-mail address (not only to the list) when
replying to this mail.

 If you can see this mail, then it means you have to subscribe to the
mailing list to be able to post messages (while on your website you
said this list is open).

--

-- 
Pozdrawiam/Regards - Bogdan                     (GNU/Linux & FreeDOS)
Kurs asemblera x86 (DOS, GNU/Linux):http://rudy.mif.pg.gda.pl/~bogdro
Grupy dyskusyjne o asm:  pl.comp.lang.asm alt.pl.asm alt.pl.asm.win32
www.Xiph.org   www.TorProject.org   Soft (EN): miniurl.pl/bogdro-soft

(Continue reading)

Jean-Pierre André | 4 Jun 2012 22:04

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

Hi,

Bogdan wrote:
> Hello everybody.
>
>   I've prepared a small patch for utils.c and ntfswipe.c:
> - in utils.c, checking for allocated blocks is fixed to pre-fetch
> data on the first run,
>    

-       if ((lcn < bmplcn)
+       if ((bmplcn < 0) || (lcn < bmplcn)
             || (lcn >= (long long)(bmplcn + (sizeof(buffer) << 3)))) {

Can you explain this one ?

AFAIK lcn is always >= 0 there, and bmplcn can only be
negative when it is == -(sizeof(buffer) << 3), so the last
condition is true when bmplcn is negative.
So the change does nothing.

> - in ntfswipe.c, wiping file tails in enhanced and wiping undelete
> data is introduced.
>    

This a long one, I will check later.

>   Additionally, a small correction is added to the layout.h file
> (change from floating-point constant to long long), so that there
> are no warnings when ntfsck.c is being compiled. You can make this

(Continue reading)

Bogdan | 5 Jun 2012 10:52

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

W dniu 04.06.2012 22:04, Jean-Pierre André wrote:
> Hi,
> 
> Bogdan wrote:
>> Hello everybody.
>>
>>   I've prepared a small patch for utils.c and ntfswipe.c:
>> - in utils.c, checking for allocated blocks is fixed to pre-fetch
>> data on the first run,
>>    
> 
> -       if ((lcn < bmplcn)
> +       if ((bmplcn < 0) || (lcn < bmplcn)
>             || (lcn >= (long long)(bmplcn + (sizeof(buffer) << 3)))) {
> 
> Can you explain this one ?
> 
> AFAIK lcn is always >= 0 there, and bmplcn can only be
> negative when it is == -(sizeof(buffer) << 3), so the last
> condition is true when bmplcn is negative.
> So the change does nothing.

 There are 2 issues here. The clue issue is the "(bmplcn < 0)"
condition, which makes us fill the buffer on the first run.
 The second issue is the second condition (the one you're talking
about). Yes, "-(sizeof(buffer) << 3)" makes bmplcn negative, so it
looks like this condition will always be the same as "lcn => 0". But
there is one small detail that makes the change: bmplcn is STATIC. It
holds its value between calls. The second condition checks if "lcn"
doesn't fall beyond the current buffer.

(Continue reading)

Jean-Pierre André | 5 Jun 2012 11:39

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

Hi,

Bogdan wrote:
> W dniu 04.06.2012 22:04, Jean-Pierre André wrote:
>    
>> Hi,
>>
>> Bogdan wrote:
>>      
>>> Hello everybody.
>>>
>>>    I've prepared a small patch for utils.c and ntfswipe.c:
>>> - in utils.c, checking for allocated blocks is fixed to pre-fetch
>>> data on the first run,
>>>
>>>        
>> -       if ((lcn<  bmplcn)
>> +       if ((bmplcn<  0) || (lcn<  bmplcn)
>>              || (lcn>= (long long)(bmplcn + (sizeof(buffer)<<  3)))) {
>>
>> Can you explain this one ?
>>
>> AFAIK lcn is always>= 0 there, and bmplcn can only be
>> negative when it is == -(sizeof(buffer)<<  3), so the last
>> condition is true when bmplcn is negative.
>> So the change does nothing.
>>      
>   There are 2 issues here. The clue issue is the "(bmplcn<  0)"
> condition, which makes us fill the buffer on the first run.
>

(Continue reading)

Bogdan | 5 Jun 2012 15:07

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

On 05.06.2012 11:39, Jean-Pierre André wrote:
> Hi,
> 
> Bogdan wrote:
>> W dniu 04.06.2012 22:04, Jean-Pierre André wrote:
>>   
>>> Hi,
>>>
>>> Bogdan wrote:
>>>     
>>>> Hello everybody.
>>>>
>>>>    I've prepared a small patch for utils.c and ntfswipe.c:
>>>> - in utils.c, checking for allocated blocks is fixed to pre-fetch
>>>> data on the first run,
>>>>
>>>>        
>>> -       if ((lcn<  bmplcn)
>>> +       if ((bmplcn<  0) || (lcn<  bmplcn)
>>>              || (lcn>= (long long)(bmplcn + (sizeof(buffer)<<  3)))) {
>>>
>>> Can you explain this one ?
>>>
>>> AFAIK lcn is always>= 0 there, and bmplcn can only be
>>> negative when it is == -(sizeof(buffer)<<  3), so the last
>>> condition is true when bmplcn is negative.
>>> So the change does nothing.
>>>      
>>   There are 2 issues here. The clue issue is the "(bmplcn<  0)"
>> condition, which makes us fill the buffer on the first run.

(Continue reading)

Jean-Pierre André | 5 Jun 2012 09:55

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

Hi,

Bogdan wrote:
> Hello everybody.
>
>   I've prepared a small patch for utils.c and ntfswipe.c:
> - in utils.c, checking for allocated blocks is fixed to pre-fetch
> data on the first run,
> - in ntfswipe.c, wiping file tails in enhanced and wiping undelete
> data is introduced.
>    

Please include your patch to ntfswipe.h (adding the field undel)

I had difficulties compiling your patch, because it is not portable :

static unsigned const int patterns[NPAT] =

This is not portable, change to "static const unsigned int"

const int NTFS_BUF_SIZE2 = 8192;
char buf[NTFS_BUF_SIZE2];

This is not portable, change to a good old "#define"

const int NTFS_BUF_SIZE2 = 4096;

same, and as this is a different value, an explanation to what both
values mean may be useful.

(Continue reading)

Bogdan | 5 Jun 2012 15:08

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

W dniu 05.06.2012 09:55, Jean-Pierre André pisze:
> Hi,
> 
> Bogdan wrote:
>> Hello everybody.
>>
>>   I've prepared a small patch for utils.c and ntfswipe.c:
>> - in utils.c, checking for allocated blocks is fixed to pre-fetch
>> data on the first run,
>> - in ntfswipe.c, wiping file tails in enhanced and wiping undelete
>> data is introduced.
>>    
> 
> Please include your patch to ntfswipe.h (adding the field undel)

 Right, I forgot that. Sorry.

> I had difficulties compiling your patch,

 What compiler are you using?

> because it is not portable :
> 
> static unsigned const int patterns[NPAT] =
> 
> This is not portable, change to "static const unsigned int"

 Done.

> const int NTFS_BUF_SIZE2 = 8192;

(Continue reading)

Jean-Pierre André | 5 Jun 2012 22:06

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

Hi again,

Bogdan wrote:

>> const int NTFS_BUF_SIZE2 = 8192;
>> char buf[NTFS_BUF_SIZE2];
>>
>> This is not portable, change to a good old "#define"
>>
>> const int NTFS_BUF_SIZE2 = 4096;
>>
>> same, and as this is a different value, an explanation to what both
>> values mean may be useful.
>>      
>   Yes, it would be. You may ask the author, because these declarations
> aren't a part of my patch and I don't know why there are 2 separate
> variables with different values.
>    

Oh, sorry for the confusion.

>> ret = ntfs_rl_pread(vol, na->rl, offset, 2,&block_size);
>> block_size = le16_to_cpu(block_size);
>>
>> This creates confusion, as block_size has a couple of representations.
>> Please use another variable such as :
>>
>> le16 block_size_le;
>> ret = ntfs_rl_pread(vol, na->rl, offset, 2,&block_size_le);
>> block_size = le16_to_cpu(block_size_le);

(Continue reading)

Jean-Pierre André | 6 Jun 2012 16:40

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

Hi,

I suspect problems while running ntfswipe with the new
options :

With option -s, I got no output after four minutes, with
the cpu used at 100%. Is it normal, should I wait longer ?

With option -t, I got internal errors on compressed files.
(details on an x86_64 attached).

The same test on big-endian computers hangs on inode
131 without outputting the error.

Can you check what is going on ?

Regards

Jean-Pierre

Bogdan wrote:
> W dniu 05.06.2012 09:55, Jean-Pierre André pisze:
>    
>> Hi,
>>
>> Bogdan wrote:
>>      
>>> Hello everybody.
>>>
>>>    I've prepared a small patch for utils.c and ntfswipe.c:

(Continue reading)

Jean-Pierre André | 6 Jun 2012 17:16

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

Hi again,

Jean-Pierre André wrote:
> Hi,
>
> I suspect problems while running ntfswipe with the new
> options :
>
> With option -s, I got no output after four minutes, with
> the cpu used at 100%. Is it normal, should I wait longer ?

An update on this one : the problem appears to be in the
following loop :

                 do
                 {
#if (!defined __STRICT_ANSI__) && (defined HAVE_RANDOM)
                         i = (size_t) (random () % NPAT);
#else
                         i = (size_t) (rand () % NPAT);
#endif
                 }
                 while ( selected[i] == 1 );

Apparently i gets random valid values, but selected[i] is always 1.
Should not there be an emergency exit when a pathological condition
arise ?

Side note : there was no srandom().

(Continue reading)

Bogdan | 7 Jun 2012 14:35

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

On 06.06.2012 16:40, Jean-Pierre André wrote:
> Hi,
> 
> I suspect problems while running ntfswipe with the new
> options :
> 
> With option -s, I got no output after four minutes, with
> the cpu used at 100%. Is it normal, should I wait longer ?
> 
> With option -t, I got internal errors on compressed files.
> (details on an x86_64 attached).
> 
> The same test on big-endian computers hangs on inode
> 131 without outputting the error.
> 
> Can you check what is going on ?

 First problem fixed. Patterns are now de-selected when all have been
used.
 About the second problem: apply the attached patch over the previous
one and check if the internal errors have the "Extents:" comment in
front. If no, then this is not a problem of my patch. If yes, then
it's hard to say. I'm doing everything the same way as above
(non-extents), but perhaps I don't know something that you know
(perhaps extents should be treated in a different way?). Does the
erroneous function (ntfs_attr_map_whole_runlist) work on extents in
compressed files in other places?
 How can I create a compressed file to check this on my system?

--

--

(Continue reading)

Jean-Pierre André | 7 Jun 2012 15:54

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

Hi,

Bogdan wrote:
> On 06.06.2012 16:40, Jean-Pierre André wrote:
>    
>> Hi,
>>
>> I suspect problems while running ntfswipe with the new
>> options :
>>
>> With option -s, I got no output after four minutes, with
>> the cpu used at 100%. Is it normal, should I wait longer ?
>>
>> With option -t, I got internal errors on compressed files.
>> (details on an x86_64 attached).
>>
>> The same test on big-endian computers hangs on inode
>> 131 without outputting the error.
>>
>> Can you check what is going on ?
>>      
>   First problem fixed. Patterns are now de-selected when all have been
> used.
>    

Still running a long time.

There is something wrong with unallocated clusters :

When starting the following loop in destroy_record :

(Continue reading)

Bogdan | 8 Jun 2012 13:07

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

On 07.06.2012 15:54, Jean-Pierre André wrote:
> Hi,
> 
> Bogdan wrote:
>> On 06.06.2012 16:40, Jean-Pierre André wrote:
>>   
>>> Hi,
>>>
>>> I suspect problems while running ntfswipe with the new
>>> options :
>>>
>>> With option -s, I got no output after four minutes, with
>>> the cpu used at 100%. Is it normal, should I wait longer ?
>>>
>>> With option -t, I got internal errors on compressed files.
>>> (details on an x86_64 attached).
>>>
>>> The same test on big-endian computers hangs on inode
>>> 131 without outputting the error.
>>>
>>> Can you check what is going on ?
>>>      
>>   First problem fixed. Patterns are now de-selected when all have been
>> used.
>>    
> 
> Still running a long time.
> 
> There is something wrong with unallocated clusters :
>

(Continue reading)

Jean-Pierre André | 8 Jun 2012 15:07

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

Hi,

Bogdan wrote:
>   Can't replicate this. I'm creating the filesystem as follows:
>
> dd if=/dev/zero of=../../dysk_ntfs bs=1M count=20
> chown bogdan:bogdan ../../dysk_ntfs
> /sbin/mkfs.ntfs -F ../../dysk_ntfs
>

How can you check the wiping, if you clear the partition
initially ? Should you not use if=/dev/random ?

> [ ! -d /home/bogdan/siemens/ ]&&  mkdir /home/bogdan/siemens/
>
>
> mount -o loop,compression ../../dysk_ntfs -t ntfs-fuse
> /home/bogdan/siemens/						&&  \
> cp ../../test-* /home/bogdan/siemens/			&&  \
> mkdir /home/bogdan/siemens/dir1				&&  \
> setfattr -h -v 0x00080000 -n system.ntfs_attrib
> /home/bogdan/siemens/dir1						&&  \
> cp ../../test-* /home/bogdan/siemens/dir1		&&  \
> md5sum /home/bogdan/siemens/t*
> rm -f /home/bogdan/siemens/test-D
> rm -f /home/bogdan/siemens/dir1/test-D
> umount /home/bogdan/siemens/
>
>
> (line breaks in the interesting part should be only after "&&  \").

(Continue reading)

Bogdan | 9 Jun 2012 16:53

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

On 08.06.2012 15:07, Jean-Pierre André wrote:
> Hi,
> 
> Bogdan wrote:
>>   Can't replicate this. I'm creating the filesystem as follows:
>>
>> dd if=/dev/zero of=../../dysk_ntfs bs=1M count=20
>> chown bogdan:bogdan ../../dysk_ntfs
>> /sbin/mkfs.ntfs -F ../../dysk_ntfs
>>
> 
> How can you check the wiping, if you clear the partition
> initially ? Should you not use if=/dev/random ?

 I'm using specified bytes (not zero) for wiping and search the
filesystem for my patterns using a hex editor or grep.

[...]

> Maybe you retry with a bigger text file which can be compressed
> and requires several extents (several MB).
> Also make sure the compression is effective by comparing the
> results of :
> 
> du /home/bogdan/siemens/t*
> and
> du --apparent-size /home/bogdan/siemens/t*

du -hs /home/bogdan/siemens/dir1/*
512     /home/bogdan/siemens/dir1/test-A

(Continue reading)

Jean-Pierre André | 9 Jun 2012 19:22

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c


Bogdan wrote:
> On 08.06.2012 15:07, Jean-Pierre André wrote:
>    

[...]
>> Not fully, I still get an error :
>>
>> Extents: Can't map ntfs_runlist (inode 22865232)
>>
>> (and of course, there are fewer than 22865232 nodes
>> on this partition).
>>      
>   The displayed value was a pointer, fixed now. Apply over previous
> patches.
>    

Same results as before. However I now get a valid inode
number :

Extents: Can't map ntfs_runlist (inode 197)

To be precise, a big-endian computer chokes before
reaching that point, but while processing the same
inode (131, which 197 is a part of).

>   The loop should take (and wipe) the data attribute of each extent.
>    

In the above situation, there is a single data attribute

(Continue reading)

Bogdan | 10 Jun 2012 14:23

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

On 09.06.2012 19:22, Jean-Pierre André wrote:
> 
> 
> Bogdan wrote:
>> On 08.06.2012 15:07, Jean-Pierre André wrote:
>>    
> 
> [...]
>>> Not fully, I still get an error :
>>>
>>> Extents: Can't map ntfs_runlist (inode 22865232)
>>>
>>> (and of course, there are fewer than 22865232 nodes
>>> on this partition).
>>>      
>>   The displayed value was a pointer, fixed now. Apply over previous
>> patches.
>>    
> 
> Same results as before. However I now get a valid inode
> number :
> 
> Extents: Can't map ntfs_runlist (inode 197)

 That's good, the last patch corrected only displaying the inode number.

> To be precise, a big-endian computer chokes before
> reaching that point, but while processing the same
> inode (131, which 197 is a part of).

(Continue reading)

Jean-Pierre André | 10 Jun 2012 14:54

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

Hi again,

Bogdan wrote:
> On 09.06.2012 19:22, Jean-Pierre André wrote:
>    
>> To be precise, a big-endian computer chokes before
>> reaching that point, but while processing the same
>> inode (131, which 197 is a part of).
>>      
>   That's bad. Even if I'm doing something very wrong, the library
> should manage to at least return an error.
>    

Actually it does, it was a matter of configuring the
log levels.

[...]
>    From all this, I'm beginning to think that my change is not needed.
> The first call to ntfs_attr_map_whole_runlist() (before "close_attr:")
> should read the whole attribute, no matter how many extents it is in,
> right? If so, the first calls to wipe_compressed_attribute() or
> wipe_attribute() will wipe the tail before reaching the "close_attr:"
> point, so searching for and wiping the extents is not needed. Is this
> correct? Seems to work even for the "big" file (I can see my marker
> bytes after the content).
>    

Most likely, the wiping is fully done while examining the
base inode.

(Continue reading)

Jean-Pierre André | 10 Jun 2012 11:08

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

Hi,

Bogdan wrote:
> On 08.06.2012 15:07, Jean-Pierre André wrote:
>    
>> Hi,
>>
>> Bogdan wrote:
>>      

More on this :

I found a lot of time is spent in ntfs_rl_pread() and
ntfs_rl_pwrite() because you feed them with the beginning
of the runlist, and they have to search the relevant entry.
You can avoid this by feeding them with the run which
immediately follows the last hole encountered :

Before entering the loop :
runlist_element *restart = na->rl;

At the end of loop (before rlc++) :
if (rlc->lcn == LCN_HOLE) restart = rlc;

replace
ret = ntfs_rl_pread(vol, na->rl, offset,...
by
ret = ntfs_rl_pread(vol, restart,
     offset - (restart->vcn << vol->cluster_size_bits),...

(Continue reading)

Bogdan | 12 Jun 2012 11:55

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

On 10.06.2012 11:08, Jean-Pierre André wrote:
> Hi,
> 
> Bogdan wrote:
>> On 08.06.2012 15:07, Jean-Pierre André wrote:
>>   
>>> Hi,
>>>
>>> Bogdan wrote:
>>>      
> 
> More on this :
> 
> I found a lot of time is spent in ntfs_rl_pread() and
> ntfs_rl_pwrite() because you feed them with the beginning
> of the runlist, and they have to search the relevant entry.
> You can avoid this by feeding them with the run which
> immediately follows the last hole encountered :
> 
> Before entering the loop :
> runlist_element *restart = na->rl;
> 
> At the end of loop (before rlc++) :
> if (rlc->lcn == LCN_HOLE) restart = rlc;
> 
> replace
> ret = ntfs_rl_pread(vol, na->rl, offset,...
> by
> ret = ntfs_rl_pread(vol, restart,
>     offset - (restart->vcn << vol->cluster_size_bits),...

(Continue reading)

Jean-Pierre André | 13 Jun 2012 12:01

Picon

Re: [PATCH] Changes for utils.c, ntfswipe.c

Hi,

Bogdan wrote:
>
>   Done (including the "+1" from your last mail). This also isn't my
> code (I'm only using this function), but the changes were simple.
>    

Oh, I have lost track of where each part of the code came
from... Sorry for the confusion.

>> There is a single tail, this should not be in a loop.
>>      
>   Can't do that. The "size" and "offset" variables can be changed on
> each iteration, so they have to be re-set to the correct values.
>    

Actually no change is needed. The tail is only wiped
once during the last iteration in the loop.

>> In the main wipe_tail function, you only process the
>> unnamed data attribute :
>>
>> na = ntfs_attr_open(ni, AT_DATA, AT_UNNAMED, 0);
>>
>> But there can also be named data attributes, you should
>> do a "while (!ntfs_attr_lookup(AT_DATA, ... ))"
>>      
>   The ntfs_attr_lookup() function requires a ntfs_attr_search_ctx
> variable, which can be obtained by calling

(Continue reading)

Return

Return to gmane.comp.file-systems.ntfs-3g.devel.

Project Web Page

NTFS-3G development discussion and problem submission

Search Archive

Language

Change language

Options

Current view: Threads only / Showing only 20 lines / Not hiding cited text.
Change to All messages, whole messages, or hide cited text.

Post a message
NNTP Newsgroup
Classic Gmane web interface
XML

XML

RSS Feed
List Information

About Gmane