1.10 task: error leaks

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

1.10 task: error leaks

Daniel Shahaf-2
The release branch creation checkpoints calls for checking for error
leaks, so here's the output of tools/dev/warn-unused-result.sh (after
removing the __attribute__ from svn_error__malfunction()):

[[[
subversion/libsvn_delta/branch.c:485:11: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
          svn_element__tree_set(branch->priv->element_tree, old_eid, NULL);
          ^~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
subversion/libsvn_delta/branch.c:486:11: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
          svn_element__tree_set(branch->priv->element_tree, new_eid, element);
          ^~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
subversion/libsvn_delta/branch.c:975:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
  svn_element__tree_set(branch->priv->element_tree, eid, element);
  ^~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 warnings generated.
subversion/libsvn_delta/element.c:465:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
  svn_element__tree_set(new_subtree, new_subtree->root_eid,
  ^~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/libsvn_subr/cache_config.c:164:18: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
  __attribute__((warn_unused_result)) svn_error_t *err
                 ^
1 warning generated.
subversion/libsvn_subr/io.c:2426:18: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
  __attribute__((warn_unused_result)) svn_error_t *err
                 ^
1 warning generated.
subversion/libsvn_subr/stream.c:1471:9: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
        svn_checksum_ctx_reset(btn->read_ctx);
        ^~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~
subversion/libsvn_subr/stream.c:1474:9: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
        svn_checksum_ctx_reset(btn->write_ctx);
        ^~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~
2 warnings generated.
subversion/libsvn_fs_fs/low_level.c:673:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
    svn_stream_puts(stream, "\n");
    ^~~~~~~~~~~~~~~ ~~~~~~~~~~~~
subversion/libsvn_fs_fs/low_level.c:890:18: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
  __attribute__((warn_unused_result)) svn_error_t *err
                 ^
2 warnings generated.
subversion/libsvn_fs_fs/transaction.c:3857:26: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
          __attribute__((warn_unused_result)) svn_error_t *err
                         ^
subversion/libsvn_fs_fs/transaction.c:3873:26: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
          __attribute__((warn_unused_result)) svn_error_t *err
                         ^
2 warnings generated.
subversion/libsvn_fs_fs/verify.c:856:26: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
          __attribute__((warn_unused_result)) svn_error_t *err2
                         ^
1 warning generated.
subversion/libsvn_fs_x/changes.c:187:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
    append_change(changes, APR_ARRAY_IDX(list, i, svn_fs_x__change_t *));
    ^~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/libsvn_fs_x/low_level.c:365:18: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
  __attribute__((warn_unused_result)) svn_error_t *err
                 ^
subversion/libsvn_fs_x/low_level.c:1134:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
    svn_stream_puts(stream, "\n");
    ^~~~~~~~~~~~~~~ ~~~~~~~~~~~~
2 warnings generated.
subversion/libsvn_fs_x/transaction.c:1262:11: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
          svn_io_dir_make(txn_dir, APR_OS_DEFAULT, iterpool);
          ^~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/libsvn_fs_x/verify.c:787:26: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
          __attribute__((warn_unused_result)) svn_error_t *err2
                         ^
1 warning generated.
subversion/libsvn_client/merge.c:368:18: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
  __attribute__((warn_unused_result)) svn_error_t *err
                 ^
1 warning generated.
subversion/libsvn_client/patch.c:2148:7: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
      svn_stream_printf(target->reject_stream,
      ^~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/libsvn_wc/adm_ops.c:418:20: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
    __attribute__((warn_unused_result)) svn_error_t *err
                   ^
1 warning generated.
subversion/libsvn_wc/wc_db.c:16528:15: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
              lock_remove_txn(queue->wcroot, cqi->local_relpath, work_item,
              ^~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/svn/conflict-callbacks.c:1657:11: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
          svn_cmdline_fprintf(stderr, iterpool, "%s\n",
          ^~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/svn/notify.c:213:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
  svn_hash_keys(conflicted_paths, all_conflicts, result_pool);
  ^~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/svnfsfs/load-index-cmd.c:150:7: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
      svn_stream_readline(input, &line, "\n", &eol, iterpool);
      ^~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/tests/libsvn_repos/authz-test.c:280:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
    print_user_rights(NULL, NULL, 0, &authz->anon_rights, pool);
    ^~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
subversion/tests/libsvn_repos/authz-test.c:282:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
    print_user_rights(NULL, NULL, 0, &authz->authn_rights, pool);
    ^~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
subversion/tests/libsvn_client/conflicts-test.c:543:7: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
      sbox_wc_move(b, move_src_path, new_dir_path);
      ^~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/tests/libsvn_repos/dump-load-test.c:84:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
  svn_stream_close(stream);
  ^~~~~~~~~~~~~~~~ ~~~~~~
subversion/tests/libsvn_repos/dump-load-test.c:134:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
  svn_stream_close(stream);
  ^~~~~~~~~~~~~~~~ ~~~~~~
2 warnings generated.
subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c:1634:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
  svn_hash_write2(props, svn_stream_from_stringbuf(hash_rep, pool), "END",
  ^~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/tests/libsvn_subr/mergeinfo-test.c:1765:6: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
     svn_rangelist_to_string(&tmp_string, rangelist, pool);
     ^~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/tests/libsvn_subr/priority-queue-test.c:128:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
  verify_empty_queue(queue);
  ^~~~~~~~~~~~~~~~~~ ~~~~~
subversion/tests/libsvn_subr/priority-queue-test.c:157:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
  verify_empty_queue(queue);
  ^~~~~~~~~~~~~~~~~~ ~~~~~
subversion/tests/libsvn_subr/priority-queue-test.c:217:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
  verify_empty_queue(queue);
  ^~~~~~~~~~~~~~~~~~ ~~~~~
3 warnings generated.
tools/client-side/svnconflict/svnconflict.c:366:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
    svn_cmdline_printf(pool, "text-conflict\n");
    ^~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~
tools/client-side/svnconflict/svnconflict.c:371:7: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
      svn_cmdline_printf(pool, "prop-conflict: %s\n", propname);
      ^~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/client-side/svnconflict/svnconflict.c:383:7: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
      svn_cmdline_printf(pool, "tree-conflict: %s %s\n",
      ^~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/client-side/svnconflict/svnconflict.c:404:7: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
      svn_cmdline_printf(pool, "%d: %s\n", id, label);
      ^~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
4 warnings generated.
tools/dev/svnmover/svnmover.c:2439:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
    svn_stringbuf_from_stream(&text, src, 0, scratch_pool);
    ^~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/dev/svnmover/svnmover.c:2779:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
  svn_element__tree_set(from_subtree->tree, from_subtree->tree->root_eid,
  ^~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
]]]

I think we should enable this warning by default in maintainer mode in
supporting compilers, but that's a separate discussion to just fixing
these error leaks in trunk (& 1.10.x).

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: 1.10 task: error leaks

Julian Foad-5
Daniel Shahaf wrote:
> The release branch creation checkpoints calls for checking for error
> leaks, so here's the output of tools/dev/warn-unused-result.sh (after
> removing the __attribute__ from svn_error__malfunction()):
>
> [about 41 instances]

Thank you, Daniel!

Anyone volunteering to fix them?

> I think we should enable this warning by default in maintainer mode in
> supporting compilers, but that's a separate discussion to just fixing
> these error leaks in trunk (& 1.10.x).

+1. I was going to say that, as soon as I saw this long list.

Anyone want to make it happen?

(I volunteer to do some of this too. In the role of Release Manager I
need to be asking for volunteers for as much as possible, so please
don't anyone take offence at my continuing to do so.)

- Julian
Reply | Threaded
Open this post in threaded view
|

Re: 1.10 task: error leaks

Julian Foad-5
In reply to this post by Daniel Shahaf-2
Daniel Shahaf wrote:
[...]
>            svn_element__tree_set(branch->priv->element_tree, old_eid, NULL);
>            svn_element__tree_set(branch->priv->element_tree, new_eid, element);
>    svn_element__tree_set(branch->priv->element_tree, eid, element);
>    svn_element__tree_set(new_subtree, new_subtree->root_eid,
[...]

I fixed those in r1817814 by removing the unused 'svn_error_t *' return
type.

(This reminds me the 'element' stuff and 'tools/dev/svnmover' is still
in trunk... unused, unfinished. I wonder if I should remove it or if I
need to do anything with it before 1.10.)

- Julian
Reply | Threaded
Open this post in threaded view
|

Re: 1.10 task: error leaks

Daniel Shahaf-2
In reply to this post by Julian Foad-5
Julian Foad wrote on Mon, 11 Dec 2017 19:51 +0000:
> Daniel Shahaf wrote:
> > I think we should enable this warning by default in maintainer mode in
> > supporting compilers, but that's a separate discussion to just fixing
> > these error leaks in trunk (& 1.10.x).
>
> +1. I was going to say that, as soon as I saw this long list.
>
> Anyone want to make it happen?

We already have an instance of __attribute__((warn_unused_result)) in
mod_dav_svn, not guarded by any #ifdef or anything, so I think we don't need to
bother with compiler checks (AC_TRY_COMPILE() and friends), but rather, we can
simply add that attribute unconditionally.

Of course, then we'd have a new problem: we might declare or define a function
without the __attribute__ annotation.  It would be best if we could tell our C
compilers, "_Any_ function that returns svn_error_t* should be treated as
having the __attribute__((warn_unused_result)) annotation".  Is that possible?

> (I volunteer to do some of this too. In the role of Release Manager I
> need to be asking for volunteers for as much as possible, so please
> don't anyone take offence at my continuing to do so.)

None taken; it should be a team effort.  Thanks for leading it.

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: 1.10 task: error leaks

Julian Foad-5
In reply to this post by Daniel Shahaf-2
Daniel Shahaf wrote:
> The release branch creation checkpoints calls for checking for error
> leaks, so here's the output of tools/dev/warn-unused-result.sh
[...]
> ...: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]

This kind of warning is when the script inappropriately puts the
attribute on a variable declaration, so we can ignore these.

> subversion/libsvn_subr/stream.c:1471:9:
>          svn_checksum_ctx_reset(btn->read_ctx);
> subversion/libsvn_subr/stream.c:1474:9:
>          svn_checksum_ctx_reset(btn->write_ctx);

These are the only two callers of this new-for-1.10 API:

/**
  * Reset an existing checksum @a ctx to initial state.
  * @see svn_checksum_ctx_create()
  *
  * @since New in 1.10.
  */
svn_error_t *
svn_checksum_ctx_reset(svn_checksum_ctx_t *ctx);

One possible solution would be to drop the error return.

I have taken the more conservative option of adding SVN_ERR() around the
calls, in r1818307.

> subversion/libsvn_client/patch.c:2148:7:
>        svn_stream_printf(target->reject_stream,

r1818309.

> subversion/libsvn_fs_fs/low_level.c:673:5:
>      svn_stream_puts(stream, "\n");
> subversion/libsvn_fs_x/changes.c:187:5:
>      append_change(changes, APR_ARRAY_IDX(list, i, svn_fs_x__change_t *));
> subversion/libsvn_fs_x/low_level.c:1134:5:
>      svn_stream_puts(stream, "\n");
> subversion/libsvn_fs_x/transaction.c:1262:11:
>            svn_io_dir_make(txn_dir, APR_OS_DEFAULT, iterpool);
> subversion/libsvn_wc/wc_db.c:16528:15:
>                lock_remove_txn(queue->wcroot, cqi->local_relpath, work_item,
> subversion/svn/conflict-callbacks.c:1657:11:
>            svn_cmdline_fprintf(stderr, iterpool, "%s\n",
> subversion/svn/notify.c:213:3:
>    svn_hash_keys(conflicted_paths, all_conflicts, result_pool);
> subversion/svnfsfs/load-index-cmd.c:150:7:
>        svn_stream_readline(input, &line, "\n", &eol, iterpool);

r1818310.

> subversion/tests/libsvn_repos/authz-test.c:280:5:
>      print_user_rights(NULL, NULL, 0, &authz->anon_rights, pool);
> subversion/tests/libsvn_repos/authz-test.c:282:5:
>      print_user_rights(NULL, NULL, 0, &authz->authn_rights, pool);
> subversion/tests/libsvn_client/conflicts-test.c:543:7:
>        sbox_wc_move(b, move_src_path, new_dir_path);
> subversion/tests/libsvn_repos/dump-load-test.c:84:3:
>    svn_stream_close(stream);
> subversion/tests/libsvn_repos/dump-load-test.c:134:3:
>    svn_stream_close(stream);
> subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c:1634:3:
>    svn_hash_write2(props, svn_stream_from_stringbuf(hash_rep, pool), "END",
> subversion/tests/libsvn_subr/mergeinfo-test.c:1765:6:
>       svn_rangelist_to_string(&tmp_string, rangelist, pool);
> subversion/tests/libsvn_subr/priority-queue-test.c:128:3:
>    verify_empty_queue(queue);
> subversion/tests/libsvn_subr/priority-queue-test.c:157:3:
>    verify_empty_queue(queue);
> subversion/tests/libsvn_subr/priority-queue-test.c:217:3:
>    verify_empty_queue(queue);
> tools/client-side/svnconflict/svnconflict.c:366:5:
>      svn_cmdline_printf(pool, "text-conflict\n");
> tools/client-side/svnconflict/svnconflict.c:371:7:
>        svn_cmdline_printf(pool, "prop-conflict: %s\n", propname);
> tools/client-side/svnconflict/svnconflict.c:383:7:
>        svn_cmdline_printf(pool, "tree-conflict: %s %s\n",
> tools/client-side/svnconflict/svnconflict.c:404:7:
>        svn_cmdline_printf(pool, "%d: %s\n", id, label);
> tools/dev/svnmover/svnmover.c:2439:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
>      svn_stringbuf_from_stream(&text, src, 0, scratch_pool);

Still to do.

- Julian
Reply | Threaded
Open this post in threaded view
|

Re: 1.10 task: error leaks

Julian Foad-5
>> subversion/tests/libsvn_repos/authz-test.c:280:5:
>>      print_user_rights(NULL, NULL, 0, &authz->anon_rights, pool);
>> subversion/tests/libsvn_repos/authz-test.c:282:5:
>>      print_user_rights(NULL, NULL, 0, &authz->authn_rights, pool);
>> subversion/tests/libsvn_client/conflicts-test.c:543:7:
>>        sbox_wc_move(b, move_src_path, new_dir_path);
>> subversion/tests/libsvn_repos/dump-load-test.c:84:3:
>>    svn_stream_close(stream);
>> subversion/tests/libsvn_repos/dump-load-test.c:134:3:
>>    svn_stream_close(stream);
>> subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c:1634:3:
>>    svn_hash_write2(props, svn_stream_from_stringbuf(hash_rep, pool),
>> "END",
>> subversion/tests/libsvn_subr/mergeinfo-test.c:1765:6:
>>       svn_rangelist_to_string(&tmp_string, rangelist, pool);
>> subversion/tests/libsvn_subr/priority-queue-test.c:128:3:
>>    verify_empty_queue(queue);
>> subversion/tests/libsvn_subr/priority-queue-test.c:157:3:
>>    verify_empty_queue(queue);
>> subversion/tests/libsvn_subr/priority-queue-test.c:217:3:
>>    verify_empty_queue(queue);
>> tools/client-side/svnconflict/svnconflict.c:366:5:
>>      svn_cmdline_printf(pool, "text-conflict\n");
>> tools/client-side/svnconflict/svnconflict.c:371:7:
>>        svn_cmdline_printf(pool, "prop-conflict: %s\n", propname);
>> tools/client-side/svnconflict/svnconflict.c:383:7:
>>        svn_cmdline_printf(pool, "tree-conflict: %s %s\n",
>> tools/client-side/svnconflict/svnconflict.c:404:7:
>>        svn_cmdline_printf(pool, "%d: %s\n", id, label);
>> tools/dev/svnmover/svnmover.c:2439:5:
>>      svn_stringbuf_from_stream(&text, src, 0, scratch_pool);

r1818318.

And on running the script I found a few more, calling a 'notify_func'
through a pointer...

subversion/libsvn_fs_fs/pack.c:2070
subversion/libsvn_fs_fs/pack.c:2125
subversion/libsvn_fs_fs/pack.c:2135
subversion/libsvn_fs_x/pack.c:2207
subversion/libsvn_fs_x/pack.c:2261

r1818320.

That's the lot.

- Julian
Reply | Threaded
Open this post in threaded view
|

Re: 1.10 task: error leaks

Daniel Shahaf-2
Julian Foad wrote on Fri, 15 Dec 2017 19:51 +0000:
> r1818320.
>
> That's the lot.

Thanks, Julian!