Skip to content

Conversation

@kazarmy
Copy link
Member

@kazarmy kazarmy commented Mar 8, 2021

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the documentation and the rizin book with the relevant information (if needed)

Detailed description

This pr fixes warnings of the following form in the optimized asan build of #260 in rizin code (https://github.com/rizinorg/rizin/runs/2031121204, View raw logs, search for "-Wstringop-truncation" ignoring the capstone code):

2021-03-04T12:52:02.6893560Z In file included from /usr/include/string.h:495,
2021-03-04T12:52:02.6894069Z                  from ../shlr/winkd/iob_pipe.c:4:
2021-03-04T12:52:02.6895383Z In function ‘strncpy’,
2021-03-04T12:52:02.6896113Z     inlined from ‘iob_pipe_open’ at ../shlr/winkd/iob_pipe.c:75:2:
2021-03-04T12:52:02.6897385Z /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ specified bound 108 equals destination size [-Wstringop-truncation]
2021-03-04T12:52:02.6898430Z   106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
2021-03-04T12:52:02.6899110Z       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Test plan

All builds are green.

Closing issues

...

Copy link
Member

@ret2libc ret2libc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this actually fixes the warnings? It seems to me nothing changes. The string truncation can happen anyway (AFAIK the issue is that you limit a string potentially longer than the destination) and actually by using -1 you just never use the last char, which was already set to \0.

@codecov
Copy link

codecov bot commented Mar 8, 2021

Codecov Report

Merging #794 (5c2ffee) into dev (d058198) will decrease coverage by 0.02%.
The diff coverage is 40.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #794      +/-   ##
==========================================
- Coverage   42.81%   42.78%   -0.03%     
==========================================
  Files         871      871              
  Lines      316986   316985       -1     
==========================================
- Hits       135703   135610      -93     
- Misses     181283   181375      +92     
Impacted Files Coverage Δ
librz/debug/p/native/linux/linux_coredump.c 0.00% <0.00%> (ø)
shlr/winkd/iob_pipe.c 0.00% <0.00%> (ø)
librz/bin/format/elf/elf.c 77.90% <100.00%> (ø)
librz/bin/format/mach0/mach0.c 60.47% <100.00%> (ø)
shlr/gdb/src/gdbclient/responses.c 15.03% <0.00%> (-18.43%) ⬇️
librz/debug/p/debug_gdb.c 48.19% <0.00%> (-12.14%) ⬇️
librz/reg/reg.c 77.77% <0.00%> (-5.26%) ⬇️
shlr/gdb/src/gdbclient/core.c 36.80% <0.00%> (-2.68%) ⬇️
librz/core/cmd_debug.c 27.35% <0.00%> (+0.06%) ⬆️
librz/util/sys.c 60.77% <0.00%> (+0.22%) ⬆️
... and 6 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 d058198...5c2ffee. Read the comment docs.

@kazarmy
Copy link
Member Author

kazarmy commented Mar 8, 2021

Are you sure this actually fixes the warnings?

Yes, see https://github.com/rizinorg/rizin/runs/2055943596.

It seems to me nothing changes. The string truncation can happen anyway

The problem isn't the truncation. It's the specified bound 108 equals destination size part of the warning. Btw, there's an explanation of what I think is happening at capstone-engine/capstone#1730 (comment).

@kazarmy kazarmy merged commit d5aa6ec into rizinorg:dev Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants