Skip to content
This repository was archived by the owner on Mar 11, 2025. It is now read-only.

Limit nested filters to avoid stack overflow #979

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

djc
Copy link
Collaborator

@djc djc commented Mar 7, 2024

No description provided.

@djc djc requested review from Kijewski and GuillaumeGomez March 7, 2024 13:51
@GuillaumeGomez
Copy link
Collaborator

Can you add a ui test to see the error output too please?

@GuillaumeGomez
Copy link
Collaborator

I think you forgot the ui stderr file. ;)

@djc
Copy link
Collaborator Author

djc commented Mar 7, 2024

I haven't actually used the ui tests before... I finally figured out how to work the system, but not sure how to get a better error message.

@GuillaumeGomez
Copy link
Collaborator

Like I did in #954: by providing your own. :)

@djc djc force-pushed the filter-recursion branch from d890a0f to e1c6cd5 Compare March 7, 2024 14:29
@djc
Copy link
Collaborator Author

djc commented Mar 7, 2024

That's providing a separate error, but not inspecting the nom error to refine the error. Made an attempt which doesn't seem to work. Not sure how much more time I want to spend on this.

@Kijewski
Copy link
Collaborator

Kijewski commented Mar 7, 2024

Can you try

    fn filtered(i: &'a str, mut level: Level) -> ParseResult<'a, Self> {
        #[allow(clippy::type_complexity)]
        fn filter<'a>(
            i: &'a str,
            level: &mut Level,
        ) -> ParseResult<'a, (&'a str, Option<Vec<Expr<'a>>>)> {
            let (i, _) = char('|')(i)?;
            *level = level.nest(i)?.1;
            pair(ws(identifier), opt(|i| Expr::arguments(i, *level, false)))(i)
        }

        let (mut i, mut res) = Self::prefix(i, level)?;
        while let (j, Some((fname, args))) = opt(|i| filter(i, &mut level))(i)? {
            i = j;
            res = Self::Filter(fname, {
                let mut args = match args {
                    Some(inner) => inner,
                    None => Vec::new(),
                };
                args.insert(0, res);
                args
            })
        }
        Ok((i, res))
    }

@GuillaumeGomez
Copy link
Collaborator

If it becomes too much of a hassle for you, I can take over this PR. ;)

@djc djc force-pushed the filter-recursion branch from e1c6cd5 to f6dd29a Compare April 5, 2024 07:53
@djc
Copy link
Collaborator Author

djc commented Apr 5, 2024

@Kijewski that worked well, thanks!

I don't trust that the new ui test is actually working well? Feels like it shouldn't be triggering here (since the Level-based nesting should allow greater depth than the previous approach).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants