linux-kernel - Re: [PATCH 0/3] make ntfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <39DAF2CE-95BC-4E0C-B30E-4686DF0E8D31@tuxera.com>
Date:	Fri, 19 Aug 2011 22:52:35 +0100
From:	Anton Altaparmakov <anton@...era.com>
To:	Jesper Juhl <jj@...osbits.net>
Cc:	linux-kernel@...r.kernel.org, linux-ntfs-dev@...ts.sourceforge.net
Subject: Re: [PATCH 0/3] make ntfs_free() NULL safe

Hi,

On 19 Aug 2011, at 22:30, Jesper Juhl wrote:
> Here's a small series of patches that make it safe to call ntfs_free() 
> with a NULL pointer and reaps some bennefits from that.
> 
> The first patch in the series simply makes ntfs_free() safe to call with a 
> NULL pointer. This fits with many other kernel freeing functions, that are 
> generally safe to call with NULL pointers.
> 
> The second patch adds some documentation to ntfs_free() similar to what's 
> already provided for the allocation functions.  ntfs_free() is fairly 
> simple so you could argue that such documentation is not really needed, 
> but I say it's still nice to have if for no other reason than 
> completeness.
> 
> The third patch removes a number of tests for NULL pointers before calls 
> to ntfs_free() that patch 1 make redundant.

Patches look fine.  Feel free to add my

	Acked-by: Anton Altaparmakov <anton@...era.com>

and to send them to Linus for inclusion…

> This whole things came about because Coverity Prevent spotted that in 
> fs/ntfs/runlist.c on line 967 we call ntfs_runlists_merge() which frees 
> its second argument and we then explicitly free that argument via 
> ntfs_free() again on line 970. This patch series also makes that a non 
> issue.

Ah but Coverity Prevent is incorrect in its spotting!

Have a look yourself!

ntfs_runlists_merge() _ONLY_ frees its second argument if it returns success.  If it returns error it does _NOT_ free its second argument!

And line 970 is _ONLY_ executed if ntfs_runlists_merge() returned error, i.e. in the case that the second argument was _NOT_ freed.  If the argument was freed, ntfs_runlists_merge() would have returned success, and then line 970 would never have been reached…

So I am afraid this is a bug in Coverity Prevent rather than in NTFS.  (-:

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/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/