Bonjour Jean-Pierre,
I started playing around with your latest NTFS-3G release with support
for file ownership and permissions from
http://pagesperso-orange.fr/b.andre/security.html
http://pagesperso-orange.fr/b.andre/ntfs-3g-1.2129SR.1.tgz
and started off with an already used NTFS volume which I created using mkntfs
from ntfsprogs. I saw warnings of ntfs-3g not findig $Secure and after creating
it by hand as a normal file, I tried
vim /ntfs/.NTFS-3G/UserMapping
and got a crash. The cause seems to have been that it also appears to need
$INDEX_ROOT which it also didn't find until I created it by with touch.
I Started ntfs-3g in the debugger and saw that the cause was that
xsdh->entry
as returned by was zero ntfs_index_lookup() and it was deferenced without
further check before dereferencing it.
I only assume that missing $INDEX_ROOT played a role in this crash because
it was the error message which ntfs_index_lookup() printed before returning
the zero xsdh->entry.
I saw that ntfs_index_lookup() does not touch xsdh->entry if the lookup
fails, so I added a check to abort the function in that case. Please find
the patch which I created for verification and testing below.
It also adds a return value check to another place where ntfs_index_lookup()
is used in a similar way. There are two more places which do not check the
return vaule but they contain check of (* ntfs_index_context)->entry before
it is dereferenced, and I am not sure what would be the best way to handle
the found/not found return value from ntfs_index_lookup() in these places.
The patch below also contains a change to the header file to add
__attribute__ ((__warn_unused_result__))
to the declaration of ntfs_index_lookup() if gcc-3.4 or newer is used. So
if you use a compiler like that, you'll the the warnings which are generated
as a result.
I actutally didn't check what it prints but I verified that the crash was
fxied with this change applied but if needed I can check it of cou
I would be happy if you would be able to review the second part of the patch
and be able to tell me whether you find it acceptable for merging into your
branch as it is with improvements for example on the error which is printed.
I included first part (for index.h) only for demonstration, but you can apply
it as well. Ideall, the preprocessor defines should be put into a header file
which commonly included from all header files so that the macro can be used
in all of them to ensure that warnings are generated when the return values
of critical functions are not evaluated.
best regards,
Bernhard
This patch for ntfs-3g-1.2216SR.1 consists of two parts:
#1 - include/ntfs-3g/index.h - add __attribute__ ((__warn_unused_result__))
to ntfs_index_lookup() if compiling with gcc-3.4 or newer
(it's supported since then)
-> cosmetic, to find places where the return value of
ntfs_index_lookup() (found / not found) is not checked.
#2 - libntfs-3g/security.c
check the return value of ntfs_index_lookup() before accessing
a struct which is only filled with data if the lookup was successful.
-> Fixes a chrash with $Secure found but $INDEX_ROOT not.
I triggered it with:
vim /ntfs/.NTFS-3G/UserMapping
(seems to be some effect from use uf .UserMapping.swp)
--- ntfs-3g-1.2216SR.1/include/ntfs-3g/index.h
+++ ntfs-3g-1.2216SR.1/include/ntfs-3g/index.h
@@ -25,6 +25,32 @@
#ifndef _NTFS_INDEX_H
#define _NTFS_INDEX_H
+/* Convenience macros to test the versions of gcc.
+ Use them like this:
+ #if __GNUC_PREREQ (2,8)
+ ... code requiring gcc 2.8 or later ...
+ #endif
+ Note - they won't work for gcc1 or glibc1, since the _MINOR macros
+ were not defined then. */
+#ifndef __GNUC_PREREQ
+# if defined __GNUC__ && defined __GNUC_MINOR__
+# define __GNUC_PREREQ(maj, min) \
+ ((__GNUC__ << 16) + __GNUC_MINOR__ >= ((maj) << 16) + (min))
+# else
+# define __GNUC_PREREQ(maj, min) 0
+# endif
+#endif
+
+/* allows us to warn about unused results of certain function calls */
+#ifndef __attribute_warn_unused_result__
+# if __GNUC_PREREQ (3,4)
+# define __attribute_warn_unused_result__ \
+ __attribute__ ((__warn_unused_result__))
+# else
+# define __attribute_warn_unused_result__ /* empty */
+# endif
+#endif
+
#include "attrib.h"
#include "types.h"
#include "layout.h"
@@ -108,7 +134,7 @@
extern void ntfs_index_ctx_reinit(ntfs_index_context *ictx);
extern int ntfs_index_lookup(const void *key, const int key_len,
- ntfs_index_context *ictx);
+ ntfs_index_context *ictx) __attribute_warn_unused_result__;
extern INDEX_ENTRY *ntfs_index_next(INDEX_ENTRY *ie,
ntfs_index_context *ictx);
--- ntfs-3g-1.2216SR.1/libntfs-3g/security.c
+++ ntfs-3g-1.2216SR.1/libntfs-3g/security.c
@@ -1076,8 +1076,11 @@
ntfs_index_ctx_reinit(xsii);
offs = size = 0;
keyid = cpu_to_le32(-1);
- ntfs_index_lookup((char*)&keyid,
- sizeof(SII_INDEX_KEY), xsii);
+ if(ntfs_index_lookup((char*)&keyid,
+ sizeof(SII_INDEX_KEY), xsii)) {
+ ntfs_log_perror("Failed to find index key");
+ return 0;
+ }
entry = xsii->entry;
psii = (struct SII*)xsii->entry;
if (psii) {
@@ -1234,7 +1237,10 @@
*/
key.hash = hash;
key.security_id = cpu_to_le32(0);
- ntfs_index_lookup((char*)&key, sizeof(SDH_INDEX_KEY), xsdh);
+ if (ntfs_index_lookup((char*)&key, sizeof(SDH_INDEX_KEY),
xsdh)) {
+ ntfs_log_perror("Failed to find index key");
+ return 0;
+ }
entry = xsdh->entry;
found = FALSE;
/* lookup() may return a node with no data, if so get next */
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
ntfs-3g-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ntfs-3g-devel
