Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix allocation error, detected using ASAN #10193

Merged
merged 3 commits into from
Apr 13, 2020

Conversation

dioni21
Copy link
Contributor

@dioni21 dioni21 commented Apr 11, 2020

Motivation and Context

I just found an ASAN warning in zpool cmd

Apparently it only turns on when a pool has some removed vdev data.

Description

Just add the free() when buffer is no more needed.

How Has This Been Tested?

Compiled in my home setup, ran, and error is gone

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the contributing document.
  • I have added tests to cover my changes.
  • I have run the ZFS Test Suite with this change applied.
  • All commit messages are properly formatted and contain Signed-off-by.

@dioni21
Copy link
Contributor Author

dioni21 commented Apr 11, 2020

Forgot to add this.

Configuration:
configure --enable-silent-rules --enable-dependency-tracking --enable-code-coverage --enable-asan --enable-debug --enable-debuginfo --enable-debug-kmem --enable-debug-kmem-tracking

Error messages (somewhat edited):

# zpool status
  pool: XXX
 state: ONLINE
  scan: scrub repaired 0B in 0 days 20:30:09 with 0 errors on Fri Apr 10 00:04:48 2020
remove: Removal of vdev 2 copied 8.84G in 0h0m, completed on Sat Apr  4 00:08:34 2020
    56.4K memory used for removed device mappings
config:

        NAME        STATE     READ WRITE CKSUM
        bad         ONLINE       0     0     0
          mirror-0  ONLINE       0     0     0
            xxx1    ONLINE       0     0     0
            xxx2    ONLINE       0     0     0
        special
          special   ONLINE       0     0     0
        logs
          zil       ONLINE       0     0     0
        cache
          cache     ONLINE       0     0     0

errors: No known data errors

  pool: YYY
 state: ONLINE
  scan: resilvered 0B in 0 days 00:16:43 with 0 errors on Fri Apr 10 16:25:56 2020
config:

        NAME        STATE     READ WRITE CKSUM
        tank        ONLINE       0     0     0
          yyy1      ONLINE       0     0     0
        special
          special   ONLINE       0     0     0
        logs
          mirror-2  ONLINE       0     0     0
            zil     ONLINE       0     0     0
            zil     ONLINE       0     0     0
        cache
          cache     ONLINE       0     0     0

errors: No known data errors

=================================================================
==3527797==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 9 byte(s) in 1 object(s) allocated from:
    #0 0x7f3820b3e52d in strdup (/lib64/libasan.so.5+0x9652d)
    #1 0x7f3820982986 in zfs_strdup libzfs_util.c:678
    #2 0x7f38209581db in zpool_vdev_name (/lib64/libzfs.so.2+0x881db)
    #3 0x56276a91920e in print_removal_status /usr/src/debug/zfs-0.8.0-716_g36a6e2335.fc31.x86_64/cmd/zpool/zpool_main.c:7320
    #4 0x56276a93889a in status_callback /usr/src/debug/zfs-0.8.0-716_g36a6e2335.fc31.x86_64/cmd/zpool/zpool_main.c:7938
    #5 0x56276a90d745 in pool_list_iter /usr/src/debug/zfs-0.8.0-716_g36a6e2335.fc31.x86_64/cmd/zpool/zpool_iter.c:179
    #6 0x56276a90dbfd in for_each_pool /usr/src/debug/zfs-0.8.0-716_g36a6e2335.fc31.x86_64/cmd/zpool/zpool_iter.c:253
    #7 0x56276a92edb2 in zpool_do_status /usr/src/debug/zfs-0.8.0-716_g36a6e2335.fc31.x86_64/cmd/zpool/zpool_main.c:8142
    #8 0x56276a90ad0b  (/usr/sbin/zpool+0x1cd0b)
    #9 0x7f382054f1a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)

SUMMARY: AddressSanitizer: 9 byte(s) leaked in 1 allocation(s).

Signed-off-by: João Carlos Mendes Luis <jonny@jonny.eng.br>
@ahrens
Copy link
Member

ahrens commented Apr 11, 2020

Thanks for noticing this!

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Apr 11, 2020
@codecov-io
Copy link

codecov-io commented Apr 11, 2020

Codecov Report

Merging #10193 into master will decrease coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10193      +/-   ##
==========================================
- Coverage   79.30%   79.21%   -0.10%     
==========================================
  Files         387      387              
  Lines      123387   123388       +1     
==========================================
- Hits        97858    97739     -119     
- Misses      25529    25649     +120     
Flag Coverage Δ
#kernel 79.88% <ø> (ø)
#user 65.13% <100.00%> (-0.49%) ⬇️
Impacted Files Coverage Δ
cmd/zpool/zpool_main.c 80.61% <100.00%> (+0.10%) ⬆️
module/os/linux/spl/spl-zlib.c 55.35% <0.00%> (-28.58%) ⬇️
module/zfs/vdev_indirect.c 73.50% <0.00%> (-11.00%) ⬇️
module/zfs/vdev_raidz.c 90.95% <0.00%> (-2.51%) ⬇️
cmd/zed/agents/fmd_api.c 88.61% <0.00%> (-1.78%) ⬇️
module/icp/api/kcf_mac.c 37.71% <0.00%> (-1.72%) ⬇️
module/os/linux/zfs/zvol_os.c 87.00% <0.00%> (-1.33%) ⬇️
lib/libzfs/libzfs_changelist.c 85.15% <0.00%> (-1.18%) ⬇️
module/zfs/vdev_mirror.c 94.77% <0.00%> (-1.12%) ⬇️
module/zfs/zvol.c 84.06% <0.00%> (-1.01%) ⬇️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c618f87...f22b88f. Read the comment docs.

The test for VDEV_TYPE_INDIRECT is done after a memory allocation, and
could return from function without freeing it.

Since we don't need that alocation yet, just postpone it.

Signed-off-by: João Carlos Mendes Luís <jonny@jonny.eng.br>
@dioni21
Copy link
Contributor Author

dioni21 commented Apr 11, 2020

I'm new to this merge and update method from github, sorry if I did something wrong.

Just added another leak fix, at f22b88f

Error appears at zpool iostat -v:

=================================================================
==633513==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 328 byte(s) in 1 object(s) allocated from:
    #0 0x7f2d9385df16 in __interceptor_calloc (/lib64/libasan.so.5+0x10df16)
    #1 0x55c9e3784814  (/usr/sbin/zpool+0x4d814)
    #2 0x55c9e377d8f3 in print_vdev_stats /usr/src/debug/zfs-0.8.0-717_g09cfb3f3d.fc31.x86_64/cmd/zpool/zpool_main.c:4398
    #3 0x55c9e377e40f in print_vdev_stats /usr/src/debug/zfs-0.8.0-717_g09cfb3f3d.fc31.x86_64/cmd/zpool/zpool_main.c:4532
    #4 0x55c9e3780f62 in print_iostat /usr/src/debug/zfs-0.8.0-717_g09cfb3f3d.fc31.x86_64/cmd/zpool/zpool_main.c:4658
    #5 0x55c9e3756745 in pool_list_iter /usr/src/debug/zfs-0.8.0-717_g09cfb3f3d.fc31.x86_64/cmd/zpool/zpool_iter.c:179
    #6 0x55c9e377ff47 in zpool_do_iostat /usr/src/debug/zfs-0.8.0-717_g09cfb3f3d.fc31.x86_64/cmd/zpool/zpool_main.c:5527
    #7 0x55c9e3753d0b  (/usr/sbin/zpool+0x1cd0b)
    #8 0x7f2d931f71a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)

SUMMARY: AddressSanitizer: 328 byte(s) leaked in 1 allocation(s).

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Apr 13, 2020
@behlendorf behlendorf merged commit 75c6201 into openzfs:master Apr 13, 2020
@dioni21 dioni21 deleted the zpool_remove branch April 15, 2020 03:33
as-com pushed a commit to as-com/zfs that referenced this pull request Jun 20, 2020
The test for VDEV_TYPE_INDIRECT is done after a memory allocation, and
could return from function without freeing it.  Since we don't need that
allocation yet, just postpone it.

Add a missing free() when buffer is no longer needed.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: João Carlos Mendes Luís <jonny@jonny.eng.br>
Closes openzfs#10193 
(cherry picked from commit 75c6201)
as-com pushed a commit to as-com/zfs that referenced this pull request Jun 20, 2020
The test for VDEV_TYPE_INDIRECT is done after a memory allocation, and
could return from function without freeing it.  Since we don't need that
allocation yet, just postpone it.

Add a missing free() when buffer is no longer needed.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: João Carlos Mendes Luís <jonny@jonny.eng.br>
Closes openzfs#10193 
(cherry picked from commit 75c6201)
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
The test for VDEV_TYPE_INDIRECT is done after a memory allocation, and
could return from function without freeing it.  Since we don't need that
allocation yet, just postpone it.

Add a missing free() when buffer is no longer needed.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: João Carlos Mendes Luís <jonny@jonny.eng.br>
Closes openzfs#10193
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants