Remove all variable-length arrays on the stack #45
Conversation
|
You can ignore the failed appveyor build, I haven't finished hooking that up yet in issue #42 (I didn't expect it to start trying heh) |
|
I'll take a look over the patch tomorrow, I kicked off a Firefox tryserver CI run in the meantime: Note that this picks up 2a49189 too - since these two changesets passed the Travis-CI tests I am not too worried, but this will at least exercise the Firefox integration code and unit test. |
| @@ -49,118 +52,116 @@ verify_full(void) | |||
| // Number of clients to simulate. | |||
| const int nclients = 10; | |||
|
|
|||
| // New scope to avoid goto weirdness | |||
There was a problem hiding this comment.
Was the cause of this comment (and the new scope) related to VLAs, or is this just coincidental?
There was a problem hiding this comment.
It'd be easier to see the change here as two separate commits in any case :)
There was a problem hiding this comment.
This was related, but I'll move it into a separate commit for better readability.
In the earlier code, I had some VLAs defined in the middle of the function. This meant that there was a new stack frame created halfway down the function body and the cleanup label ended up in being placed in this new child stack frame. The PCHECK_C macro then wouldn't work in the first half of the function body (the parent stack frame), because the PCHECK_C macro contained a goto that was trying to jump into the child stack frame that didn't exist. Putting the new explicit scope in there meant that the cleanup label was in the parent stack frame and then PCHECK_C would work everywhere in the function...
| @@ -192,6 +192,10 @@ verify_full(void) | |||
| free(for_server_a); | |||
| if (for_server_b) | |||
| free(for_server_b); | |||
| if (output) | |||
There was a problem hiding this comment.
I guess it is not enforced by the current style checker settings but it's safer to always use braces, the Mozilla style guide mentions the "dangling else" bug.
"Always brace controlled statements, even a single-line consequent of an if else else. This is redundant typically, but it avoids dangling else bugs, so it's safer at scale than fine-tuning."
The more common bug I see when braces aren't enforced around control structures though is the simpler "goto fail"-style bug, that is that a new patch author misreads the existing code and does something like:
- if (a)
- doA();
+ if (a && b)
+ doA();
+ doB();
I don't think it's worth really worrying about in this patch since this style is already used elsewhere, I will make the case in another issue that this should be enforced and the code auto-formatted :)
There was a problem hiding this comment.
Yes, enforcing this everywhere sounds good to me.
henrycg
force-pushed
the
remove-variable-length-arrays
branch
from
d2e3015
to
75f9b38
Compare
|
I just split the commit into two: one to remove the VLAs and one to fix the formatting. |
This commit removes all stack-allocated VLAs.
Last week, @sadjad convinced me that stack-allocated arrays are dangerous: If an attacker can control the size of the allocated array, the attacker can allocate a huge array on the stack and crash the program using a stack overflow.
I don't think that this is a terribly pressing concern for libprio, but since MSVC doesn't support VLAs anyways, we might as well get rid of them entirely.