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.

Sign up for GitHub

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

Jump to bottom

Cleanup some of the ncgen memory problems identified by Ed #558

Merged
merged 2 commits into from
May 18, 2018
Merged

Cleanup some of the ncgen memory problems identified by Ed #558

merged 2 commits into from
May 18, 2018

Conversation

DennisHeimbigner
Copy link
Collaborator

No description provided.

edhartnett
edhartnett reviewed Oct 31, 2017
View reviewed changes
ncgen/bytebuffer.c Outdated
@@ -250,7 +250,7 @@ bbTailpeek(Bytebuffer* bb, char* pelem)
char*
bbDup(const Bytebuffer* bb)
{
char* result = (char*)emalloc(bb->length+1);
char* result = (char*)malloc(bb->length+1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I never cast the result of mallocs. See https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc for reasons.

@edhartnett
Copy link
Contributor

Does this fix all the ncgen memory leaks? Quick work! ;-)

@DennisHeimbigner
Copy link
Collaborator Author

Only some of them. The global metadata and data trees created during the parse
are not reclaimed. Also, apparently is a leak inside libcurl that I cannot get rid of
(according to valgrind because I cannot get the google address checking to work).

@DennisHeimbigner
Copy link
Collaborator Author

I find the arguments against casting unpersuasive. But frankly
either way is ok as far as I am concernted.

@WardF
Copy link
Member

WardF commented Nov 1, 2017

Looks great, checking on ARM and Windows now.

@WardF WardF added this to the 4.5.1 milestone Nov 1, 2017
@WardF WardF self-assigned this Nov 1, 2017
@WardF
Copy link
Member

WardF commented Nov 1, 2017

Given the common wisdom of casting from 'malloc' I would agree with Dennis that a single post asserting differently is unpersuasive. But if he is unconcerned, so am I, and we can backtrack if an issue arises.

@edhartnett
Copy link
Contributor

Actually wasn't suggesting that he take all the malloc casts away in this case, just offering the observation for future consideration.

@WardF
Copy link
Member

WardF commented Nov 1, 2017

Seeing a handful of failures on OSX, still testing windows and ARM.

The failures can be seen here:

The failure described below seems to be common amongst the tests:

*** comparing binary against source CDL file *** 
*** creating C code for CAM file ref_camrun.cdl...
ncgen: output language  not implemented
ncgen(20763,0x7fffb69a0340) malloc: *** error for object 0x10a59a508: pointer being freed was not allocated

Update, 32-bit Windows tests passed. Will update as ARM and 64-bit Windows tests run.

@WardF
Copy link
Member

WardF commented Nov 1, 2017

Update: 64-bit Windows tests also passed.

@WardF
Copy link
Member

WardF commented Nov 2, 2017

As did ARM. This issue appears to be OSX specific. I will see if I can figure out what's going on.

WardF
WardF reviewed Nov 6, 2017
View reviewed changes
ncgen/main.c Outdated
@@ -299,8 +296,7 @@ main(
derror("%s: output language is null", progname);
return(1);
}
lang_name = (char*) emalloc(strlen(optarg)+1);
(void)strcpy(lang_name, optarg);
lang_name = estrdup(optarg);
Copy link
Member

Choose a reason for hiding this comment

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

This line is returning "" on OSX, for reasons I'm not sure about yet. Is there a problem with using the previous behavior for determining the language? Because "" is being returned we are falling through to "language not implemented" and then nullfree is crashing because it is trying to free memory lang_name that has not been allocated. We can fix that by checking for a length-zero lang_name before freeing, but that doesn't solve the output language not implemented problem.

Not sure if tagging people is required for these individual comments, so tagging @DennisHeimbigner just to be sure.

@edhartnett edhartnett mentioned this pull request Jan 19, 2018
Closed
@WardF WardF modified the milestones: 4.6.0, 4.6.1 Jan 25, 2018
@WardF WardF modified the milestones: 4.6.1, 4.7.0 Mar 20, 2018
@WardF
Copy link
Member

WardF commented Apr 20, 2018

Was able to fix the issue above, but running into a problem with ref_typescope test in ncdump/tst_ncgen4.sh when K=3, on OSX.

@WardF
Copy link
Member

WardF commented Apr 20, 2018

This pull request introduces 1 alert and fixes 10 when merging ee7bbb2 into e595a8c - view on lgtm.com

new alerts:

  • 1 for Missing return statement

fixed alerts:

  • 4 for Missing header guard
  • 3 for Wrong type of arguments to formatting function
  • 2 for Duplicate include guard
  • 1 for Wrong number of arguments to formatting function

Comment posted by lgtm.com

@WardF
Copy link
Member

WardF commented Apr 23, 2018

This pull request introduces 1 alert when merging ee0086f into 51de066 - view on lgtm.com

new alerts:

  • 1 for Missing return statement

Comment posted by lgtm.com

This was referenced May 12, 2018
Merged
Merged
@WardF WardF mentioned this pull request May 16, 2018
Merged
@WardF
Copy link
Member

WardF commented May 17, 2018

This code should be merged already, double-checking.

@WardF WardF merged commit 61a6c0c into master May 18, 2018
@DennisHeimbigner DennisHeimbigner deleted the cleanncgen.dmh branch June 30, 2018 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WardF WardF WardF left review comments

@edhartnett edhartnett edhartnett left review comments

Assignees

@WardF WardF

Labels
None yet
Projects
None yet
Milestone
4.6.2
Development

Successfully merging this pull request may close these issues.
3 participants
@DennisHeimbigner @edhartnett @WardF