Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#5907 closed enhancement (fixed)

Replaces strdup() / free() by CPLStrdup() / CPLFree()

Reported by: yzhong Owned by: warmerdam
Priority: normal Milestone: 2.0.0
Component: default Version: svn-trunk
Severity: normal Keywords: strdup
Cc: tamas

Description

The strdup() function is deprecated under Windows and not safe to use. I have replace a large number of strdup() to CPLStrdup(), and their corresponding free() to CPLFree().

There are a few left, which I am not too sure whether changing will cause other problems. Therefore, I leave them untouched. They are in the following files:

gdal\frmts\pcidsk\sdk\pcidsk_shape.h gdal\ogr\ogrsf_frmts\geojson\libjson\json_object.c gdal\port\cpl_vsisimple.cpp

If you do a global search, you will find them all.

Also, in the original code, I notice that some memory that are allocated by strdup() are released by CPLFree(). Therefore, if you only look at the changes according to the the patch file, CPLStrdup() and CPLFree() don't always come in pair.

I understand that there might be cases where memory is allocated by CPLStrdup() but released by free() in the original code. I don't have time to go through all these yet because there are just too many free() calls in the entire project. But I assume that it is fine since we have define to CPLFree to free.

#define CPLFree free

Attachments (1)

patches.zip (6.8 KB ) - added by yzhong 9 years ago.

Download all attachments as: .zip

Change History (6)

by yzhong, 9 years ago

Attachment: patches.zip added

comment:1 by warmerdam, 9 years ago

IMHO the so called deprecation of strdup() in MSVC is no reason for us to stop using it. I am dubious about the value of this change.

comment:2 by Even Rouault, 9 years ago

Resolution: fixed
Status: newclosed

Ah, just seeing @warmerdam comment now I've committed... We had a discussion with yzhong on https://github.com/OSGeo/gdal/pull/51 and apparently the use of strdup() causes heap memory corruption for some reason which aren't clear to me either. Hopefull the changes will not cause harm.

trunk r28831 "Replaces strdup() / free() by CPLStrdup() / CPLFree() to avoid issues on Windows with newer MSVC versions (derived patch by yzhong + extra fixes from mine, #5907)"

I dropped the change in libjson-c as it is an external library whose code is imported in GDAL, so change in it should be contributed to the upstream project (which doesn't use GDAL CPLxxx routines)

comment:3 by Even Rouault, 9 years ago

From https://github.com/OSGeo/gdal/pull/55 : trunk r28971 "VSIStrdup(): avoid using strdup() (#5907)"

comment:4 by Even Rouault, 9 years ago

Cc: tamas added

Tamas, I was wondering if you'd have any clue why strdup() would suddenly cause problems with newer MSVC versions (VS2013 apparently). I haven't yet seen a really convincing explanation, although apparently changing strdup() by malloc()+strcpy() seems to improves things for the reporter.

comment:5 by Even Rouault, 9 years ago

Milestone: 2.02.0.0

Milestone renamed

Note: See TracTickets for help on using tickets.