Skip to content

lldb/source/Host/linux/Host.cpp:94: Possible missing field width in scanf %s ? #89710

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

Closed
dcb314 opened this issue Apr 23, 2024 · 2 comments
Closed

Comments

@dcb314
Copy link

dcb314 commented Apr 23, 2024

Static analyser cppcheck says:

lldb/source/Host/linux/Host.cpp:94:7: warning: sscanf() without field width limits can crash with huge input data. [invalidscanf]

Source code is

if (sscanf(Rest.data(),
"%d %s %c %d %d %d %d %d %u %lu %lu %lu %lu %lu %lu %ld %ld",
&stat_fields.pid, stat_fields.comm, &stat_fields.state,
&stat_fields.ppid, &stat_fields.pgrp, &stat_fields.session,
&stat_fields.tty_nr, &stat_fields.tpgid, &stat_fields.flags,
&stat_fields.minflt, &stat_fields.cminflt, &stat_fields.majflt,
&stat_fields.cmajflt, &stat_fields.utime, &stat_fields.stime,
&stat_fields.cutime, &stat_fields.cstime) < 0) {

but

char comm[task_comm_len];

and

constexpr int task_comm_len = 16;

Might there be some value in adding the string maximum length to the %s ?

@llvmbot
Copy link
Member

llvmbot commented Apr 23, 2024

@llvm/issue-subscribers-lldb

Author: None (dcb314)

Static analyser cppcheck says:

lldb/source/Host/linux/Host.cpp:94:7: warning: sscanf() without field width limits can crash with huge input data. [invalidscanf]

Source code is

if (sscanf(Rest.data(),
"%d %s %c %d %d %d %d %d %u %lu %lu %lu %lu %lu %lu %ld %ld",
&stat_fields.pid, stat_fields.comm, &stat_fields.state,
&stat_fields.ppid, &stat_fields.pgrp, &stat_fields.session,
&stat_fields.tty_nr, &stat_fields.tpgid, &stat_fields.flags,
&stat_fields.minflt, &stat_fields.cminflt, &stat_fields.majflt,
&stat_fields.cmajflt, &stat_fields.utime, &stat_fields.stime,
&stat_fields.cutime, &stat_fields.cstime) < 0) {

but

char comm[task_comm_len];

and

constexpr int task_comm_len = 16;

Might there be some value in adding the string maximum length to the %s ?

xgupta added a commit to xgupta/llvm-project that referenced this issue Jun 7, 2024
DavidSpickett added a commit to DavidSpickett/llvm-project that referenced this issue Jul 24, 2024
As reported in llvm#89710, the %s
code used for `comm` could and probably does, overflow the buffer.
Likely we haven't seen it cause problems because the following data
is overwritten right afterwards.

Also scanf isn't a great choice here as this `comm` can include many characters
that might trip up %s.

We don't actually use `comm`, so parse but don't store it so we're
not overflowing anything.
DavidSpickett added a commit that referenced this issue Jul 25, 2024
…0387)

As reported in #89710, the %s
code used for `comm` could and probably does, overflow the buffer.
Likely we haven't seen it cause problems because the following data is
overwritten right afterwards.

Also scanf isn't a great choice here as this `comm` can include many
characters that might trip up %s.

We don't actually use `comm`, so parse but don't store it so we're not
overflowing anything.
@DavidSpickett
Copy link
Collaborator

Fixed by #100387, and we will rework the code so that we are not using scanf on a field that can potentially include spaces and special characters.

yuxuanchen1997 pushed a commit that referenced this issue Jul 25, 2024
…0387)

Summary:
As reported in #89710, the %s
code used for `comm` could and probably does, overflow the buffer.
Likely we haven't seen it cause problems because the following data is
overwritten right afterwards.

Also scanf isn't a great choice here as this `comm` can include many
characters that might trip up %s.

We don't actually use `comm`, so parse but don't store it so we're not
overflowing anything.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250556
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants