Ticket #105 (closed cage: fixed)

Opened 6 years ago

Last modified 6 years ago

Enforce NULL checks on C function arguments

Reported by: Infinoid Owned by: infinoid
Priority: normal Milestone:
Component: core Version:
Severity: medium Keywords: headerizer
Cc: Language:
Patch status: Platform:

Description

In  http://www.nntp.perl.org/group/perl.perl6.internals/2008/12/msg49677.html, Nicholas Clark suggested we autogenerate some checks for NULL pointers in C function arguments. This seems like a good idea, so I'm doing so.

I've already found a few such issues, two of which are fixed here:  http://www.parrotvm.org/svn/parrot/revision?rev=34525

I've got at least one more issue that I haven't found the right solution for yet (Parrot_ascii_charset_ptr is NULL in miniparrot, triggering an assert failure when string_make_direct is called). So it's still a work in progress. But I'm going to post the diffs of what I've done so far.

Attachments

diff.trunk.assert_args.txt Download (19.0 KB) - added by jkeenan 6 years ago.
Work done so far in the 'assert_args' branch.
headerizer-asserts-as-local-variable-decl.patch Download (2.2 KB) - added by Infinoid 6 years ago.
tweak-test.patch Download (1.7 KB) - added by Infinoid 6 years ago.

Change History

  Changed 6 years ago by Infinoid

I've applied my work so far to trunk, thus, the attachments here have been rendered obsolete. For those changes, please see subversion revs r34719, r34720, r34721, and r34722.

The syntax changed somewhat from the original version. Thanks to some feedback from Andy Lester, the output has been better organized, and the syntax has been made a bit prettier. The per-function checks now look like: "ASSERT_ARGS(check_invoke_type);"

  Changed 6 years ago by Infinoid

One additional note, I've added a codingstd test to make sure the assert macros are actually being used. Implementation of this feature will be complete once t/codingstd/c_arg_assert.t passes. (I currently still have 1064 functions left to tag.)

Changed 6 years ago by jkeenan

Work done so far in the 'assert_args' branch.

  Changed 6 years ago by jkeenan

I created the 'assert_args' branch in SVN to poke away at this with Infinoid's coaching. The file attached, diff.trunk.assert_args.txt, shows the results so far, as recorded by

svn diff https://svn.perl.org/parrot/trunk@34776 https://svn.perl.org/parrot/branches/assert_args@HEAD

So far I've reduced the count of unused assert macros by approximately 50. If I've made any obvious errors in the C-code, please correct them in this branch. I'll resume work in this branch this weekend, deferring merging to a later point.

Thank you very much.
kid51

follow-up: ↓ 5   Changed 6 years ago by Infinoid

The current state of ASSERT_ARGS() is such that it needs to be placed directly after the local variables. Not doing this results in an "ISO C90 forbids mixing declarations and code" warning, or something along those lines.

One major complaint I've had while doing this is when a function has a significant portion of its logic in the local variable initializers. In order to do the best possible job, ASSERT_ARGS() should go at the very top of the function, so it can protect that initialization logic as well. I've got a patch that allows this, by making the assert code look like a local variable itself. I haven't tested whether the compiler actually *does* the asserts or just optimizes the whole thing out yet, but it does clean up the syntax nicely. I'm attaching that as headerizer-asserts-as-local-variable-decl.patch.

Unfortunately it means there are a ton of ASSERT_ARGS() which need to be moved up. I'm going to see how much of that I can do with a simple regex. But for now, tweak-test.patch adds a second test which will generate a list of functions whose ASSERT_ARGS() are in the wrong spot.

Changed 6 years ago by Infinoid

Changed 6 years ago by Infinoid

in reply to: ↑ 4 ; follow-up: ↓ 6   Changed 6 years ago by Infinoid

Replying to Infinoid:

I've just tested headerizer-asserts-as-local-variable-decl.patch, and the assertions crash when they're supposed to. So that means it's a viable solution, and definitely preferred in the long run. Unfortunately, when the assertions fail, the resulting message talks about it crashing in the file and line number of PARROT_ARG_ASSERT, not in the caller. So I'm going to have to add some FILE and LINE magic here; I should have an updated patch for that tonight.

Unfortunately it means there are a ton of ASSERT_ARGS() which need to be moved up. I'm going to see how much of that I can do with a simple regex. But for now, tweak-test.patch adds a second test which will generate a list of functions whose ASSERT_ARGS() are in the wrong spot.

I've whipped up a perl regex script to automatically perform this change, which works in the majority of cases. So I think this conversion won't take too long, and shouldn't slow down additional tagging in the meantime.

Mark

in reply to: ↑ 5   Changed 6 years ago by Infinoid

  • status changed from new to closed
  • resolution set to fixed

Replying to Infinoid:

I've just tested headerizer-asserts-as-local-variable-decl.patch, and the assertions crash when they're supposed to. So that means it's a viable solution, and definitely preferred in the long run. Unfortunately, when the assertions fail, the resulting message talks about it crashing in the file and line number of PARROT_ARG_ASSERT, not in the caller. So I'm going to have to add some FILE and LINE magic here; I should have an updated patch for that tonight.

Done.

I've whipped up a perl regex script to automatically perform this change, which works in the majority of cases. So I think this conversion won't take too long, and shouldn't slow down additional tagging in the meantime.

Done.

And now that ASSERT_ARGS() always appears directly after a function's opening brace, that made it trivial to write a perl script to tag the rest of the functions automatically. So that's done too, and the c_arg_assert.t test is passing.

So I've finished this work and tested everything and fixed up all of the issues and everything's looking good here. I've merged the branch work back into trunk and gotten everything looking nice there, too, so I removed the branch (and its tag).

We'll see whether it works on all platforms and compilers. Other than that, the only points I can see for future work on this are: * automatic argument checking inserted by pmc2c * automatic argument checking inserted by ops2c * move the ASSERT_ARGS_* macros to the .c file's static section... there's no reason for them to be in the global headers.

Since we've accomplished the goal of this ticket, I am closing it. Andy++ and kid51++ for their help and feedback.

Note: See TracTickets for help on using tickets.