Skip to content

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Jun 8, 2020

If the current character is a line break character, it cannot be a tab
or space character, so we would always fail with an invalid sequence
error. Obviously, these conditions are meant to be exclusive.

If the current character is a line break character, it cannot be a tab
or space character, so we would always fail with an invalid sequence
error.  Obviously, these conditions are meant to be exclusive.
@nikic
Copy link
Member

nikic commented Jun 8, 2020

This change looks right to me, but it's not immediately clear how it leads to a segfault. From a cursory reading, it would just caused an invalid sequence error.

@cmb69
Copy link
Member Author

cmb69 commented Jun 8, 2020

The segfaults happens in the final call to that function, because the following condition is false then (lb_cnt==1, lb_ptr==0):

if ((in_pp == NULL || in_left_p == NULL) && lb_cnt == lb_ptr) {

but shortly after in_pp and in_left_p (both being NULL) are dereferenced. It seems to me that the additional constraint (&& lb_cnt == lb_ptr) is not correct, but even if I remove it, there would be three warnings regarding invalid sequences, although there are only two invalid sequences in the test.

@nikic
Copy link
Member

nikic commented Jun 8, 2020

@cmb69 Shouldn't we fix that condition as well? It's not clear to me that there is no other code path that could run into the same issue.

If `in_pp == NULL || in_left_p == NULL` is true, we hit a segfault if
we are not returning right away.  Therefore, the additional constraints
don't make sense.
@cmb69
Copy link
Member Author

cmb69 commented Jun 8, 2020

Yes, you're right. These constraints don't make sense.

@cmb69
Copy link
Member Author

cmb69 commented Jun 8, 2020

Thanks! Applied as 12c59f6.

@cmb69 cmb69 closed this Jun 8, 2020
@cmb69 cmb69 deleted the cmb/74267 branch June 8, 2020 21:30
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