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.

Sign up for GitHub

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 global buffer overflow in IDLC #1900

Merged
merged 1 commit into from
Dec 11, 2023
Merged

Fix global buffer overflow in IDLC #1900

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions src/tools/idlpp/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -965,18 +965,21 @@ static void put_a_line(
*/
{
size_t len;
char * out_p;
char * tp;

if (no_output)
return;
len = strlen( out);
tp = out_p = out + len - 2; /* Just before '\n' */
while (char_type[ *out_p & UCHARMAX] & SPA)
out_p--; /* Remove trailing white spaces */
if (out_p < tp) {
*++out_p = '\n';
*++out_p = EOS;
if (len > 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

A line with just a whitespace char and a line-break is not trimmed (or is that intentional?). I think this could be >= 2 and ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is intentional, but it is good to double-check. From my reading of the code, the intent is that there is always at least a character, followed by a newline, followed by a terminating 0. This then makes the -2 valid. The loop implies that the character is assumed to be a "non-whitespace" one.

Firstly, this means that under the original assumptions, whether or not you actually do anything in the case of len == 2 is irrelevant.

The bug report only proves that the "non-whitespace" assumption is false. It doesn't prove that the character+\n+\0 assumption is false. So my decision to add a test for len > 2 (or, equivalently under the original assumption, len >= 2) is simply because it wasn't immediately obvious that len < 2 is truly impossible.

So, the only surprising bit is that in the case of an input of the form: whitespace character+\n+\0, it outputs the whitespace character. Considering that this is merely the preprocessor and the IDL lexer doesn't mind the white space, I figured this edge case didn't really matter.

{
char * out_p;
char * tp;
tp = out_p = out + len - 2; /* Just before '\n' */
while (out_p > out && char_type[ *out_p & UCHARMAX] & SPA)
Copy link
Contributor

Choose a reason for hiding this comment

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

... this can be changed to out_p >= out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not unless you can prove that the object that out points into has at least one byte preceding it:

C99 6.5.6 Additive operators ¶8:

When an expression that has integer type is added to or subtracted from a pointer, the result has the type of the pointer operand. If the pointer operand points to an element of an array object, and the array is large enough, the result points to an element offset from the original element such that the difference of the subscripts of the resulting and original array elements equals the integer expression. In other words, if the expression P points to the i-th element of an array object, the expressions (P)+N (equivalently, N+(P)) and (P)-N (where N has the value n) point to, respectively, the i+n-th and i−n-th elements of the array object, provided they exist. Moreover, if the expression P points to the last element of an array object, the expression (P)+1 points one past the last element of the array object, and if the expression Q points one past the last element of an array object, the expression (Q)-1 points to the last element of the array object. If both the pointer operand and the result point to elements of the same array object, or one past the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined. If the result points one past the last element of the array object, it shall not be used as the operand of a unary * operator that is evaluated.

So in the case of out_p == out and *out_p getting classified as white space, the result would be undefined behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point! I missed the undefined behavior wrt pointer subtraction. This could be rewritten by using an integer counter for number of chars to be stripped, but I agree that it's not worth the effort here.

out_p--; /* Remove trailing white spaces */
if (out_p < tp && !(char_type[ *out_p & UCHARMAX] & SPA)) {
*++out_p = '\n';
*++out_p = EOS;
}
}
if (mcpp_fputs( out, MCPP_OUT) == EOF)
cfatal( "File write error", NULL, 0L, NULL); /* _F_ */
Expand Down
Loading