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.

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

Use strcpy rather than strdup #5610

Merged
merged 4 commits into from Jun 15, 2018

Conversation

Projects
None yet
2 participants
@geographika
Member

geographika commented Jun 14, 2018

I guess I must have missed this in the original pull request. This was a fix supplied by @tbonfort and relates to this - #5277

This pull request includes the code from tbonfort@bacf44f

If strdup is used on Windows/MSVC then char variables don't get set correctly (at least when using the debugger).

mapstring.c Outdated
pszReturn = strdup( pszString );
pszReturn = msSmallMalloc(strlen(pszString) + 1);

This comment has been minimized.

@rouault

rouault Jun 14, 2018

Contributor

a more efficient implementation would store strlen(pszString) in a nStringLength variable for reuse. And instead of strcpy(), you would do memcpy(pszReturn, pszString, nStringLength + 1 /* nul terminated byte*/)

@geographika

This comment has been minimized.

Member

geographika commented Jun 14, 2018

@rouault - thanks for the review. msStrdup is used throughout the codebase so I don't want to add any overhead. Could any string lengths be greater than an unsigned int used for nStringLength?

mapstring.c Outdated
pszReturn = strdup( pszString );
unsigned int nStringLength = strlen(pszString);

This comment has been minimized.

@rouault

rouault Jun 14, 2018

Contributor

use size_t for type safety instead of unsigned int

mapstring.c Outdated
pszReturn = strdup( pszString );
unsigned int nStringLength = strlen(pszString);
char *pszReturn = malloc(nStringLength + 1);
memcpy(pszReturn, pszString, nStringLength + 1); /* null terminated byte*/

This comment has been minimized.

@rouault

rouault Jun 14, 2018

Contributor

memcpy() should be done after the below NULL check

mapstring.c Outdated
pszReturn = strdup( pszString );
unsigned int nStringLength = strlen(pszString);

This comment has been minimized.

@rouault

rouault Jun 14, 2018

Contributor

Being in a .c file, the variable declarations should be done at the beginning of the function to please MSVC

size_t nStringLength;
char* pszReturn;
mapstring.c Outdated
{
char *pszReturn;
size_t nStringLength = strlen(pszString) + 1; /* null terminated byte*/

This comment has been minimized.

@rouault

rouault Jun 14, 2018

Contributor

the declaration and its affectation of both variables should be separated. Namely after the if (pszString == NULL) pszString = ""; check
You're close ;-)

mapstring.c Outdated
pszReturn = strdup( pszString );
if (pszReturn == NULL) {
fprintf(stderr, "msSmallMalloc(): Out of memory allocating %ld bytes.\n",
(long)strlen(pszString));

This comment has been minimized.

@rouault

rouault Jun 14, 2018

Contributor

strlen(pszString) could be replaced by nStringLength

@geographika

This comment has been minimized.

Member

geographika commented Jun 15, 2018

@rouault - thanks for your patience! Variables now set after the NULL check.

@rouault rouault merged commit af14610 into mapserver:branch-7-2 Jun 15, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

geographika added a commit to geographika/mapserver that referenced this pull request Jun 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment