On Wed, Dec 16, 2020 at 03:07:04AM +0100, Stefan Sperling wrote:
> During recent discussion of histedit changes an issue with error handling
> came up. If a function handles two errors at once, it is possible that
> both errors end up being stored in the same 'static struct got_error',
> in which case one error will be overwritten.
> 
> In the case under discussion, the user saw "object not found" while the
> expected error text was something like "log message cannot be empty".
> This could happen in all sorts of ways. The current error message system
> will yield unpredictable results depending on how it is used.
> 
> The patch below adds an array of pre-allocated errors which we can cycle
> through in order to prevent this problem, at least in single-threaded
> contexts. The problem will now only happen if more errors are in flight
> than fit into the array. And that's not going to be a problem in practice.
> 
> I realize that this is not a thread-safe solution due to lack of any atomic
> operations and locking. We do need a thread-safe solution for tog(1).
> 
> I don't want to wire pthreads into this part of the program. And I'd really
> like to avoid any dynamic memory allocation because I want this code to be
> able to operate under ENOMEM conditions. And not having to worry about
> errors leaking memory is nice.
> 
> Declaring the array index as an atomic_int from stdatomic.h would be an
> ideal solution from my point of view. I've tested this approach on OpenBSD
> and it works. The caveat is that this is a gcc/clang extension, and an
> optional feature of C11. For now, the index is a plain unsigned int.
> Are there any concerns about using atomic_int?

I have gone ahead and committed these changes. Feedback on how to resolve
the thread-safety issue would still be welcome. At the moment I am leaning
towards indexing the array with an atomic_int if the compiler supports it,
and ignoring the issue on older compilers (not worse than the status quo).

> Another thread-safety problem is that the code users strerror() but
> switching to strerror_r() on top of the changes below would be trivial.

I've also done this.

> diff 024c8833f087804d46239c8919147870ab57242b 9969f4a0944bd8fc2c9945d8d94e9f3d7d4cb13f
> blob - 772c50ed2ea44fc86f4cb411b60f6bbf55ef0a88
> blob + a41a721678814c6e3adef116e9e6445a894d022a
> --- lib/error.c
> +++ lib/error.c
> @@ -38,6 +38,18 @@
>  #define nitems(_a) (sizeof(_a) / sizeof((_a)[0]))
>  #endif
>  
> +static struct got_custom_error {
> +	struct got_error err;
> +	char msg[4080];
> +} custom_errors[16];
> +
> +static struct got_custom_error *
> +get_custom_err(void)
> +{
> +	static unsigned int idx;
> +	return &custom_errors[(idx++) % nitems(custom_errors)];
> +}
> +
>  const struct got_error *
>  got_error(int code)
>  {
> @@ -54,14 +66,16 @@ got_error(int code)
>  const struct got_error *
>  got_error_msg(int code, const char *msg)
>  {
> -	static struct got_error err;
> +	struct got_custom_error *cerr = get_custom_err();
> +	struct got_error *err = &cerr->err;
>  	size_t i;
>  
>  	for (i = 0; i < nitems(got_errors); i++) {
>  		if (code == got_errors[i].code) {
> -			err.code = code;
> -			err.msg = msg;
> -			return &err;
> +			err->code = code;
> +			strlcpy(cerr->msg, msg, sizeof(cerr->msg));
> +			err->msg = cerr->msg;
> +			return err;
>  		}
>  	}
>  
> @@ -71,51 +85,51 @@ got_error_msg(int code, const char *msg)
>  const struct got_error *
>  got_error_from_errno(const char *prefix)
>  {
> -	static struct got_error err;
> -	static char err_msg[PATH_MAX + 20];
> +	struct got_custom_error *cerr = get_custom_err();
> +	struct got_error *err = &cerr->err;
>  
> -	snprintf(err_msg, sizeof(err_msg), "%s: %s", prefix,
> +	snprintf(cerr->msg, sizeof(cerr->msg), "%s: %s", prefix,
>  	    strerror(errno));
>  
> -	err.code = GOT_ERR_ERRNO;
> -	err.msg = err_msg;
> -	return &err;
> +	err->code = GOT_ERR_ERRNO;
> +	err->msg = cerr->msg;
> +	return err;
>  }
>  
>  const struct got_error *
>  got_error_from_errno2(const char *prefix, const char *prefix2)
>  {
> -	static struct got_error err;
> -	static char err_msg[(PATH_MAX * 2) + 20];
> +	struct got_custom_error *cerr = get_custom_err();
> +	struct got_error *err = &cerr->err;
>  
> -	snprintf(err_msg, sizeof(err_msg), "%s: %s: %s", prefix, prefix2,
> +	snprintf(cerr->msg, sizeof(cerr->msg), "%s: %s: %s", prefix, prefix2,
>  	    strerror(errno));
>  
> -	err.code = GOT_ERR_ERRNO;
> -	err.msg = err_msg;
> -	return &err;
> +	err->code = GOT_ERR_ERRNO;
> +	err->msg = cerr->msg;
> +	return err;
>  }
>  
>  const struct got_error *
>  got_error_from_errno3(const char *prefix, const char *prefix2,
>      const char *prefix3)
>  {
> -	static struct got_error err;
> -	static char err_msg[(PATH_MAX * 3) + 20];
> +	struct got_custom_error *cerr = get_custom_err();
> +	struct got_error *err = &cerr->err;
>  
> -	snprintf(err_msg, sizeof(err_msg), "%s: %s: %s: %s", prefix, prefix2,
> -	    prefix3, strerror(errno));
> +	snprintf(cerr->msg, sizeof(cerr->msg), "%s: %s: %s: %s", prefix,
> +	    prefix2, prefix3, strerror(errno));
>  
> -	err.code = GOT_ERR_ERRNO;
> -	err.msg = err_msg;
> -	return &err;
> +	err->code = GOT_ERR_ERRNO;
> +	err->msg = cerr->msg;
> +	return err;
>  }
>  
>  const struct got_error *
>  got_error_from_errno_fmt(const char *fmt, ...)
>  {
> -	static struct got_error err;
> -	static char err_msg[PATH_MAX * 4 + 64];
> +	struct got_custom_error *cerr = get_custom_err();
> +	struct got_error *err = &cerr->err;
>  	char buf[PATH_MAX * 4];
>  	va_list ap;
>  
> @@ -123,11 +137,11 @@ got_error_from_errno_fmt(const char *fmt, ...)
>  	vsnprintf(buf, sizeof(buf), fmt, ap);
>  	va_end(ap);
>  
> -	snprintf(err_msg, sizeof(err_msg), "%s: %s", buf, strerror(errno));
> +	snprintf(cerr->msg, sizeof(cerr->msg), "%s: %s", buf, strerror(errno));
>  
> -	err.code = GOT_ERR_ERRNO;
> -	err.msg = err_msg;
> -	return &err;
> +	err->code = GOT_ERR_ERRNO;
> +	err->msg = cerr->msg;
> +	return err;
>  }
>  
>  const struct got_error *
> @@ -148,8 +162,7 @@ got_ferror(FILE *f, int code)
>  const struct got_error *
>  got_error_no_obj(struct got_object_id *id)
>  {
> -	static char msg[sizeof("object   not found") +
> -	    SHA1_DIGEST_STRING_LENGTH];
> +	char msg[sizeof("object   not found") + SHA1_DIGEST_STRING_LENGTH];
>  	char id_str[SHA1_DIGEST_STRING_LENGTH];
>  	int ret;
>  
> @@ -166,7 +179,7 @@ got_error_no_obj(struct got_object_id *id)
>  const struct got_error *
>  got_error_not_ref(const char *refname)
>  {
> -	static char msg[sizeof("reference   not found") + 1004];
> +	char msg[sizeof("reference   not found") + 1004];
>  	int ret;
>  
>  	ret = snprintf(msg, sizeof(msg), "reference %s not found", refname);
> @@ -196,17 +209,17 @@ got_error_uuid(uint32_t uuid_status, const char *prefi
>  const struct got_error *
>  got_error_path(const char *path, int code)
>  {
> -	static struct got_error err;
> -	static char msg[PATH_MAX + 128];
> +	struct got_custom_error *cerr = get_custom_err();
> +	struct got_error *err = &cerr->err;
>  	size_t i;
>  
>  	for (i = 0; i < nitems(got_errors); i++) {
>  		if (code == got_errors[i].code) {
> -			err.code = code;
> -			snprintf(msg, sizeof(msg), "%s: %s", path,
> +			err->code = code;
> +			snprintf(cerr->msg, sizeof(cerr->msg), "%s: %s", path,
>  			    got_errors[i].msg);
> -			err.msg = msg;
> -			return &err;
> +			err->msg = cerr->msg;
> +			return err;
>  		}
>  	}
>  
> @@ -216,8 +229,8 @@ got_error_path(const char *path, int code)
>  const struct got_error *
>  got_error_fmt(int code, const char *fmt, ...)
>  {
> -	static struct got_error err;
> -	static char msg[PATH_MAX * 4 + 128];
> +	struct got_custom_error *cerr = get_custom_err();
> +	struct got_error *err = &cerr->err;
>  	char buf[PATH_MAX * 4];
>  	va_list ap;
>  	size_t i;
> @@ -228,11 +241,11 @@ got_error_fmt(int code, const char *fmt, ...)
>  
>  	for (i = 0; i < nitems(got_errors); i++) {
>  		if (code == got_errors[i].code) {
> -			err.code = code;
> -			snprintf(msg, sizeof(msg), "%s: %s", buf,
> +			err->code = code;
> +			snprintf(cerr->msg, sizeof(cerr->msg), "%s: %s", buf,
>  			    got_errors[i].msg);
> -			err.msg = msg;
> -			return &err;
> +			err->msg = cerr->msg;
> +			return err;
>  		}
>  	}
>  
> 
>