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

OpenXmlElement.CreateXmlReader: Prevent XmlConvertingReaderFactory from using a disposed TextReader #940

Conversation

sorensenmatias
Copy link
Contributor

@sorensenmatias sorensenmatias commented May 26, 2021

OpenXmlElement.CreateXmlReader: Prevent XmlConvertingReaderFactory from using a disposed TextReader.

@ghost
Copy link

ghost commented May 26, 2021

CLA assistant check
All CLA requirements met.

@sorensenmatias sorensenmatias changed the title OpenXmlElement.CreateXmlReader: Prevent XmlConvertingReaderFactory from using a disposed XmlReader OpenXmlElement.CreateXmlReader: Prevent XmlConvertingReaderFactory from using a disposed TextReader May 26, 2021
Copy link
Member

@twsouthwick twsouthwick left a comment

Choose a reason for hiding this comment

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

Can you include a regression test for this!

Thanks for the fix!

src/DocumentFormat.OpenXml/OpenXmlElement.cs Show resolved Hide resolved
@twsouthwick
Copy link
Member

/azp run

@sorensenmatias
Copy link
Contributor Author

@twsouthwick I added a test that shows the problem I encountered, and confirms the fix.

@sorensenmatias
Copy link
Contributor Author

Is there anything else I can help with to get this merged?

@twsouthwick
Copy link
Member

/azp run

twsouthwick
twsouthwick previously approved these changes Jun 24, 2021
Copy link
Member

@twsouthwick twsouthwick left a comment

Choose a reason for hiding this comment

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

LGTM!

@twsouthwick
Copy link
Member

@sorensenmatias I've been pretty busy and so the delay is on me here. Once the tests pass, this can be merged in. Thanks for the contribution!

@twsouthwick twsouthwick added this to the v2.13.1 milestone Jun 24, 2021
@twsouthwick
Copy link
Member

/azp run

twsouthwick
twsouthwick previously approved these changes Jun 24, 2021
@sorensenmatias
Copy link
Contributor Author

Thanks @twsouthwick. I am not sure I completely follow though, is there a problem with the tests currently?

@twsouthwick
Copy link
Member

Yeah - we have warnings as error on, and there's a style warning that's getting flagged

@twsouthwick
Copy link
Member

/azp run

@twsouthwick
Copy link
Member

I've pushed a fix for it (needed to add a copyright)

@sorensenmatias
Copy link
Contributor Author

Thanks. I fixed one more warning regarding UTF8 BOM.

@twsouthwick
Copy link
Member

/azp run

@twsouthwick twsouthwick merged commit 4dc9661 into dotnet:main Jul 4, 2021
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.

2 participants