cleanup: Convert all variable length arrays to heap allocations #100

JFreegman · 2020-10-28T03:04:23Z

VLA's are inherently unsafe so the safest option is to not use them. The trade off here is a lot of extra code for mallocs/frees and more possible memory leaks. But I think that's a small price to pay for greatly reducing the possibility of stack corruption.

I also added the -Wvla compile option to the Makefile, which gives a warning when VLA's are used.

This change is

robinlinden · 2020-10-28T21:15:06Z

src/toxic.c

@@ -837,6 +888,7 @@ static Tox *load_tox(char *data_path, struct Tox_Options *tox_opts, Tox_Err_New

        if (store_data(m, data_path) == -1) {
            exit_toxic_err("failed in load_tox", FATALERR_FILEOP);
+            return NULL;


You added a lot of return NULL; after exit_toxic_err calls. Don't they exit?

I added those to see if they would get rid of a strange codefactor warning, which they did. So I can either leave them (harmless) or @iphydf can fix the false positive.

Going to leave them in even though CodeFactor no longer is a required check?

src/qr_code.c

src/misc_tools.h

JFreegman · 2020-10-29T04:48:05Z

Two of the CodeFactor warnings are false positives (popen() and system()) and one about complexity is completely useless (doesn't give me a line or function) and can't be resolved.

All of the Codacity warnings about strlen() are false positives. I don't quite understand the warning:

Does not handle strings that are not \0-terminated; if given one it may perform an over-read (it could cause a crash if unprotected) (CWE-126).

A C string is null terminated by definition. The warning amounts to "if this is buggy code it might crash".

src/toxic.c

JFreegman · 2020-10-30T17:32:00Z

Three of the four codefactor issues have been fixed. To summarize:

system("clear") was replaced with a printf escape sequence with the same functionality
Some logic in dir_match() was split into a helper function
The misc_tools.c warning was due to the entire source file's complexity being summed, so I just moved a few of the functions that were only used in one place over to their respective source file. Also did a bit of refactoring.

Still don't know what to do about popen()

VLA's are inherently unsafe so the safest option is to not use them

Instead of using various different forms of string arrays and having to handle them differently for string completion, we now always use char pointer arrays. This allows us to remove some large stack allocations, remove a bunch of confusing defines that keep track of global array sizes, and generally unclutters the code so it's easier to read.

robinlinden · 2020-11-01T20:49:13Z

src/toxic.c

@@ -837,6 +888,7 @@ static Tox *load_tox(char *data_path, struct Tox_Options *tox_opts, Tox_Err_New

        if (store_data(m, data_path) == -1) {
            exit_toxic_err("failed in load_tox", FATALERR_FILEOP);
+            return NULL;


Going to leave them in even though CodeFactor no longer is a required check?

Also moved some single use functions from misc_tools to their respective files

JFreegman added the cleanup Internal code cleanup, possibly affecting semantics, e.g. deleting a deprecated feature. label Oct 28, 2020

JFreegman added this to the 0.8.x milestone Oct 28, 2020

JFreegman assigned nurupo and zoff99 Oct 28, 2020

restyled-io bot mentioned this pull request Oct 28, 2020

Restyle Convert all variable length arrays to heap allocations #101

Closed

JFreegman force-pushed the say_no_to_VLAs branch from d97782d to 9b98852 Compare October 28, 2020 03:05

JFreegman changed the title ~~Convert all variable length arrays to heap allocations~~ cleanup: Convert all variable length arrays to heap allocations Oct 28, 2020

JFreegman force-pushed the say_no_to_VLAs branch 3 times, most recently from 961a962 to 9dafa15 Compare October 28, 2020 04:54

restyled-io bot mentioned this pull request Oct 28, 2020

Restyle cleanup: Convert all variable length arrays to heap allocations #102

Closed

JFreegman force-pushed the say_no_to_VLAs branch 6 times, most recently from 198b851 to bd4fc6a Compare October 28, 2020 18:13

robinlinden requested changes Oct 28, 2020

View reviewed changes

pull-request-attention bot assigned JFreegman and robinlinden and unassigned JFreegman Oct 28, 2020

JFreegman force-pushed the say_no_to_VLAs branch 4 times, most recently from 88bff06 to e7ae063 Compare October 29, 2020 04:27

JFreegman force-pushed the say_no_to_VLAs branch from e7ae063 to a4468c9 Compare October 29, 2020 05:22

robinlinden requested changes Oct 29, 2020

View reviewed changes

src/toxic.c Show resolved Hide resolved

src/toxic.c Show resolved Hide resolved

pull-request-attention bot unassigned robinlinden Oct 29, 2020

pull-request-attention bot assigned JFreegman and unassigned robinlinden Oct 29, 2020

JFreegman force-pushed the say_no_to_VLAs branch from a4468c9 to 351f38a Compare October 29, 2020 21:56

robinlinden approved these changes Oct 29, 2020

View reviewed changes

JFreegman force-pushed the say_no_to_VLAs branch 3 times, most recently from 060f52c to 69f088a Compare October 30, 2020 17:25

JFreegman force-pushed the say_no_to_VLAs branch 3 times, most recently from 1d28c6f to 2a4b4e3 Compare November 1, 2020 14:54

JFreegman requested a review from a team as a code owner November 1, 2020 15:04

JFreegman force-pushed the say_no_to_VLAs branch 2 times, most recently from 53c0eaa to d6f0ca2 Compare November 1, 2020 16:11

JFreegman added 2 commits November 1, 2020 11:29

Convert all variable length arrays to heap allocations

2b43340

VLA's are inherently unsafe so the safest option is to not use them

JFreegman force-pushed the say_no_to_VLAs branch from d6f0ca2 to 2d10677 Compare November 1, 2020 16:31

JFreegman requested a review from robinlinden November 1, 2020 16:32

pull-request-attention bot assigned robinlinden Nov 1, 2020

JFreegman requested review from a team and removed request for a team November 1, 2020 16:33

robinlinden approved these changes Nov 1, 2020

View reviewed changes

pull-request-attention bot unassigned robinlinden Nov 1, 2020

Refactor a few complex functions

e7a0c32

Also moved some single use functions from misc_tools to their respective files

JFreegman force-pushed the say_no_to_VLAs branch from 2d10677 to e7a0c32 Compare November 1, 2020 20:55

JFreegman unassigned nurupo and zoff99 Nov 1, 2020

JFreegman merged commit e7a0c32 into TokTok:master Nov 1, 2020

JFreegman deleted the say_no_to_VLAs branch November 1, 2020 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup: Convert all variable length arrays to heap allocations #100

cleanup: Convert all variable length arrays to heap allocations #100

JFreegman commented Oct 28, 2020 •

edited by iphydf

Loading

robinlinden Oct 28, 2020

JFreegman Oct 29, 2020

robinlinden Nov 1, 2020

JFreegman commented Oct 29, 2020 •

edited

Loading

JFreegman commented Oct 30, 2020

robinlinden Nov 1, 2020

cleanup: Convert all variable length arrays to heap allocations #100

cleanup: Convert all variable length arrays to heap allocations #100

Conversation

JFreegman commented Oct 28, 2020 • edited by iphydf Loading

robinlinden Oct 28, 2020

Choose a reason for hiding this comment

JFreegman Oct 29, 2020

Choose a reason for hiding this comment

robinlinden Nov 1, 2020

Choose a reason for hiding this comment

JFreegman commented Oct 29, 2020 • edited Loading

JFreegman commented Oct 30, 2020

robinlinden Nov 1, 2020

Choose a reason for hiding this comment

JFreegman commented Oct 28, 2020 •

edited by iphydf

Loading

JFreegman commented Oct 29, 2020 •

edited

Loading