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

Crash fixes for CopyInherited #7709

Merged
merged 2 commits into from
Nov 1, 2022
Merged

Crash fixes for CopyInherited #7709

merged 2 commits into from
Nov 1, 2022

Conversation

Sorpirit
Copy link
Contributor

Found two bug in the CopyInherited class.

Operating System: Windows
DocFX Version Used: 2.58.9
Template used: default

1. NullReferenceException exception crash

Steps to Reproduce:

  1. Try to generate documentation for constructor of the class without inherited documentation for constructor
class A : IDispoable
{
   /// <inheritdoc /> 
   public A() { }
   
   /// <inheritdoc /> 
   public Dispose() { }
}

Expected Behavior:
Inheritdoc will be ignored(hopefully warning would be thrown).
Actual Behavior:
Program crashes with NullReferenceException.
Suggesting fix:
Add null check(line 45) before getting last item from Inheritance list(line 49)

2. StackOverflow crash

Steps to Reproduce:

  1. Try to generate documentation for IEnumerator when inheriting both from IEnumerator and IEnumerable.
class A : IEnumerator<Other>, IEnumerable<Other>
{
   ...
   /// <inheritdoc />
   IEnumerator<Other> IEnumerable<Other>.GetEnumerator()
   ...

   ...
   /// <inheritdoc />
   IEnumerator IEnumerable.GetEnumerator()
   ...
}

Expected Behavior:
Documentation would be successfully generated.
Actual Behavior:
Program crashes with StackOverflow.
Suggesting fix:
Add check(line 131) when copying from src to dest if it is the same in order to stop recursion.

Copy link
Contributor

@yufeih yufeih left a comment

Choose a reason for hiding this comment

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

Thank you for providing this fix! The fix looks good to me, but to verify the fix and to prevent future regressions, could you help add some test cases to cover the two scenarios in PR description? #7696 contains some examples to add a test case.

@herohua herohua added the v2 label Dec 21, 2021
@yufeih yufeih enabled auto-merge (squash) November 1, 2022 03:27
@yufeih yufeih merged commit 86db612 into dotnet:dev Nov 1, 2022
@yufeih yufeih added the bug-fix Makes the pull request to appear in "Bug Fixes" section of the next release note label Dec 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix Makes the pull request to appear in "Bug Fixes" section of the next release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants