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.

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

Revert "Issue #6562 - Prevent undefined behaviour in in filter_stuff_func (#6563)" #6634

Merged
merged 1 commit into from
Feb 28, 2025

Conversation

progier389
Copy link
Contributor

@progier389 progier389 commented Feb 25, 2025

This reverts commit c27fe45.

Reverting c27fe45 that is causing
an heap corruption: buffer overflow

Issue: #6562

Reviewed by: @tbordaz , @droideck (Thanks!)

…tuff_func (389ds#6563)"

This reverts commit c27fe45.

Reverting c27fe45 that is causing
 an heap corruption: buffer overflow
@progier389
Copy link
Contributor Author

Reverting the commit, but this is strange as the change seems trivial.
I wonder if this change is not exposing another bug since the function returns -1 in some other cases ...

@progier389
Copy link
Contributor Author

Indeed the way get_filter_internal and get_filter_list handles fstr/ftmp seems a bit fishy
I think that original change is fine but we are mishandling things in error case.

@progier389
Copy link
Contributor Author

I do not understand why filter_escape_filter_value_extended is trying to use a ftmp value (it should only access to the filter data but not to the filter string ....)

@progier389
Copy link
Contributor Author

I still propose to revert the original fix (to avoid having failing tests)
then we will need some more work to understand the reason of the heap corruption

Copy link
Member

@droideck droideck left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@tbordaz tbordaz left a comment

Choose a reason for hiding this comment

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

LGTM

@progier389 progier389 merged commit 1795e4d into 389ds:main Feb 28, 2025
200 of 201 checks passed
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.

3 participants