Hi Julia,
On 11 May 2008, at 21:19, Julia Lawall wrote:
> The following code, starting at line 2591 in the file fs/ntfs/mft.c
> does
> not look right:
>
> m = map_extent_mft_record(base_ni, bit, &ni);
> if (IS_ERR(m)) {
> ntfs_error(vol->sb, "Failed to map allocated extent "
> "mft record 0x%llx.", (long long)bit);
> err = PTR_ERR(m);
> /* Set the mft record itself not in use. */
> m->flags &= cpu_to_le16(
> ~le16_to_cpu(MFT_RECORD_IN_USE));
>
> If m satisfies IS_ERR(m) it does not seem correct to dereference it.
> Other error handling code in the same function also sets m->flags,
> but not
> for the same value of m. In this case m has been redefined.
> Perhaps the
> solution is to save the previous value of m in a temporary variable so
> that the flags field can be updated here.
Thanks for that! Nice catch. In practice map_extent_mft_record()
will only fail if there is not enough memory so it will almost never
trigger but nonetheless it needs to be fixed. Below is a quick fix
for this.
Best regards,
Anton
--
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/
Fix dereference of incorrect variable. Thanks to Julia Lawall for
catching this bug.
Signed-off-by: Anton Altaparmakov <ai...@ca...>
diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
index 2ad5c8b..86e5002 100644
--- a/fs/ntfs/mft.c
+++ b/fs/ntfs/mft.c
@@ -2576,6 +2576,8 @@ mft_rec_already_initialized:
flush_dcache_page(page);
SetPageUptodate(page);
if (base_ni) {
+ MFT_RECORD *m_tmp;
+
/*
* Setup the base mft record in the extent mft
record. This
* completes initialization of the allocated extent
mft record
@@ -2588,11 +2590,11 @@ mft_rec_already_initialized:
* attach it to the base inode @base_ni and map, pin,
and lock
* its, i.e. the allocated, mft record.
*/
- m = map_extent_mft_record(base_ni, bit, &ni);
- if (IS_ERR(m)) {
+ m_tmp = map_extent_mft_record(base_ni, bit, &ni);
+ if (IS_ERR(m_tmp)) {
ntfs_error(vol->sb, "Failed to map allocated
extent "
"mft record 0x%llx.", (long
long)bit);
- err = PTR_ERR(m);
+ err = PTR_ERR(m_tmp);
/* Set the mft record itself not in use. */
m->flags &= cpu_to_le16(
~le16_to_cpu(MFT_RECORD_IN_USE));
@@ -2603,6 +2605,7 @@ mft_rec_already_initialized:
ntfs_unmap_page(page);
goto undo_mftbmp_alloc;
}
+ BUG_ON(m != m_tmp);
/*
* Make sure the allocated mft record is written out
to disk.
* No need to set the inode dirty because the caller
is going
|