[PATCH 1/2] Annotate internal_error with the attribute noreturn

[PATCH 1/2] Annotate internal_error with the attribute noreturn

[Justus Winter]

Annotating functions that do not return with the noreturn attribute
(which is understood by both gcc and clang) prevents static analyzers
from generating false positives (internal_error is used to terminate
the process and is used extensively in error handling code paths).

Remove the return statement that was placed there to appease the
compiler. Functions annotated with noreturn are not supposed to return
any values.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 util/error_util.c |    2 --
 util/error_util.h |    2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/util/error_util.c b/util/error_util.c
index 630d228..3cf353a 100644
--- a/util/error_util.c
+++ b/util/error_util.c
@@ -35,7 +35,5 @@ _internal_error (const char *format, ...)
     vfprintf (stderr, format, va_args);
 
     exit (1);
-
-    return 1;
 }
 
diff --git a/util/error_util.h b/util/error_util.h
index bb15822..24a644b 100644
--- a/util/error_util.h
+++ b/util/error_util.h
@@ -30,7 +30,7 @@
  * Note that PRINTF_ATTRIBUTE comes from talloc.h
  */
 int
-_internal_error (const char *format, ...) PRINTF_ATTRIBUTE (1, 2);
+_internal_error (const char *format, ...) PRINTF_ATTRIBUTE (1, 2) __attribute__ ((noreturn));
 
 /* There's no point in continuing when we've detected that we've done
  * something wrong internally (as opposed to the user passing in a
-- 
1.7.10.4

[PATCH 2/2] Avoid potentially dereferencing a NULL pointer

[Justus Winter]

GMIME_IS_MULTIPART and GMIME_IS_MESSAGE both handle NULL pointers
gracefully, but the G_OBJECT_TYPE used in the error handling block
dereferences it without checking it first.

Fix this by checking whether parent->part is valid.

Found using the clang static analyzer.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 mime-node.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mime-node.c b/mime-node.c
index 97e8b48..839737a 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -291,7 +291,7 @@ mime_node_child (mime_node_t *parent, int child)
     GMimeObject *sub;
     mime_node_t *node;
 
-    if (!parent || child < 0 || child >= parent->nchildren)
+    if (!parent || !parent->part || child < 0 || child >= parent->nchildren)
 	return NULL;
 
     if (GMIME_IS_MULTIPART (parent->part)) {
-- 
1.7.10.4

Re: [PATCH 2/2] Avoid potentially dereferencing a NULL pointer

[Austin Clements]

Quoth Justus Winter on Sep 21 at  2:50 pm:
> GMIME_IS_MULTIPART and GMIME_IS_MESSAGE both handle NULL pointers
> gracefully, but the G_OBJECT_TYPE used in the error handling block
> dereferences it without checking it first.
> 
> Fix this by checking whether parent->part is valid.
> 
> Found using the clang static analyzer.

Neat.

Can this actually happen, though?  If so, I think this point is too
late to be checking for a NULL part field.  It should probably be
checked when the mime_node_t is created so that mime_node_t never has
a NULL part field.

> Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
> ---
>  mime-node.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mime-node.c b/mime-node.c
> index 97e8b48..839737a 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -291,7 +291,7 @@ mime_node_child (mime_node_t *parent, int child)
>      GMimeObject *sub;
>      mime_node_t *node;
>  
> -    if (!parent || child < 0 || child >= parent->nchildren)
> +    if (!parent || !parent->part || child < 0 || child >= parent->nchildren)
>  	return NULL;
>  
>      if (GMIME_IS_MULTIPART (parent->part)) {

Re: [PATCH 2/2] Avoid potentially dereferencing a NULL pointer

[Justus Winter]

Quoting Austin Clements (2012-09-22 18:19:08)
> Quoth Justus Winter on Sep 21 at  2:50 pm:
> > GMIME_IS_MULTIPART and GMIME_IS_MESSAGE both handle NULL pointers
> > gracefully, but the G_OBJECT_TYPE used in the error handling block
> > dereferences it without checking it first.
> > 
> > Fix this by checking whether parent->part is valid.
> > 
> > Found using the clang static analyzer.
> 
> Neat.

Yes. Besides this the code turns up no warnings (modulo one false
positive, clang doesn't understand that progress_notify is never
called if it's NULL in notmuch_database_upgrade b/c the signal handler
isn't set up then).

> Can this actually happen, though?  If so, I think this point is too
> late to be checking for a NULL part field.  It should probably be
> checked when the mime_node_t is created so that mime_node_t never has
> a NULL part field.

I'm not sure actually. Then again this patch isn't hacky at all and
being somewhat defensive isn't bad imho.

Justus

Re: [PATCH 1/2] Annotate internal_error with the attribute noreturn

[David Bremner]

Justus Winter <4winter@informatik.uni-hamburg.de> writes:

> Annotating functions that do not return with the noreturn attribute
> (which is understood by both gcc and clang) prevents static analyzers
> from generating false positives (internal_error is used to terminate
> the process and is used extensively in error handling code paths).
>
> Remove the return statement that was placed there to appease the
> compiler. Functions annotated with noreturn are not supposed to return
> any values.
>

This patch looks OK (although I don't know the pragma details).  Do you
(or anyone) happen to know why _internal_error is an int function?
Making it void wouldn't fix the problem with static analzers, but having
an int function without a return looks ugly to me.

d

Re: [PATCH 1/2] Annotate internal_error with the attribute noreturn

[Austin Clements]

LGTM, though I agree with David that this should return void now, if
that's possible.

Do we want to wrap the __attribute__((noreturn)) in an #ifdef __GNUC__
(or provide a PRINTF_ATTRIBUTE-like macro) or is that already a lost
cause?

Quoth Justus Winter on Sep 21 at  2:50 pm:
> Annotating functions that do not return with the noreturn attribute
> (which is understood by both gcc and clang) prevents static analyzers
> from generating false positives (internal_error is used to terminate
> the process and is used extensively in error handling code paths).
> 
> Remove the return statement that was placed there to appease the
> compiler. Functions annotated with noreturn are not supposed to return
> any values.
> 
> Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
> ---
>  util/error_util.c |    2 --
>  util/error_util.h |    2 +-
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/util/error_util.c b/util/error_util.c
> index 630d228..3cf353a 100644
> --- a/util/error_util.c
> +++ b/util/error_util.c
> @@ -35,7 +35,5 @@ _internal_error (const char *format, ...)
>      vfprintf (stderr, format, va_args);
>  
>      exit (1);
> -
> -    return 1;
>  }
>  
> diff --git a/util/error_util.h b/util/error_util.h
> index bb15822..24a644b 100644
> --- a/util/error_util.h
> +++ b/util/error_util.h
> @@ -30,7 +30,7 @@
>   * Note that PRINTF_ATTRIBUTE comes from talloc.h
>   */
>  int
> -_internal_error (const char *format, ...) PRINTF_ATTRIBUTE (1, 2);
> +_internal_error (const char *format, ...) PRINTF_ATTRIBUTE (1, 2) __attribute__ ((noreturn));
>  
>  /* There's no point in continuing when we've detected that we've done
>   * something wrong internally (as opposed to the user passing in a

Re: [PATCH 1/2] Annotate internal_error with the attribute noreturn

[Tomi Ollila]

On Sat, Sep 22 2012, Austin Clements wrote:

> LGTM, though I agree with David that this should return void now, if
> that's possible.
>
> Do we want to wrap the __attribute__((noreturn)) in an #ifdef __GNUC__
> (or provide a PRINTF_ATTRIBUTE-like macro) or is that already a lost
> cause?

Good point -- this should be NORETURN_ATTRIBUTE instead...

Also, http://gcc.gnu.org/onlinedocs/gcc-4.3.2//gcc/Function-Attributes.html states:

"It does not make sense for a noreturn function to have a return type other
than void."

(I think that hasn't changed since :)

Tomi

>
> Quoth Justus Winter on Sep 21 at  2:50 pm:
>> Annotating functions that do not return with the noreturn attribute
>> (which is understood by both gcc and clang) prevents static analyzers
>> from generating false positives (internal_error is used to terminate
>> the process and is used extensively in error handling code paths).
>> 
>> Remove the return statement that was placed there to appease the
>> compiler. Functions annotated with noreturn are not supposed to return
>> any values.
>> 
>> Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
>> ---
>>  util/error_util.c |    2 --
>>  util/error_util.h |    2 +-
>>  2 files changed, 1 insertion(+), 3 deletions(-)
>> 
>> diff --git a/util/error_util.c b/util/error_util.c
>> index 630d228..3cf353a 100644
>> --- a/util/error_util.c
>> +++ b/util/error_util.c
>> @@ -35,7 +35,5 @@ _internal_error (const char *format, ...)
>>      vfprintf (stderr, format, va_args);
>>  
>>      exit (1);
>> -
>> -    return 1;
>>  }
>>  
>> diff --git a/util/error_util.h b/util/error_util.h
>> index bb15822..24a644b 100644
>> --- a/util/error_util.h
>> +++ b/util/error_util.h
>> @@ -30,7 +30,7 @@
>>   * Note that PRINTF_ATTRIBUTE comes from talloc.h
>>   */
>>  int
>> -_internal_error (const char *format, ...) PRINTF_ATTRIBUTE (1, 2);
>> +_internal_error (const char *format, ...) PRINTF_ATTRIBUTE (1, 2) __attribute__ ((noreturn));
>>  
>>  /* There's no point in continuing when we've detected that we've done
>>   * something wrong internally (as opposed to the user passing in a
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

RFC: Annotate internal_error with the attribute noreturn 2nd patchset

[Justus Winter]

Thanks for your feedback. I've updated the patch series accordingly.

We need to discuss where those kind of macro definitions abstracting
away compiler differences should go. None of the files in util
includes notmuch-private.h, so I was unsure whether it's okay to put
them there.

Justus

[PATCH 1/5] RFC: Provide a __has_attribute compatibility macro

[Justus Winter]

__has_attribute is defined by clang and tests whether a given function
attribute is supported by clang.

Add a compatibility macro for other compilers.

Note: This is work in progress, please don't merge this patch. The
question that needs to be discussed is where this kind of macro should
be defined.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 util/error_util.h |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/util/error_util.h b/util/error_util.h
index bb15822..1b11047 100644
--- a/util/error_util.h
+++ b/util/error_util.h
@@ -23,6 +23,14 @@
 
 #include <talloc.h>
 
+/* clang provides this macro to test for support for function
+ * attributes. If it isn't defined, this provides a compatibility
+ * macro for other compilers.
+ */
+#ifndef __has_attribute
+#define __has_attribute(x) 0
+#endif
+
 /* There's no point in continuing when we've detected that we've done
  * something wrong internally (as opposed to the user passing in a
  * bogus value).
-- 
1.7.10.4

[PATCH 2/5] RFC: Provide a NORETURN_ATTRIBUTE macro similar to PRINTF_ATTRIBUTE

[Justus Winter]

This attribute is understood by gcc since version 2.5. clang provides
support for testing for function attributes using __has_attribute. For
other compilers this macro evaluates to the empty string.

Note: This is work in progress, please don't merge this patch. The
question that needs to be discussed is where this kind of macro should
be defined.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 util/error_util.h |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/util/error_util.h b/util/error_util.h
index 1b11047..27e119f 100644
--- a/util/error_util.h
+++ b/util/error_util.h
@@ -31,6 +31,22 @@
 #define __has_attribute(x) 0
 #endif
 
+/* Provide a NORETURN_ATTRIBUTE macro similar to PRINTF_ATTRIBUTE from
+ * talloc.
+ *
+ * This attribute is understood by gcc since version 2.5. clang
+ * provides support for testing for function attributes.
+ */
+#ifndef NORETURN_ATTRIBUTE
+#if (__GNUC__ >= 3 ||				\
+     (__GNUC__ == 2 && __GNUC_MINOR__ >= 5) ||	\
+     __has_attribute (noreturn))
+#define NORETURN_ATTRIBUTE __attribute__ ((noreturn))
+#else
+#define NORETURN_ATTRIBUTE
+#endif
+#endif
+
 /* There's no point in continuing when we've detected that we've done
  * something wrong internally (as opposed to the user passing in a
  * bogus value).
-- 
1.7.10.4

[PATCH 3/5] Fix the COERCE_STATUS macro

[Justus Winter]

Fix the COERCE_STATUS macro to handle _internal_error being declared
as void function.

Note that the function _internal_error does not return. Evaluating to
NOTMUCH_STATUS_SUCCESS is done purely to appease the compiler.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 lib/notmuch-private.h |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index bfb4111..7a409f5 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -136,13 +136,17 @@ typedef enum _notmuch_private_status {
  * to or greater than NOTMUCH_STATUS_LAST_STATUS. (The idea here is
  * that the caller has previously handled any expected
  * notmuch_private_status_t values.)
+ *
+ * Note that the function _internal_error does not return. Evaluating
+ * to NOTMUCH_STATUS_SUCCESS is done purely to appease the compiler.
  */
 #define COERCE_STATUS(private_status, format, ...)			\
     ((private_status >= (notmuch_private_status_t) NOTMUCH_STATUS_LAST_STATUS)\
      ?									\
-     (notmuch_status_t) _internal_error (format " (%s).\n",		\
-                                         ##__VA_ARGS__,			\
-                                         __location__)			\
+     _internal_error (format " (%s).\n",				\
+                      ##__VA_ARGS__,					\
+                      __location__),					\
+     (notmuch_status_t) NOTMUCH_PRIVATE_STATUS_SUCCESS			\
      :									\
      (notmuch_status_t) private_status)
 
-- 
1.7.10.4

[PATCH 4/5] Annotate internal_error with the attribute noreturn

[Justus Winter]

Annotating functions that do not return with the noreturn attribute
(which is understood by both gcc and clang) prevents static analyzers
from generating false positives (internal_error is used to terminate
the process and is used extensively in error handling code paths).

Remove the return statement that was placed there to appease the
compiler. Functions annotated with noreturn are not supposed to return
any values.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 util/error_util.c |    4 +---
 util/error_util.h |    4 ++--
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/util/error_util.c b/util/error_util.c
index 630d228..d6e60fc 100644
--- a/util/error_util.c
+++ b/util/error_util.c
@@ -24,7 +24,7 @@
 
 #include "error_util.h"
 
-int
+void
 _internal_error (const char *format, ...)
 {
     va_list va_args;
@@ -35,7 +35,5 @@ _internal_error (const char *format, ...)
     vfprintf (stderr, format, va_args);
 
     exit (1);
-
-    return 1;
 }
 
diff --git a/util/error_util.h b/util/error_util.h
index 27e119f..d4d4584 100644
--- a/util/error_util.h
+++ b/util/error_util.h
@@ -53,8 +53,8 @@
  *
  * Note that PRINTF_ATTRIBUTE comes from talloc.h
  */
-int
-_internal_error (const char *format, ...) PRINTF_ATTRIBUTE (1, 2);
+void
+_internal_error (const char *format, ...) PRINTF_ATTRIBUTE (1, 2) NORETURN_ATTRIBUTE;
 
 /* There's no point in continuing when we've detected that we've done
  * something wrong internally (as opposed to the user passing in a
-- 
1.7.10.4

[PATCH 5/5] Avoid potentially dereferencing a NULL pointer

[Justus Winter]

GMIME_IS_MULTIPART and GMIME_IS_MESSAGE both handle NULL pointers
gracefully, but the G_OBJECT_TYPE used in the error handling block
dereferences it without checking it first.

Fix this by checking whether parent->part is valid.

Found using the clang static analyzer.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 mime-node.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mime-node.c b/mime-node.c
index 97e8b48..839737a 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -291,7 +291,7 @@ mime_node_child (mime_node_t *parent, int child)
     GMimeObject *sub;
     mime_node_t *node;
 
-    if (!parent || child < 0 || child >= parent->nchildren)
+    if (!parent || !parent->part || child < 0 || child >= parent->nchildren)
 	return NULL;
 
     if (GMIME_IS_MULTIPART (parent->part)) {
-- 
1.7.10.4

Re: RFC: Annotate internal_error with the attribute noreturn 2nd patchset

[David Bremner]

Justus Winter <4winter@informatik.uni-hamburg.de> writes:

> We need to discuss where those kind of macro definitions abstracting
> away compiler differences should go. None of the files in util
> includes notmuch-private.h, so I was unsure whether it's okay to put
> them there.

How about a new header file in util/

function-attributes.h or something like that?

The util stuff is "lower level" than lib/ so it makes sense to me to put
common stuff there.

d

Re: RFC: Annotate internal_error with the attribute noreturn 2nd patchset

[Justus Winter]

Quoting David Bremner (2012-09-24 12:44:04)
> Justus Winter <4winter@informatik.uni-hamburg.de> writes:
> 
> > We need to discuss where those kind of macro definitions abstracting
> > away compiler differences should go. None of the files in util
> > includes notmuch-private.h, so I was unsure whether it's okay to put
> > them there.
> 
> How about a new header file in util/
> 
> function-attributes.h or something like that?
> 
> The util stuff is "lower level" than lib/ so it makes sense to me to put
> common stuff there.

Maybe we should add this file to compat?

Justus

Re: RFC: Annotate internal_error with the attribute noreturn 2nd patchset

[David Bremner]

Justus Winter <4winter@informatik.uni-hamburg.de> writes:

>> The util stuff is "lower level" than lib/ so it makes sense to me to put
>> common stuff there.
>
> Maybe we should add this file to compat?

Sure, given appropriate re-wording of compat/README

d

[PATCH 1/6] Provide a __has_attribute compatibility macro

[Justus Winter]

__has_attribute is defined by clang and tests whether a given function
attribute is supported by clang.

Add a compatibility macro for other compilers.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 compat/function-attributes.h |   31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 compat/function-attributes.h

diff --git a/compat/function-attributes.h b/compat/function-attributes.h
new file mode 100644
index 0000000..18f9090
--- /dev/null
+++ b/compat/function-attributes.h
@@ -0,0 +1,31 @@
+/* function-attributes.h - Provides compiler abstractions for
+ *                         function attributes
+ *
+ * Copyright (c) 2012 Justus Winter <4winter@informatik.uni-hamburg.de>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ */
+
+#ifndef FUNCTION_ATTRIBUTES_H
+#define FUNCTION_ATTRIBUTES_H
+
+/* clang provides this macro to test for support for function
+ * attributes. If it isn't defined, this provides a compatibility
+ * macro for other compilers.
+ */
+#ifndef __has_attribute
+#define __has_attribute(x) 0
+#endif
+
+#endif
-- 
1.7.10.4

[PATCH 2/6] Provide a NORETURN_ATTRIBUTE macro similar to PRINTF_ATTRIBUTE

[Justus Winter]

This attribute is understood by gcc since version 2.5. clang provides
support for testing for function attributes using __has_attribute. For
other compilers this macro evaluates to the empty string.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 compat/function-attributes.h |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/compat/function-attributes.h b/compat/function-attributes.h
index 18f9090..8450a17 100644
--- a/compat/function-attributes.h
+++ b/compat/function-attributes.h
@@ -28,4 +28,20 @@
 #define __has_attribute(x) 0
 #endif
 
+/* Provide a NORETURN_ATTRIBUTE macro similar to PRINTF_ATTRIBUTE from
+ * talloc.
+ *
+ * This attribute is understood by gcc since version 2.5. clang
+ * provides support for testing for function attributes.
+ */
+#ifndef NORETURN_ATTRIBUTE
+#if (__GNUC__ >= 3 ||				\
+     (__GNUC__ == 2 && __GNUC_MINOR__ >= 5) ||	\
+     __has_attribute (noreturn))
+#define NORETURN_ATTRIBUTE __attribute__ ((noreturn))
+#else
+#define NORETURN_ATTRIBUTE
+#endif
+#endif
+
 #endif
-- 
1.7.10.4

[PATCH 3/6] Extend compat/README

[Justus Winter]

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 compat/README |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/compat/README b/compat/README
index 38e2e14..12aacf4 100644
--- a/compat/README
+++ b/compat/README
@@ -1,6 +1,6 @@
 notmuch/compat
 
-This directory consists of two things:
+This directory consists of three things:
 
 1. Small programs used by the notmuch configure script to test for the
    availability of certain system features, (library functions, etc.).
@@ -14,3 +14,8 @@ This directory consists of two things:
 
    The compilation of these files is made conditional on the output of
    the test programs from [1].
+
+3. Macro definitions abstracting compiler differences (e.g. function
+   attributes).
+
+   For example: function-attributes.h
-- 
1.7.10.4

[PATCH 4/6] Fix the COERCE_STATUS macro

[Justus Winter]

Fix the COERCE_STATUS macro to handle _internal_error being declared
as void function.

Note that the function _internal_error does not return. Evaluating to
NOTMUCH_STATUS_SUCCESS is done purely to appease the compiler.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 lib/notmuch-private.h |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index bfb4111..7a409f5 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -136,13 +136,17 @@ typedef enum _notmuch_private_status {
  * to or greater than NOTMUCH_STATUS_LAST_STATUS. (The idea here is
  * that the caller has previously handled any expected
  * notmuch_private_status_t values.)
+ *
+ * Note that the function _internal_error does not return. Evaluating
+ * to NOTMUCH_STATUS_SUCCESS is done purely to appease the compiler.
  */
 #define COERCE_STATUS(private_status, format, ...)			\
     ((private_status >= (notmuch_private_status_t) NOTMUCH_STATUS_LAST_STATUS)\
      ?									\
-     (notmuch_status_t) _internal_error (format " (%s).\n",		\
-                                         ##__VA_ARGS__,			\
-                                         __location__)			\
+     _internal_error (format " (%s).\n",				\
+                      ##__VA_ARGS__,					\
+                      __location__),					\
+     (notmuch_status_t) NOTMUCH_PRIVATE_STATUS_SUCCESS			\
      :									\
      (notmuch_status_t) private_status)
 
-- 
1.7.10.4

Re: [PATCH 4/6] Fix the COERCE_STATUS macro

[Austin Clements]

Quoth Justus Winter on Sep 24 at  5:21 pm:
> Fix the COERCE_STATUS macro to handle _internal_error being declared
> as void function.
> 
> Note that the function _internal_error does not return. Evaluating to
> NOTMUCH_STATUS_SUCCESS is done purely to appease the compiler.
> 
> Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
> ---
>  lib/notmuch-private.h |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
> index bfb4111..7a409f5 100644
> --- a/lib/notmuch-private.h
> +++ b/lib/notmuch-private.h
> @@ -136,13 +136,17 @@ typedef enum _notmuch_private_status {
>   * to or greater than NOTMUCH_STATUS_LAST_STATUS. (The idea here is
>   * that the caller has previously handled any expected
>   * notmuch_private_status_t values.)
> + *
> + * Note that the function _internal_error does not return. Evaluating
> + * to NOTMUCH_STATUS_SUCCESS is done purely to appease the compiler.
>   */
>  #define COERCE_STATUS(private_status, format, ...)			\
>      ((private_status >= (notmuch_private_status_t) NOTMUCH_STATUS_LAST_STATUS)\
>       ?									\
> -     (notmuch_status_t) _internal_error (format " (%s).\n",		\
> -                                         ##__VA_ARGS__,			\
> -                                         __location__)			\
> +     _internal_error (format " (%s).\n",				\
> +                      ##__VA_ARGS__,					\
> +                      __location__),					\
> +     (notmuch_status_t) NOTMUCH_PRIVATE_STATUS_SUCCESS			\

Just a nit: why not simply NOTMUCH_STATUS_SUCCESS?

Otherwise, this series LGTM.  No need to roll another version just for
this comment.

>       :									\
>       (notmuch_status_t) private_status)
>  

Re: [PATCH 4/6] Fix the COERCE_STATUS macro

[Tomi Ollila]

On Mon, Sep 24 2012, Austin Clements <amdragon@MIT.EDU> wrote:

> Quoth Justus Winter on Sep 24 at  5:21 pm:
>> Fix the COERCE_STATUS macro to handle _internal_error being declared
>> as void function.
>> 
>> Note that the function _internal_error does not return. Evaluating to
>> NOTMUCH_STATUS_SUCCESS is done purely to appease the compiler.
>> 
>> Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
>> ---
>>  lib/notmuch-private.h |   10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>> 
>> diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
>> index bfb4111..7a409f5 100644
>> --- a/lib/notmuch-private.h
>> +++ b/lib/notmuch-private.h
>> @@ -136,13 +136,17 @@ typedef enum _notmuch_private_status {
>>   * to or greater than NOTMUCH_STATUS_LAST_STATUS. (The idea here is
>>   * that the caller has previously handled any expected
>>   * notmuch_private_status_t values.)
>> + *
>> + * Note that the function _internal_error does not return. Evaluating
>> + * to NOTMUCH_STATUS_SUCCESS is done purely to appease the compiler.
>>   */
>>  #define COERCE_STATUS(private_status, format, ...)			\
>>      ((private_status >= (notmuch_private_status_t) NOTMUCH_STATUS_LAST_STATUS)\
>>       ?									\
>> -     (notmuch_status_t) _internal_error (format " (%s).\n",		\
>> -                                         ##__VA_ARGS__,			\
>> -                                         __location__)			\
>> +     _internal_error (format " (%s).\n",				\
>> +                      ##__VA_ARGS__,					\
>> +                      __location__),					\
>> +     (notmuch_status_t) NOTMUCH_PRIVATE_STATUS_SUCCESS			\
>
> Just a nit: why not simply NOTMUCH_STATUS_SUCCESS?
>
> Otherwise, this series LGTM.  No need to roll another version just for
> this comment.

I agree. LGTM.

>
>>       :									\
>>       (notmuch_status_t) private_status)

[PATCH 5/6] Annotate internal_error with the attribute noreturn

[Justus Winter]

Annotating functions that do not return with the noreturn attribute
(which is understood by both gcc and clang) prevents static analyzers
from generating false positives (internal_error is used to terminate
the process and is used extensively in error handling code paths).

Remove the return statement that was placed there to appease the
compiler. Functions annotated with noreturn are not supposed to return
any values.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 util/error_util.c |    4 +---
 util/error_util.h |    6 ++++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/util/error_util.c b/util/error_util.c
index 630d228..d6e60fc 100644
--- a/util/error_util.c
+++ b/util/error_util.c
@@ -24,7 +24,7 @@
 
 #include "error_util.h"
 
-int
+void
 _internal_error (const char *format, ...)
 {
     va_list va_args;
@@ -35,7 +35,5 @@ _internal_error (const char *format, ...)
     vfprintf (stderr, format, va_args);
 
     exit (1);
-
-    return 1;
 }
 
diff --git a/util/error_util.h b/util/error_util.h
index bb15822..17c8727 100644
--- a/util/error_util.h
+++ b/util/error_util.h
@@ -23,14 +23,16 @@
 
 #include <talloc.h>
 
+#include "function-attributes.h"
+
 /* There's no point in continuing when we've detected that we've done
  * something wrong internally (as opposed to the user passing in a
  * bogus value).
  *
  * Note that PRINTF_ATTRIBUTE comes from talloc.h
  */
-int
-_internal_error (const char *format, ...) PRINTF_ATTRIBUTE (1, 2);
+void
+_internal_error (const char *format, ...) PRINTF_ATTRIBUTE (1, 2) NORETURN_ATTRIBUTE;
 
 /* There's no point in continuing when we've detected that we've done
  * something wrong internally (as opposed to the user passing in a
-- 
1.7.10.4

[PATCH 6/6] Avoid potentially dereferencing a NULL pointer

[Justus Winter]

GMIME_IS_MULTIPART and GMIME_IS_MESSAGE both handle NULL pointers
gracefully, but the G_OBJECT_TYPE used in the error handling block
dereferences it without checking it first.

Fix this by checking whether parent->part is valid.

Found using the clang static analyzer.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 mime-node.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mime-node.c b/mime-node.c
index 97e8b48..839737a 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -291,7 +291,7 @@ mime_node_child (mime_node_t *parent, int child)
     GMimeObject *sub;
     mime_node_t *node;
 
-    if (!parent || child < 0 || child >= parent->nchildren)
+    if (!parent || !parent->part || child < 0 || child >= parent->nchildren)
 	return NULL;
 
     if (GMIME_IS_MULTIPART (parent->part)) {
-- 
1.7.10.4

Re: [PATCH 1/6] Provide a __has_attribute compatibility macro

[David Bremner]

Justus Winter <4winter@informatik.uni-hamburg.de> writes:

> __has_attribute is defined by clang and tests whether a given function
> attribute is supported by clang.

series pushed

d