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

mavgen: protect for too deep inlcude trees #495

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

olliw42
Copy link
Contributor

@olliw42 olliw42 commented Dec 31, 2020

protect against too deep include trees
useful for mavlink/mavlink#1537 (the include trees tend to become deeper and deeper ...)

@hamishwillee
Copy link
Contributor

@peterbarker This looks like a very good idea to me (eventually we'll hit the limit of inclusion, and this will save us a lot of time explaining to people why their dialect doesn't work)

@peterbarker
Copy link
Contributor

Perhaps this would be neater as:

count = 0
while True:
    if not expand_one_iteration():
       break;
    count += 1
    if count >= MAXIMUM_INCLUDE_FILE_NESTING:
        print("ERROR include tree is too deeply nested!")
        sys.exit(1)

Give-or-take an off-by-one error or two.

My reasoning is just that for loops are usually used for bounded iterations, so checking the number of times you iterated looks a little odd.

@lightmare
Copy link
Contributor

The original PR won't abort, because i will never be set to MAXIMUM_INCLUDE_FILE_NESTING. range(N) does not contain N.
(In other words, for i in range(N) is not the same thing as for(i = 0; i < N; ++i) ;))

The variant suggested by @peterbarker will work. I'd only note that while True: ... if something: break seems awkward, and the comparison should probably be strict, so as to not abort after doing exactly the allowed amount.

count = 0
while expand_one_iteration():
    count += 1
    if count > MAXIMUM_INCLUDE_FILE_NESTING:
        print("ERROR include tree is too deeply nested!")
        sys.exit(1)

@olliw42
Copy link
Contributor Author

olliw42 commented Apr 6, 2021

strange
I certainly had tested it for that occasion (by simply reducing the limit to a small value like 1 or 2 which triggers) and it worked for me (otherwise I wouldn't have dared to offer it). I just double checked again and still found it to work for me.
But I agree it's not so nice for python

the simplest probably would be

    for n in range(MAXIMUM_INCLUDE_FILE_NESTING+1):
        if n >= MAXIMUM_INCLUDE_FILE_NESTING:
            print("ERROR include tree is too deeply nested!")
            sys.exit(1)
        if not expand_oneiteration():
            break

works for me

@lightmare
Copy link
Contributor

Yeah if the range is extended by +1, it will get to the max. If you really want to use a for loop, put the error in its else branch, you don't need to check "are we past max rounds" each time.

for _ in range(MAXIMUM_INCLUDE_FILE_NESTING+1):
    if not expand_oneiteration():
        break
else:
    print("ERROR include tree is too deeply nested!")
    sys.exit(1)

But I still think the while loop is more easily understood, especially to people not familiar with nuances of how ranges and for-break-else work in Python.

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.

4 participants