# [PATCH] compat: move strdup(3) replacement to its own file 5 messages from 2016-09-03 to 2016-09-08. Participants: René Scharfe, Johannes Schindelin, Junio C Hamano. Thread: https://gitlist.dev/t/43997 ## René Scharfe, 2016-09-03 15:59 Subject: [PATCH] compat: move strdup(3) replacement to its own file Message-ID: <89725a44-8afa-1eb1-732a-23b1e264616c@web.de> URL: https://gitlist.dev/e/89725a44-8afa-1eb1-732a-23b1e264616c%40web.de ``` Move our implementation of strdup(3) out of compat/nedmalloc/ and allow it to be used independently from USE_NED_ALLOCATOR. This reduces the difference of our copy of nedmalloc from the original, making it easier to update, and allows for easier testing and reusing of our version of strdup(). Signed-off-by: Rene Scharfe --- Makefile | 17 ++++++++++++++--- compat/nedmalloc/nedmalloc.c | 16 ---------------- compat/strdup.c | 11 +++++++++++ git-compat-util.h | 8 ++++++++ 4 files changed, 33 insertions(+), 19 deletions(-) create mode 100644 compat/strdup.c diff --git a/Makefile b/Makefile index d96ecb7..7f18492 100644 --- a/Makefile +++ b/Makefile @@ -296,6 +296,11 @@ all:: # Define USE_NED_ALLOCATOR if you want to replace the platforms default # memory allocators with the nedmalloc allocator written by Niall Douglas. # +# Define OVERRIDE_STRDUP to override the libc version of strdup(3). +# This is necessary when using a custom allocator in order to avoid +# crashes due to allocation and free working on different 'heaps'. +# It's defined automatically if USE_NED_ALLOCATOR is set. +# # Define NO_REGEX if you have no or inferior regex support in your C library. # # Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the @@ -1456,8 +1461,14 @@ ifdef NATIVE_CRLF endif ifdef USE_NED_ALLOCATOR - COMPAT_CFLAGS += -Icompat/nedmalloc - COMPAT_OBJS += compat/nedmalloc/nedmalloc.o + COMPAT_CFLAGS += -Icompat/nedmalloc + COMPAT_OBJS += compat/nedmalloc/nedmalloc.o + OVERRIDE_STRDUP = YesPlease +endif + +ifdef OVERRIDE_STRDUP + COMPAT_CFLAGS += -DOVERRIDE_STRDUP + COMPAT_OBJS += compat/strdup.o endif ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT @@ -2029,7 +2040,7 @@ endif ifdef USE_NED_ALLOCATOR compat/nedmalloc/nedmalloc.sp compat/nedmalloc/nedmalloc.o: EXTRA_CPPFLAGS = \ - -DNDEBUG -DOVERRIDE_STRDUP -DREPLACE_SYSTEM_ALLOCATOR + -DNDEBUG -DREPLACE_SYSTEM_ALLOCATOR compat/nedmalloc/nedmalloc.sp: SPARSE_FLAGS += -Wno-non-pointer-null endif diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c index 2d4ef59..1cc31c3 100644 --- a/compat/nedmalloc/nedmalloc.c +++ b/compat/nedmalloc/nedmalloc.c @@ -948,22 +948,6 @@ void **nedpindependent_comalloc(nedpool *p, size_t elems, size_t *sizes, void ** return ret; } -#ifdef OVERRIDE_STRDUP -/* - * This implementation is purely there to override the libc version, to - * avoid a crash due to allocation and free on different 'heaps'. - */ -char *strdup(const char *s1) -{ - size_t len = strlen(s1) + 1; - char *s2 = malloc(len); - - if (s2) - memcpy(s2, s1, len); - return s2; -} -#endif - #if defined(__cplusplus) } #endif diff --git a/compat/strdup.c b/compat/strdup.c new file mode 100644 index 0000000..f3fb978 --- /dev/null +++ b/compat/strdup.c @@ -0,0 +1,11 @@ +#include "../git-compat-util.h" + +char *gitstrdup(const char *s1) +{ + size_t len = strlen(s1) + 1; + char *s2 = malloc(len); + + if (s2) + memcpy(s2, s1, len); + return s2; +} diff --git a/git-compat-util.h b/git-compat-util.h index db89ba7..93f6f23 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -663,6 +663,14 @@ void *gitmemmem(const void *haystack, size_t haystacklen, const void *needle, size_t needlelen); #endif +#ifdef OVERRIDE_STRDUP +#ifdef strdup +#undef strdup +#endif +#define strdup gitstrdup +char *gitstrdup(const char *s); +#endif + #ifdef NO_GETPAGESIZE #define getpagesize() sysconf(_SC_PAGESIZE) #endif -- 2.10.0 ``` ## Johannes Schindelin, 2016-09-04 07:46 Subject: Re: [PATCH] compat: move strdup(3) replacement to its own file Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1609040941210.129229%40virtualbox In-Reply-To: <89725a44-8afa-1eb1-732a-23b1e264616c@web.de> ``` Hi René, I imagine you Cc:ed me because the nedmalloc stuff came in via the Windows port, contributed by Marius (who is no longer active on the Git project because it works well enough for him)? On Sat, 3 Sep 2016, René Scharfe wrote: > Move our implementation of strdup(3) out of compat/nedmalloc/ and allow > it to be used independently from USE_NED_ALLOCATOR. This reduces the > difference of our copy of nedmalloc from the original, making it easier > to update, and allows for easier testing and reusing of our version of > strdup(). I would like to suggest an additional paragraph to explain why we do not need to #include "git-compat-util.h" in nedmalloc from now on: Please note that nedmalloc never actually uses strdup() itself, therefore we need not enforce gitstrdup() usage in nedmalloc.c. The patch looks quite straight-forward otherwise. (Junio, if you want an ACK from me, you hereby got it). Thanks! Dscho ``` ## René Scharfe, 2016-09-06 15:40 Subject: Re: [PATCH] compat: move strdup(3) replacement to its own file Message-ID: <6926b39c-4448-c463-33f7-d9eae841c635@web.de> URL: https://gitlist.dev/e/6926b39c-4448-c463-33f7-d9eae841c635%40web.de In-Reply-To: ``` Am 04.09.2016 um 09:46 schrieb Johannes Schindelin: > Hi René, > > I imagine you Cc:ed me because the nedmalloc stuff came in via the Windows > port, contributed by Marius (who is no longer active on the Git project > because it works well enough for him)? Kind of; it's also a follow-up to the recent discussion you started about compiler warnings in that code. > On Sat, 3 Sep 2016, René Scharfe wrote: > >> Move our implementation of strdup(3) out of compat/nedmalloc/ and allow >> it to be used independently from USE_NED_ALLOCATOR. This reduces the >> difference of our copy of nedmalloc from the original, making it easier >> to update, and allows for easier testing and reusing of our version of >> strdup(). > > I would like to suggest an additional paragraph to explain why we do not > need to #include "git-compat-util.h" in nedmalloc from now on: > > Please note that nedmalloc never actually uses strdup() itself, > therefore we need not enforce gitstrdup() usage in nedmalloc.c. Well, OK. I think the missing point is that the original nedmalloc doesn't come with strdup() and doesn't need it. Only _users_ of nedmalloc need it. Marius added it in nedmalloc.c, but strdup.c is a better place for it. René ``` ## Junio C Hamano, 2016-09-07 17:44 Subject: Re: [PATCH] compat: move strdup(3) replacement to its own file Message-ID: URL: https://gitlist.dev/e/xmqqfupbob9c.fsf%40gitster.mtv.corp.google.com In-Reply-To: <6926b39c-4448-c463-33f7-d9eae841c635@web.de> ``` René Scharfe writes: > Well, OK. I think the missing point is that the original nedmalloc > doesn't come with strdup() and doesn't need it. Only _users_ of > nedmalloc need it. Marius added it in nedmalloc.c, but strdup.c is a > better place for it. Thanks. I'll add these lines like so: Move our implementation of strdup(3) out of compat/nedmalloc/ and allow it to be used independently from USE_NED_ALLOCATOR. The original nedmalloc doesn't come with strdup() and doesn't need it. Only _users_ of nedmalloc need it, which was added when we imported it to our compat/ hierarchy. This reduces the difference of our copy of nedmalloc from the original, making it easier to update, and allows for easier testing and reusing of our version of strdup(). ``` ## Johannes Schindelin, 2016-09-08 06:59 Subject: Re: [PATCH] compat: move strdup(3) replacement to its own file Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1609080859160.129229%40virtualbox In-Reply-To: ``` Hi, On Wed, 7 Sep 2016, Junio C Hamano wrote: > René Scharfe writes: > > > Well, OK. I think the missing point is that the original nedmalloc > > doesn't come with strdup() and doesn't need it. Only _users_ of > > nedmalloc need it. Marius added it in nedmalloc.c, but strdup.c is a > > better place for it. > > Thanks. I'll add these lines like so: > > Move our implementation of strdup(3) out of compat/nedmalloc/ and > allow it to be used independently from USE_NED_ALLOCATOR. The > original nedmalloc doesn't come with strdup() and doesn't need it. > Only _users_ of nedmalloc need it, which was added when we imported > it to our compat/ hierarchy. > > This reduces the difference of our copy of nedmalloc from the > original, making it easier to update, and allows for easier testing > and reusing of our version of strdup(). Excellent! Thanks, Dscho ```