fix coverity defects with CID 147626-28 #5304

heary-cao · 2016-10-19T12:55:23Z

issues:
fix coverity defects
coverity scan CID:147626, Type:Dereference before null check
coverity scan CID:147628, Type:Dereference before null check

Signed-off-by: cao.xuewen cao.xuewen@zte.com.cn

mention-bot · 2016-10-19T12:55:27Z

@heary-cao, thanks for your PR! By analyzing the history of the files in this pull request, we identified @behlendorf, @grwilson and @ahrens to be potential reviewers.

behlendorf · 2016-10-19T18:05:19Z

module/zfs/zfs_vnops.c

-	    zfs_putpage_commit_cb, pp);
+	if (zsb->z_log != NULL)
+		zfs_log_write(zsb->z_log, tx, TX_WRITE, zp, pgoff, pglen, 0,
+		    zfs_putpage_commit_cb, pp);


This should never be a possible case in the existing code. Pages which are part of a snapshot should never be allowed to be dirtied, see commit 1e8db77. This check was added just in case somehow a page got dirtied. Now that pretty clearly can never happen or we would have NULL dereferenced here long ago and there are no issues like this open in the tracker. Given that I'd rather remove the NULL check around zil_commit() a few lines down.

This also has the advantage of bringing this code slightly closer inline with upstream at least in terms of intent and assumptions.

@tuxoko what are your thoughts on this.

@behlendorf
Yes, I agree with you. I don't see why we need this.

@behlendorf @tuxoko
thank your for review it.

behlendorf · 2016-10-19T18:07:07Z

module/zfs/zfs_vfsops.c

@@ -1701,8 +1701,7 @@ zfs_resume_fs(zfs_sb_t *zsb, const char *osname)
 		 * Since we couldn't setup the sa framework, try to force
 		 * unmount this file system.
 		 */
-		if (zsb->z_os)
-			(void) zfs_umount(zsb->z_sb);
+		(void) zfs_umount(zsb->z_sb);


That's a good catch.

Can you explain this change?

The original fix here doesn't look quite right. Take a look at the stack from the original bug report. If zsb->z_os were NULL we should never have been able to make it to where the node finally crashed.

#1518

zfs_resume_fs zfs_umount zfs_sb_teardown dmu_objset_pool <- PANIC

In zfs_sb_teardown several checks for zsb->z_os == NULL are made before it's used which should have prevented this. What exactly in this first check preventing that the other ones aren't?

@behlendorf
The commit message says concurrent umount, so it's likely caused by race. Though I don't think the patch actually fix the race. Also, using zfs_umount here looks strange, I'm not sure how things would work in terms of VFS after this call.

The fact that zsb->z_os can change outside a lock is definitely racy and problematic. Rather than needlessly change this let's leave it as is for now and I'll make a note in the Coverity analysis. @heary-cao go ahead and drop this hunk when you next refresh this patch.

@behlendorf @tuxoko
ok,
thanks.

tuxoko · 2016-10-19T18:30:39Z

lib/libuutil/uu_dprintf.c

@@ -68,7 +68,8 @@ uu_dprintf_create(const char *name, uu_dprintf_severity_t severity,
 {
 	uu_dprintf_t *D;

-	if (uu_check_name(name, UU_NAME_DOMAIN) == -1) {
+	if (name != NULL ||
+	    uu_check_name(name, UU_NAME_DOMAIN) == -1) {


What are you trying to do...

tuxoko · 2016-10-19T18:31:16Z

module/zfs/zfs_fm.c

@@ -131,7 +131,8 @@ zfs_ereport_start(nvlist_t **ereport_out, nvlist_t **detector_out,
 	 * If we are doing a spa_tryimport() or in recovery mode,
 	 * ignore errors.
 	 */
-	if (spa_load_state(spa) == SPA_LOAD_TRYIMPORT ||
+	if (spa != NULL ||
+	    spa_load_state(spa) == SPA_LOAD_TRYIMPORT ||


What are you trying to do....

These are all defects identified by the Coverity static analysis tool. Specifically these are all cases where Coverity determined there's a NULL check latter in the function but it's pointless because by some call path we've already dereferenced the variable. So either the NULL check is pointless or it needs to be done sooner.

Type:Dereference before null check

@behlendorf
If you look carefully, it doesn't fix null dereference...

Right the fix itself is wrong here. It should be spa == NULL in the check so it returns immediately return when no spa is passed. Or better yet just drop think hunk completely. This is an internal function all callers know to always pass a spa. Period.

This chunk isn't correct. Yes it prevents a NULL dereference in this block but the same thing can happen 5 lines down. Please drop think hunk from the patch and we'll address it if need be in a different PR.

tuxoko · 2016-10-19T18:31:43Z

module/zfs/zfs_vfsops.c

@@ -1701,8 +1701,7 @@ zfs_resume_fs(zfs_sb_t *zsb, const char *osname)
 		 * Since we couldn't setup the sa framework, try to force
 		 * unmount this file system.
 		 */
-		if (zsb->z_os)
-			(void) zfs_umount(zsb->z_sb);
+		(void) zfs_umount(zsb->z_sb);


Can you explain this change?

tuxoko · 2016-10-19T18:32:47Z

module/zfs/zfs_vnops.c

-	    zfs_putpage_commit_cb, pp);
+	if (zsb->z_log != NULL)
+		zfs_log_write(zsb->z_log, tx, TX_WRITE, zp, pgoff, pglen, 0,
+		    zfs_putpage_commit_cb, pp);


@behlendorf
Yes, I agree with you. I don't see why we need this.

tuxoko · 2016-10-19T18:39:23Z

@behlendorf
TBH, I really don't like a mechanical fix like this. Just as this patch shows, when you just try to silence a static analysis, but not really understand what the code you touch is doing. It often make things worse.

behlendorf · 2016-10-19T19:35:37Z

@tuxoko it's a useful tool as long as it's not used purely mechanically. Each change needs to be understood and you're right it shouldn't be used just to silence a warning but to make the code clearer.

heary-cao

@behlendorf , @tuxoko
please review again.
thank your.

heary-cao · 2016-10-20T14:47:05Z

module/zfs/zfs_vnops.c

-	    zfs_putpage_commit_cb, pp);
+	if (zsb->z_log != NULL)
+		zfs_log_write(zsb->z_log, tx, TX_WRITE, zp, pgoff, pglen, 0,
+		    zfs_putpage_commit_cb, pp);


@behlendorf @tuxoko
thank your for review it.

heary-cao · 2016-10-20T15:01:57Z

module/zfs/zfs_vfsops.c

@@ -1701,8 +1701,7 @@ zfs_resume_fs(zfs_sb_t *zsb, const char *osname)
 		 * Since we couldn't setup the sa framework, try to force
 		 * unmount this file system.
 		 */
-		if (zsb->z_os)
-			(void) zfs_umount(zsb->z_sb);
+		(void) zfs_umount(zsb->z_sb);


@behlendorf @tuxoko
ok,
thanks.

behlendorf · 2016-11-07T22:12:35Z

module/zfs/zfs_fm.c

@@ -131,7 +131,8 @@ zfs_ereport_start(nvlist_t **ereport_out, nvlist_t **detector_out,
 	 * If we are doing a spa_tryimport() or in recovery mode,
 	 * ignore errors.
 	 */
-	if (spa_load_state(spa) == SPA_LOAD_TRYIMPORT ||
+	if (spa != NULL ||
+	    spa_load_state(spa) == SPA_LOAD_TRYIMPORT ||


This chunk isn't correct. Yes it prevents a NULL dereference in this block but the same thing can happen 5 lines down. Please drop think hunk from the patch and we'll address it if need be in a different PR.

behlendorf · 2016-11-07T22:18:51Z

lib/libuutil/uu_dprintf.c

+	D->uud_name = strdup(name);
+	if (D->uud_name == NULL) {
+		uu_free(D);
+		return (NULL);


The intention of this function is clearly to allow name to be NULL. Rather than removing this code I'd recommend only running the uu_check_name() when name != NULL. I'd suggest moving to it's own PR too so we can merge the other parts of this PR.

@behlendorf
thanks for your suggest
commit other PR:#5376
please review it.
thanks.

issues: fix coverity defects coverity scan CID:147626, Type:Dereference before null check coverity scan CID:147628, Type:Dereference before null check Signed-off-by: cao.xuewen cao.xuewen@zte.com.cn

behlendorf · 2016-11-08T18:22:31Z

@tuxoko any remaining concerns with the trimmed down patch proposed.

CID 147626: Type:Dereference before null check CID 147628: Type:Dereference before null check Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov Reviewed-by: Chunwei Chen <david.chen@osnexus.com> Signed-off-by: cao.xuewen <cao.xuewen@zte.com.cn> Closes openzfs#5304

behlendorf requested changes Oct 19, 2016

View reviewed changes

tuxoko suggested changes Oct 19, 2016

View reviewed changes

heary-cao force-pushed the before_null_check branch 2 times, most recently from e12cdcb to 18d865c Compare October 21, 2016 08:47

heary-cao changed the title ~~fix coverity defects with CID 147625-26-28-29~~ fix coverity defects with CID 147626-28-29 Oct 21, 2016

heary-cao force-pushed the before_null_check branch from 18d865c to 59b4b5c Compare October 25, 2016 02:46

heary-cao commented Nov 1, 2016

View reviewed changes

behlendorf requested changes Nov 7, 2016

View reviewed changes

heary-cao force-pushed the before_null_check branch from 59b4b5c to 18fdd8a Compare November 8, 2016 02:05

heary-cao changed the title ~~fix coverity defects with CID 147626-28-29~~ fix coverity defects with CID 147626-28 Nov 8, 2016

heary-cao force-pushed the before_null_check branch from 18fdd8a to 0935f03 Compare November 8, 2016 02:36

fix coverity defects with CID 147626-28

e8088a9

issues: fix coverity defects coverity scan CID:147626, Type:Dereference before null check coverity scan CID:147628, Type:Dereference before null check Signed-off-by: cao.xuewen cao.xuewen@zte.com.cn

heary-cao force-pushed the before_null_check branch from 0935f03 to e8088a9 Compare November 8, 2016 07:00

behlendorf approved these changes Nov 8, 2016

View reviewed changes

tuxoko approved these changes Nov 8, 2016

View reviewed changes

behlendorf merged commit a36cc8d into openzfs:master Nov 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix coverity defects with CID 147626-28 #5304

fix coverity defects with CID 147626-28 #5304

heary-cao commented Oct 19, 2016 •

edited

Loading

mention-bot commented Oct 19, 2016

behlendorf Oct 19, 2016

tuxoko Oct 19, 2016

heary-cao Oct 20, 2016

behlendorf Oct 19, 2016

tuxoko Oct 19, 2016

tuxoko Oct 19, 2016

behlendorf Oct 19, 2016

tuxoko Oct 19, 2016

behlendorf Oct 19, 2016

heary-cao Oct 20, 2016

tuxoko Oct 19, 2016

tuxoko Oct 19, 2016

behlendorf Oct 19, 2016

tuxoko Oct 19, 2016

behlendorf Oct 19, 2016

behlendorf Nov 7, 2016

tuxoko Oct 19, 2016

tuxoko Oct 19, 2016

tuxoko commented Oct 19, 2016

behlendorf commented Oct 19, 2016

heary-cao left a comment

heary-cao Oct 20, 2016

heary-cao Oct 20, 2016

behlendorf Nov 7, 2016

behlendorf Nov 7, 2016

heary-cao Nov 8, 2016

behlendorf commented Nov 8, 2016

fix coverity defects with CID 147626-28 #5304

fix coverity defects with CID 147626-28 #5304

Conversation

heary-cao commented Oct 19, 2016 • edited Loading

mention-bot commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuxoko commented Oct 19, 2016

behlendorf commented Oct 19, 2016

heary-cao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behlendorf commented Nov 8, 2016

heary-cao commented Oct 19, 2016 •

edited

Loading