While coding my latest patch I found an annoying const warning when freeing strings we allocate with talloc but then assign to a variable of type "const char *".
This made me investigate our talloc_zfree macro and I found a weird patch, made back in the times that really doesn't make much sense. I reverted that and then applied a fix to avoid const warnings, as they are bogus in this case.
I also removed a duplicated macro that was out of sync already.
Simo Sorce (3): Revert "Avoid accessing half-deallocated memory when using talloc_zfree macro." Avoid duplicating macros Avoid const warnings when deallocating memory
src/ldb_modules/memberof.c | 4 ---- src/util/util.h | 6 +----- 2 files changed, 1 insertions(+), 9 deletions(-)
This reverts commit ff57c6aeb80a52b1f52bd1dac9308a69dc7a4774.
This commit doesn't really make sense, we are never accessing freed memory as all we are dealing with is a pointer which is never itsef part of the memory we are freeing (if it were, it would be an error in the caller and we shouldn't mask it in this macro). --- src/util/util.h | 6 +----- 1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/src/util/util.h b/src/util/util.h index b6ecfc2c952bd6a37f49194d961b3f762c9546e7..66bae505df2779dbfc065a09162fd789fef9e835 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -228,11 +228,7 @@ errno_t set_debug_file_from_fd(const int fd); #define FLAGS_PID_FILE 0x0004
#ifndef talloc_zfree -#define talloc_zfree(ptr) do { \ - TALLOC_CTX *_tmp_ctx = ptr; \ - ptr = NULL; \ - talloc_free(_tmp_ctx); \ - } while(0) +#define talloc_zfree(ptr) do { talloc_free(ptr); ptr = NULL; } while(0) #endif
#ifndef discard_const_p
This macro is already available in util/util.h which is expicitly included in this file. --- src/ldb_modules/memberof.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c index f0b5b72ed11ce71aec8ccb6e7a7cbd2d02e9566e..d0fe7349238ee91ba48f2ccd78516f43ab31a8e4 100644 --- a/src/ldb_modules/memberof.c +++ b/src/ldb_modules/memberof.c @@ -35,10 +35,6 @@ #define DB_USER_CLASS "user" #define DB_OC "objectClass"
-#ifndef talloc_zfree -#define talloc_zfree(ptr) do { talloc_free(ptr); ptr = NULL; } while(0) -#endif - #ifndef MAX #define MAX(a,b) (((a) > (b)) ? (a) : (b)) #endif
In some case we allocate and assign data to a const pointer. When we then try to free it we would get a const warning because talloc_free accepts a void, not a const void pointer. Use discard_const to avoid the warning, it is safe in this case. --- src/util/util.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/util.h b/src/util/util.h index 66bae505df2779dbfc065a09162fd789fef9e835..e85facb92bb4de8d9eeaf97716e1aac252f9b32c 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -228,7 +228,7 @@ errno_t set_debug_file_from_fd(const int fd); #define FLAGS_PID_FILE 0x0004
#ifndef talloc_zfree -#define talloc_zfree(ptr) do { talloc_free(ptr); ptr = NULL; } while(0) +#define talloc_zfree(ptr) do { talloc_free(discard_const(ptr)); ptr = NULL; } while(0) #endif
#ifndef discard_const_p
On Wed, Nov 14, 2012 at 09:01:15AM -0500, Simo Sorce wrote:
In some case we allocate and assign data to a const pointer. When we then try to free it we would get a const warning because talloc_free accepts a void, not a const void pointer. Use discard_const to avoid the warning, it is safe in this case.
src/util/util.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/util.h b/src/util/util.h index 66bae505df2779dbfc065a09162fd789fef9e835..e85facb92bb4de8d9eeaf97716e1aac252f9b32c 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -228,7 +228,7 @@ errno_t set_debug_file_from_fd(const int fd); #define FLAGS_PID_FILE 0x0004
#ifndef talloc_zfree -#define talloc_zfree(ptr) do { talloc_free(ptr); ptr = NULL; } while(0) +#define talloc_zfree(ptr) do { talloc_free(discard_const(ptr)); ptr = NULL; } while(0)
I wonder if it would be cleaner to add a talloc_zfree_const() or similar which can be used in places where it is safe to suppress the warning. At other place the warning might be useful to discover e.g. some copy-and-paste errors.
bye, Sumit
#endif
#ifndef discard_const_p
1.7.1
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, 2012-11-14 at 17:16 +0100, Sumit Bose wrote:
On Wed, Nov 14, 2012 at 09:01:15AM -0500, Simo Sorce wrote:
In some case we allocate and assign data to a const pointer. When we then try to free it we would get a const warning because talloc_free accepts a void, not a const void pointer. Use discard_const to avoid the warning, it is safe in this case.
src/util/util.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/util.h b/src/util/util.h index 66bae505df2779dbfc065a09162fd789fef9e835..e85facb92bb4de8d9eeaf97716e1aac252f9b32c 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -228,7 +228,7 @@ errno_t set_debug_file_from_fd(const int fd); #define FLAGS_PID_FILE 0x0004
#ifndef talloc_zfree -#define talloc_zfree(ptr) do { talloc_free(ptr); ptr = NULL; } while(0) +#define talloc_zfree(ptr) do { talloc_free(discard_const(ptr)); ptr = NULL; } while(0)
I wonder if it would be cleaner to add a talloc_zfree_const() or similar which can be used in places where it is safe to suppress the warning. At other place the warning might be useful to discover e.g. some copy-and-paste errors.
No, I thought about it but I do not think it is worth the complexity. If you free a truly const (ie not allocated) piece of memory you'll get a talloc error/abort which is sufficient I think :)
Simo.
On 11/14/2012 03:01 PM, Simo Sorce wrote:
In some case we allocate and assign data to a const pointer. When we then try to free it we would get a const warning because talloc_free accepts a void, not a const void pointer. Use discard_const to avoid the warning, it is safe in this case.
src/util/util.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/util.h b/src/util/util.h index 66bae505df2779dbfc065a09162fd789fef9e835..e85facb92bb4de8d9eeaf97716e1aac252f9b32c 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -228,7 +228,7 @@ errno_t set_debug_file_from_fd(const int fd); #define FLAGS_PID_FILE 0x0004
#ifndef talloc_zfree -#define talloc_zfree(ptr) do { talloc_free(ptr); ptr = NULL; } while(0) +#define talloc_zfree(ptr) do { talloc_free(discard_const(ptr)); ptr = NULL; } while(0) #endif
#ifndef discard_const_p
Hi, I don't see any warnings without this patch on F17 with libtalloc-devel-2.0.7-4.fc17.x86_64. Does this patch apply to specific version?
On Mon, 2012-11-26 at 16:51 +0100, Pavel Březina wrote:
On 11/14/2012 03:01 PM, Simo Sorce wrote:
In some case we allocate and assign data to a const pointer. When we then try to free it we would get a const warning because talloc_free accepts a void, not a const void pointer. Use discard_const to avoid the warning, it is safe in this case.
src/util/util.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/util.h b/src/util/util.h index 66bae505df2779dbfc065a09162fd789fef9e835..e85facb92bb4de8d9eeaf97716e1aac252f9b32c 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -228,7 +228,7 @@ errno_t set_debug_file_from_fd(const int fd); #define FLAGS_PID_FILE 0x0004
#ifndef talloc_zfree -#define talloc_zfree(ptr) do { talloc_free(ptr); ptr = NULL; } while(0) +#define talloc_zfree(ptr) do { talloc_free(discard_const(ptr)); ptr = NULL; } while(0) #endif
#ifndef discard_const_p
Hi, I don't see any warnings without this patch on F17 with libtalloc-devel-2.0.7-4.fc17.x86_64. Does this patch apply to specific version?
I t was needed at some point in one of the patches I recently sent to the list, however I have since changed the code (after Sumit's request) so it is not strictly needed. However I think it make sense to avoid errors here because freeing an allocated const char * string is legit.
I see however the argument that maybe we normally shouldn't declare allocated strings as const, but there may be cases where this is actually useful.
Do you see any downside in discarding const in talloc_zfree() ?
Simo.
On 11/26/2012 05:04 PM, Simo Sorce wrote:
On Mon, 2012-11-26 at 16:51 +0100, Pavel Březina wrote:
On 11/14/2012 03:01 PM, Simo Sorce wrote:
In some case we allocate and assign data to a const pointer. When we then try to free it we would get a const warning because talloc_free accepts a void, not a const void pointer. Use discard_const to avoid the warning, it is safe in this case.
src/util/util.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/util.h b/src/util/util.h index 66bae505df2779dbfc065a09162fd789fef9e835..e85facb92bb4de8d9eeaf97716e1aac252f9b32c 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -228,7 +228,7 @@ errno_t set_debug_file_from_fd(const int fd); #define FLAGS_PID_FILE 0x0004
#ifndef talloc_zfree -#define talloc_zfree(ptr) do { talloc_free(ptr); ptr = NULL; } while(0) +#define talloc_zfree(ptr) do { talloc_free(discard_const(ptr)); ptr = NULL; } while(0) #endif
#ifndef discard_const_p
Hi, I don't see any warnings without this patch on F17 with libtalloc-devel-2.0.7-4.fc17.x86_64. Does this patch apply to specific version?
I t was needed at some point in one of the patches I recently sent to the list, however I have since changed the code (after Sumit's request) so it is not strictly needed. However I think it make sense to avoid errors here because freeing an allocated const char * string is legit.
I see however the argument that maybe we normally shouldn't declare allocated strings as const, but there may be cases where this is actually useful.
Do you see any downside in discarding const in talloc_zfree() ?
As you wrote earlier to Sumit questions, if you try to talloc_free non-heap memory, talloc will tell you. However, if this patch is not needed right now, I prefer compiler warning.
In general, I think freeing const variable means that something is wrong with design, although I found myself in this situation once or twice (in other projects).
But I'm giving you at least ack to the code. :-)
On Mon, 2012-11-26 at 17:52 +0100, Pavel Březina wrote:
On 11/26/2012 05:04 PM, Simo Sorce wrote:
On Mon, 2012-11-26 at 16:51 +0100, Pavel Březina wrote:
On 11/14/2012 03:01 PM, Simo Sorce wrote:
In some case we allocate and assign data to a const pointer. When we then try to free it we would get a const warning because talloc_free accepts a void, not a const void pointer. Use discard_const to avoid the warning, it is safe in this case.
src/util/util.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/util.h b/src/util/util.h index 66bae505df2779dbfc065a09162fd789fef9e835..e85facb92bb4de8d9eeaf97716e1aac252f9b32c 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -228,7 +228,7 @@ errno_t set_debug_file_from_fd(const int fd); #define FLAGS_PID_FILE 0x0004
#ifndef talloc_zfree -#define talloc_zfree(ptr) do { talloc_free(ptr); ptr = NULL; } while(0) +#define talloc_zfree(ptr) do { talloc_free(discard_const(ptr)); ptr = NULL; } while(0) #endif
#ifndef discard_const_p
Hi, I don't see any warnings without this patch on F17 with libtalloc-devel-2.0.7-4.fc17.x86_64. Does this patch apply to specific version?
I t was needed at some point in one of the patches I recently sent to the list, however I have since changed the code (after Sumit's request) so it is not strictly needed. However I think it make sense to avoid errors here because freeing an allocated const char * string is legit.
I see however the argument that maybe we normally shouldn't declare allocated strings as const, but there may be cases where this is actually useful.
Do you see any downside in discarding const in talloc_zfree() ?
As you wrote earlier to Sumit questions, if you try to talloc_free non-heap memory, talloc will tell you. However, if this patch is not needed right now, I prefer compiler warning.
In general, I think freeing const variable means that something is wrong with design, although I found myself in this situation once or twice (in other projects).
But I'm giving you at least ack to the code. :-)
Ok then I will wait for someone else to chime in whether we want this patch in or not. I do not have a strong preference either way.
Simo.
On Mon, Nov 26, 2012 at 05:52:53PM +0100, Pavel Březina wrote:
In general, I think freeing const variable means that something is wrong with design, although I found myself in this situation once or twice (in other projects).
I don't think this is necessarily true; we often use a construct such as:
const char *str;
str = talloc_asprintf("%s%s\n", foo, bar);
/* do stuff while treating str as const */
talloc_free(str);
The free above would produce a warning with glibc's free, but I think if talloc discarded const here, we might get code that even treats str as const, but is able to free str.
To me, the patch makes sense.
On 11/14/2012 03:01 PM, Simo Sorce wrote:
This macro is already available in util/util.h which is expicitly included in this file.
src/ldb_modules/memberof.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c index f0b5b72ed11ce71aec8ccb6e7a7cbd2d02e9566e..d0fe7349238ee91ba48f2ccd78516f43ab31a8e4 100644 --- a/src/ldb_modules/memberof.c +++ b/src/ldb_modules/memberof.c @@ -35,10 +35,6 @@ #define DB_USER_CLASS "user" #define DB_OC "objectClass"
-#ifndef talloc_zfree -#define talloc_zfree(ptr) do { talloc_free(ptr); ptr = NULL; } while(0) -#endif
- #ifndef MAX #define MAX(a,b) (((a) > (b)) ? (a) : (b)) #endif
Ack.
On 11/14/2012 03:01 PM, Simo Sorce wrote:
This reverts commit ff57c6aeb80a52b1f52bd1dac9308a69dc7a4774.
This commit doesn't really make sense, we are never accessing freed memory as all we are dealing with is a pointer which is never itsef part of the memory we are freeing (if it were, it would be an error in the caller and we shouldn't mask it in this macro).
src/util/util.h | 6 +----- 1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/src/util/util.h b/src/util/util.h index b6ecfc2c952bd6a37f49194d961b3f762c9546e7..66bae505df2779dbfc065a09162fd789fef9e835 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -228,11 +228,7 @@ errno_t set_debug_file_from_fd(const int fd); #define FLAGS_PID_FILE 0x0004
#ifndef talloc_zfree -#define talloc_zfree(ptr) do { \
TALLOC_CTX *_tmp_ctx = ptr; \ptr = NULL; \talloc_free(_tmp_ctx); \- } while(0)
+#define talloc_zfree(ptr) do { talloc_free(ptr); ptr = NULL; } while(0) #endif
#ifndef discard_const_p
Hi, we can use TALLOC_FREE(ctx) here, which comes from talloc.h and is identical to this macro.
On Wed, Nov 14, 2012 at 05:01:34PM +0100, Pavel Březina wrote:
On 11/14/2012 03:01 PM, Simo Sorce wrote:
This reverts commit ff57c6aeb80a52b1f52bd1dac9308a69dc7a4774.
This commit doesn't really make sense, we are never accessing freed memory as all we are dealing with is a pointer which is never itsef part of the memory we are freeing (if it were, it would be an error in the caller and we shouldn't mask it in this macro).
src/util/util.h | 6 +----- 1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/src/util/util.h b/src/util/util.h index b6ecfc2c952bd6a37f49194d961b3f762c9546e7..66bae505df2779dbfc065a09162fd789fef9e835 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -228,11 +228,7 @@ errno_t set_debug_file_from_fd(const int fd); #define FLAGS_PID_FILE 0x0004
#ifndef talloc_zfree -#define talloc_zfree(ptr) do { \
TALLOC_CTX *_tmp_ctx = ptr; \ptr = NULL; \talloc_free(_tmp_ctx); \- } while(0)
+#define talloc_zfree(ptr) do { talloc_free(ptr); ptr = NULL; } while(0) #endif
#ifndef discard_const_p
Hi, we can use TALLOC_FREE(ctx) here, which comes from talloc.h and is identical to this macro.
Is it present even in very old talloc versions? We still support RHEL5..
On Wed, 2012-11-14 at 17:06 +0100, Jakub Hrozek wrote:
On Wed, Nov 14, 2012 at 05:01:34PM +0100, Pavel Březina wrote:
On 11/14/2012 03:01 PM, Simo Sorce wrote:
This reverts commit ff57c6aeb80a52b1f52bd1dac9308a69dc7a4774.
This commit doesn't really make sense, we are never accessing freed memory as all we are dealing with is a pointer which is never itsef part of the memory we are freeing (if it were, it would be an error in the caller and we shouldn't mask it in this macro).
src/util/util.h | 6 +----- 1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/src/util/util.h b/src/util/util.h index b6ecfc2c952bd6a37f49194d961b3f762c9546e7..66bae505df2779dbfc065a09162fd789fef9e835 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -228,11 +228,7 @@ errno_t set_debug_file_from_fd(const int fd); #define FLAGS_PID_FILE 0x0004
#ifndef talloc_zfree -#define talloc_zfree(ptr) do { \
TALLOC_CTX *_tmp_ctx = ptr; \ptr = NULL; \talloc_free(_tmp_ctx); \- } while(0)
+#define talloc_zfree(ptr) do { talloc_free(ptr); ptr = NULL; } while(0) #endif
#ifndef discard_const_p
Hi, we can use TALLOC_FREE(ctx) here, which comes from talloc.h and is identical to this macro.
Is it present even in very old talloc versions? We still support RHEL5..
It probably is now, but it is very ugly and we standardized our code to use talloc_zfree(), I do not think it makes sense to change it.
plus TALLOC_FREE() does not discard const. Which is what I add in the folloowing pathc, so we'd be back with our own macro.
Simo.
On 11/14/2012 11:06 AM, Jakub Hrozek wrote:
On Wed, Nov 14, 2012 at 05:01:34PM +0100, Pavel Březina wrote:
On 11/14/2012 03:01 PM, Simo Sorce wrote:
This reverts commit ff57c6aeb80a52b1f52bd1dac9308a69dc7a4774.
This commit doesn't really make sense, we are never accessing freed memory as all we are dealing with is a pointer which is never itsef part of the memory we are freeing (if it were, it would be an error in the caller and we shouldn't mask it in this macro).
src/util/util.h | 6 +----- 1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/src/util/util.h b/src/util/util.h index b6ecfc2c952bd6a37f49194d961b3f762c9546e7..66bae505df2779dbfc065a09162fd789fef9e835 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -228,11 +228,7 @@ errno_t set_debug_file_from_fd(const int fd); #define FLAGS_PID_FILE 0x0004
#ifndef talloc_zfree -#define talloc_zfree(ptr) do { \
TALLOC_CTX *_tmp_ctx = ptr; \ptr = NULL; \talloc_free(_tmp_ctx); \- } while(0)
+#define talloc_zfree(ptr) do { talloc_free(ptr); ptr = NULL; } while(0) #endif
#ifndef discard_const_p
Hi, we can use TALLOC_FREE(ctx) here, which comes from talloc.h and is identical to this macro.
Is it present even in very old talloc versions? We still support RHEL5.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Why this is a concern? We do not plan to support any of the latest versions on RHEL5. It is less and less needed as we move forward. I think it would be safe to say that 1.9 was the last version we build for RHEL5.
On Wed 14 Nov 2012 01:35:21 PM EST, Dmitri Pal wrote:
On 11/14/2012 11:06 AM, Jakub Hrozek wrote:
On Wed, Nov 14, 2012 at 05:01:34PM +0100, Pavel Březina wrote:
On 11/14/2012 03:01 PM, Simo Sorce wrote:
This reverts commit ff57c6aeb80a52b1f52bd1dac9308a69dc7a4774.
This commit doesn't really make sense, we are never accessing freed memory as all we are dealing with is a pointer which is never itsef part of the memory we are freeing (if it were, it would be an error in the caller and we shouldn't mask it in this macro).
src/util/util.h | 6 +----- 1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/src/util/util.h b/src/util/util.h index b6ecfc2c952bd6a37f49194d961b3f762c9546e7..66bae505df2779dbfc065a09162fd789fef9e835 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -228,11 +228,7 @@ errno_t set_debug_file_from_fd(const int fd); #define FLAGS_PID_FILE 0x0004
#ifndef talloc_zfree -#define talloc_zfree(ptr) do { \
TALLOC_CTX *_tmp_ctx = ptr; \ptr = NULL; \talloc_free(_tmp_ctx); \- } while(0)
+#define talloc_zfree(ptr) do { talloc_free(ptr); ptr = NULL; } while(0) #endif
#ifndef discard_const_p
Hi, we can use TALLOC_FREE(ctx) here, which comes from talloc.h and is identical to this macro.
Is it present even in very old talloc versions? We still support RHEL5.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Why this is a concern? We do not plan to support any of the latest versions on RHEL5. It is less and less needed as we move forward. I think it would be safe to say that 1.9 was the last version we build for RHEL5.
It's also irrelevant, since RHEL carries libtalloc 2.0.x, which is recent enough.
On 11/14/2012 03:01 PM, Simo Sorce wrote:
This reverts commit ff57c6aeb80a52b1f52bd1dac9308a69dc7a4774.
This commit doesn't really make sense, we are never accessing freed memory as all we are dealing with is a pointer which is never itsef part of the memory we are freeing (if it were, it would be an error in the caller and we shouldn't mask it in this macro).
src/util/util.h | 6 +----- 1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/src/util/util.h b/src/util/util.h index b6ecfc2c952bd6a37f49194d961b3f762c9546e7..66bae505df2779dbfc065a09162fd789fef9e835 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -228,11 +228,7 @@ errno_t set_debug_file_from_fd(const int fd); #define FLAGS_PID_FILE 0x0004
#ifndef talloc_zfree -#define talloc_zfree(ptr) do { \
TALLOC_CTX *_tmp_ctx = ptr; \ptr = NULL; \talloc_free(_tmp_ctx); \- } while(0)
+#define talloc_zfree(ptr) do { talloc_free(ptr); ptr = NULL; } while(0) #endif
#ifndef discard_const_p
Ack.
sssd-devel@lists.fedorahosted.org
-
Dmitri Pal -
Jakub Hrozek -
Pavel Březina -
Simo Sorce -
Stephen Gallagher -
Sumit Bose