check for cycles during indirect reference resolution

[jan-sutter]

This PR "fixes" #1096

[EliotJones]

Thanks for the contribution! This looks like a reasonable fix, the only concern I have is whether reference generation number is needed in the lookup set too, but I think that's such an edge case as to be virtually impossible. Let's merge it and see if the integration tests all pass. I'll close the issue you raised alongside this since this PR should address it.

[BobLd]

@jan-sutter thanks a lot for opening the issue and creating a PR. I think the current approach would add non negligible overhead. Is there no other way to achieve a similar approach than maintaining a collection?

I'll have a look too on my side and let you know

[EliotJones]

Sorry @BobLd I jumped the gun here, I'm not sure there is a better fix to be honest without much more rework. We're already quite heavy on object creation so I imagine the performance impact in non-negligible but also not much beyond baseline. The problem with these recursive functions that can loop is you always need some way to break out of an invalid file, the other alternative would be pass a counter integer and just throw if it exceeds some max-val iterations but I think this approach allows us to parse the file rather than throwing which is a nicer fix.

[BobLd]

@EliotJones makes sense - I've created #1101 to track the full IndirectReference instead of just the ObjectNumber

[theolivenbaum]

Minor comment, but shouldn't this be checked using a HashSet instead of a list?

[BobLd]

@theolivenbaum 100% agreed, you can either create a PR or ill push the change shortly

[theolivenbaum]

Perfect, I'll send a PR in a bit then!

[theolivenbaum]

Done: #1112