src/strace: Enhance --string-limit/-s to remove max string length limit #298
Conversation
|
Hello, |
|
You may need to update the strace.1 man page. |
|
I've updated the man page and also rebased on current master |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| check_prog grep | ||
| run_prog ../btrfs > /dev/null | ||
| run_strace -a16 -sinf -eioctl $args > "$EXP" |
Check warning
Code scanning / shellcheck
SC2154
Enhance the -s STRSIZE/--string-limit=STRSIZE option to provide the ability to remove the max string length limit with the value argument 'inf'. This patch resolves issue 269 on github. Signed-off-by: Gautam Menghani <gautam.opensource@gmail.com>
There was a problem hiding this comment.
On 64-bit systems struct sysinfo.freeram can contain huge values exceeding the actual amount of memory available for a process to allocate.
For this reason I don't think allocating that much memory in advance "just in case" is a viable approach.
It would be much better to change memory allocation approach so that it wouldn't depend on the amount of free memory in the system but rather on actual needs of strace.
| @@ -563,12 +564,16 @@ extern enum stack_trace_modes stack_trace_mode; | |||
| # define stack_trace_mode STACK_TRACE_OFF | |||
| # endif | |||
| extern unsigned max_strlen; | |||
| extern bool truncate_output; | |||
| extern struct sysinfo info; | |||
There was a problem hiding this comment.
struct sysinfo is an implementation detail, please do not turn it into an interface.
| @@ -1203,6 +1208,8 @@ dumpiov_upto(struct tcb *, int len, kernel_ulong_t addr, kernel_ulong_t data_siz | |||
| extern void | |||
| dumpstr(struct tcb *, kernel_ulong_t addr, kernel_ulong_t len); | |||
|
|
|||
| extern uint64_t get_max_size(void); | |||
There was a problem hiding this comment.
get_max_size is an implementation detail, please do not turn it into an interface.
| max_strlen = i; | ||
| } else if (strcmp(optarg, "inf") == 0) { | ||
| truncate_output = false; | ||
| sysinfo(&info); |
There was a problem hiding this comment.
In this branch max_strlen is not assigned so it remains initialized with the default value.
This wouldn't be an issue if max_strlen wasn't used in several places besides being an argument in truncation_needed invocations.
By the way, what will happen if sysinfo failed?
| */ | ||
| uint64_t get_max_size(void) | ||
| { | ||
| return (uint64_t) floor((info.freeram - 3) / 5); |
There was a problem hiding this comment.
Isn't info.freeram an integer? If yes, how does floor help here?
| unsigned int style = user_style; | ||
| uint64_t max_len = (truncate_output) ? max_strlen : get_max_size(); |
There was a problem hiding this comment.
Why uint64_t? xmalloc takes size_t argument anyway.
| if (!str) { | ||
| const unsigned int outstr_size = | ||
| 4 * max_strlen + /* for quotes and NUL */ 3; | ||
| const uint64_t outstr_size = |
| netlink_protocol +netlink_sock_diag.test | ||
| netlink_protocol +netlink_sock_diag.test |
| truncate | ||
| truncate -e trace=truncate |
| if (cur >= abbrev_end) { | ||
| if (truncation_needed(cur, abbrev_end)) { |
There was a problem hiding this comment.
Given the definition of abbrev_end, why this change is needed?
Enhance the -s STRSIZE/--string-limit=STRSIZE option to provide the ability to remove the max string length limit with the value argument 'inf'. This patch resolves issue 269 on github.
closes #269