Page MenuHomePhabricator

Prevent nullptr dereferences in C API for counting attributes.
ClosedPublic

Authored by maleadt on Nov 8 2016, 5:11 AM.

Details

Summary

If the function or instruction doesn't have any attributes, AttributeSetNode::get returns a null pointer.

Event Timeline

maleadt updated this revision to Diff 77172.Nov 8 2016, 5:11 AM
maleadt retitled this revision from to Prevent nullptr dereferences in C API for counting attributes..
maleadt updated this object.
maleadt added reviewers: deadalnix, bkramer.
maleadt updated this revision to Diff 77352.Nov 9 2016, 7:44 AM

Fixed similar issue with the AttributeRef getters.

deadalnix added inline comments.Nov 9 2016, 10:52 AM
tools/llvm-c-test/echo.cpp
382 ↗(On Diff #77352)

Why is this necessary ?

817 ↗(On Diff #77352)

dito

maleadt added inline comments.Nov 9 2016, 11:05 AM
tools/llvm-c-test/echo.cpp
382 ↗(On Diff #77352)

It isn't, I added it as a means of a test, as it segfaults without the proposed nullptr checks.
Or where should I put tests for the C API (first time contributor here, forgive my ignorance)?

deadalnix added inline comments.Nov 9 2016, 1:58 PM
tools/llvm-c-test/echo.cpp
382 ↗(On Diff #77352)

Yes but on the other hand, it doesn't test this case for all the functions inside the test. I think this is the wrong tradeof.

However, that'd be great to add tests for these function somewhere else.

maleadt updated this revision to Diff 77989.Nov 15 2016, 6:46 AM

Move tests in their own attributes unit test file.

maleadt updated this revision to Diff 77990.Nov 15 2016, 6:48 AM

Format the new tests.

deadalnix accepted this revision.Nov 15 2016, 10:19 AM
deadalnix edited edge metadata.
deadalnix added inline comments.
tools/llvm-c-test/main.c
91–94

I would create just one, but if you think that's better that way, I'm good with it.

This revision is now accepted and ready to land.Nov 15 2016, 10:19 AM
maleadt added inline comments.Nov 15 2016, 11:10 AM
tools/llvm-c-test/main.c
91–94

No real preference at my side. I modeled it after the metadata tests, which also seemed pretty fine grained.

I don't have commit access, could somebody land this? Thanks!

Ok, let me merge this.