Re: svn commit: r1902206 - /apr/apr/trunk/encoding/apr_base64.c

[Yann Ylavic]

On Wed, Mar 29, 2023 at 1:04 PM Evgeny Kotkov via dev
 wrote:
>
> Yann Ylavic  writes:
>
> > +#undef NDEBUG /* always abort() on assert()ion failure */
> > +#include 
>
> Sorry for being late to the party, but I think there are a few problems with
> the "#undef NDEBUG" line:
>
> 1) The debug implementation of an assert() may print a diagnostic message,
> for example to stderr.  A caller of the library function may not be ready for
> this to happen when using a non-debug version of the library.
>
> 2) The actual destination of the message seems to be implementation-defined.
> For example, in Windows-based applications this may show a message box [1],
> which is probably even more unexpected for the user of the library.
>
> 3) Undefining NDEBUG before other headers may silently cause unexpected
> effects if any of those headers make some decisions based on the NDEBUG value,
> which isn't an entirely unreasonable thing to expect.

Fair points, the system printing (the faulting code) or not by default
before abort() should be #ifdef'd then? Depend on NDEBUG or an
APR_ABORT_USE_STDERR alike?
I'm not sure what the right default is though, ISTM that using system
printing before abort() for a portable [system] lib is not delusional.
This works well for users that can afford to let std outputs go or
handle/redirect them (e.g. httpd including when running as a Windows
service where the error could go to a journal/eventlog?), and an
abort() can be hard to diagnose without a message (and without a
coredump/backtrace).
Using a build time toggle does not necessarily help by the way when
all you have is an apr built by a distro/vendor (no NDEBUG set in
Debian it seems for instance).
Faulting silently by default looks harsh to me, there is no precedent
on apr abort()ing explicitly so there is no legacy behaviour to
preserve IMO. But not a strong opinion provided there is a #define'd.

>
> So, depending on whether we want the checks to soft- or hard-fault, maybe we
> could either remove the "#undef NDEBUG" line, or switch to a custom check that
> explicitly calls an abort()?

If we let some user-defined NDEBUG fly through apr_base64.c the code
will never abort(), which we always want for this int/size overflow
UB.
So possibly something like the below:

Index: encoding/apr_base64.c
===
--- encoding/apr_base64.c(revision 1908764)
+++ encoding/apr_base64.c(working copy)
@@ -20,8 +20,27 @@
  * ugly 'len' functions, which is quite a nasty cost.
  */

-#undef NDEBUG /* always abort() on assert()ion failure */
+/* An APR__ASSERT()ion failure will abort() with or without printing
+ * the faulting condition (and code location) depending on NDEBUG.
+ */
+#ifndef NDEBUG  /* use assert()/system's printing before abort() mechanism */
 #include 
+#define APR__ASSERT(cond) assert(cond)
+#else   /* just abort() */
+#if APR_HAVE_STDLIB_H
+#include 
+#endif
+#if HAVE___BUILTIN_EXPECT
+#define APR__UNLIKELY(cond) __builtin_expect(!!(cond), 0)
+#else
+#define APR__UNLIKELY(cond) (!!(cond))
+#endif
+#define APR__ASSERT(cond) do { \
+if (APR__UNLIKELY(!(cond))) { \
+abort(); \
+} \
+} while (0)
+#endif

 #include "apr_base64.h"
 #if APR_CHARSET_EBCDIC
@@ -124,7 +143,7 @@ APR_DECLARE(int) apr_base64_decode_len(const char
 bufin = (const unsigned char *) bufcoded;
 while (pr2six[*(bufin++)] <= 63);
 nprbytes = (bufin - (const unsigned char *) bufcoded) - 1;
-assert(nprbytes <= APR_BASE64_DECODE_MAX);
+APR__ASSERT(nprbytes <= APR_BASE64_DECODE_MAX);

 return (int)(((nprbytes + 3u) / 4u) * 3u + 1u);
 }
@@ -161,7 +180,7 @@ APR_DECLARE(int) apr_base64_decode_binary(unsigned
 bufin = (const unsigned char *) bufcoded;
 while (pr2six[*(bufin++)] <= 63);
 nprbytes = (bufin - (const unsigned char *) bufcoded) - 1;
-assert(nprbytes <= APR_BASE64_DECODE_MAX);
+APR__ASSERT(nprbytes <= APR_BASE64_DECODE_MAX);
 nbytesdecoded = (int)(((nprbytes + 3u) / 4u) * 3u);

 bufout = (unsigned char *) bufplain;
@@ -206,7 +225,7 @@ static const char basis_64[] =

 APR_DECLARE(int) apr_base64_encode_len(int len)
 {
-assert(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
+APR__ASSERT(len >= 0 && len <= APR_BASE64_ENCODE_MAX);

 return ((len + 2) / 3 * 4) + 1;
 }
@@ -219,7 +238,7 @@ APR_DECLARE(int) apr_base64_encode(char *encoded,
 int i;
 char *p;

-assert(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
+APR__ASSERT(len >= 0 && len <= APR_BASE64_ENCODE_MAX);

 p = encoded;
 for (i = 0; i < len - 2; i += 3) {
@@ -258,7 +277,7 @@ APR_DECLARE(int) apr_base64_encode_binary(char *en
 int i;
 char *p;

-assert(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
+APR__ASSERT(len >= 0 && len <= APR_BASE64_ENCODE_MAX);

 p = encoded;
 for (i = 0; i < len - 2; i += 3) {
@@ -292,7 +311,7 @@ APR_DECLARE(char *) apr_pbase64_encode(apr_pool_t
 char *encoded;
 apr_size_t len = strlen(string);

Re: [VOTE] Release apr-1.7.3

[Eric Covener]

On Mon, Mar 27, 2023 at 4:18 AM Ruediger Pluem  wrote:
>
> 1.7.3-rc1 is here:
>
> https://apr.apache.org/dev/dist/
>
> For the release of apr-1.7.3
>   [x]  +1 looks great!
>   [  ]  -1 something is broken
>
> I will let the vote run through end-of-week.

+1 AIX/xlc/ppc64 no regression

A Message from the Board to PMC members

[Rich Bowen]

Dear Apache Project Management Committee (PMC) members,

The Board wants to take just a moment of your time to communicate a few
things that seem to have been forgotten by a number of PMC members,
across the Foundation, over the past few years.  Please note that this
is being sent to all projects - yours has not been singled out.

The Project Management Committee (PMC) as a whole[1] is tasked with the
oversight, health, and sustainability of the project. The PMC members
are responsible collectively, and individually, for ensuring that the
project operates in a way that is in line with ASF philosophy, and in a
way that serves the developers and users of the project.

The PMC Chair is not the project leader, in any sense. It is the person
who files board reports and makes sure they are delivered on time. It
is the secretary for the project, and the project’s  ambassador to the
Board of Directors. The VP title is given as an artifact of US
corporate law, and not because the PMC Chair has any special powers. If
you are treating your PMC Chair as the project lead, or granting them
any other special powers or privileges, you need to be aware that
that’s not the intent of the Chair role. The Chair is a PMC member peer
with a few extra duties.

Every PMC member has an equal voice in deliberations. Each has one
vote. Each has veto power. Every vote weighs the same. It is not only
your right, but it is your obligation, to use that vote for the good of
the project and its users, not to appease the Chair, your employer, or
any other voice in the project. 

Every PMC member can, and should, nominate new committers, and new PMC
members. This is not the sole domain of the PMC Chair. This might be
your most important responsibility to the project, as succession
planning is the path to sustainability.

Every PMC member can, and should, respond when the Board sends email to
your private list. You should not wait for the PMC Chair to respond.
The Board views the entire PMC as responsible for the project, not just
one member.

Every PMC member should be subscribed to the private@ mailing list. If
you are not, then you are neglecting your duty of oversight. If you no
longer wish to be responsible for oversight of the project, you should
resign your PMC seat, not merely drop off of the private@ list and
ignore it. You can determine which PMC members are not subscribed to
your private list by looking at your PMC roster at
https://whimsy.apache.org/roster/committee/  Names with an asterisk (*)
next to them are not subscribed to the list. We encourage you to take a
moment to contact them with this information.

Thank you for your attention to these matters, and thank you for
keeping our projects healthy.

Rich, for The Board of Directors

[1] https://apache.org/foundation/how-it-works.html#pmc-members

Re: svn commit: r1902206 - /apr/apr/trunk/encoding/apr_base64.c

[Evgeny Kotkov via dev]

Yann Ylavic  writes:

> +#undef NDEBUG /* always abort() on assert()ion failure */
> +#include 

Sorry for being late to the party, but I think there are a few problems with
the "#undef NDEBUG" line:

1) The debug implementation of an assert() may print a diagnostic message,
for example to stderr.  A caller of the library function may not be ready for
this to happen when using a non-debug version of the library.

2) The actual destination of the message seems to be implementation-defined.
For example, in Windows-based applications this may show a message box [1],
which is probably even more unexpected for the user of the library.

3) Undefining NDEBUG before other headers may silently cause unexpected
effects if any of those headers make some decisions based on the NDEBUG value,
which isn't an entirely unreasonable thing to expect.

So, depending on whether we want the checks to soft- or hard-fault, maybe we
could either remove the "#undef NDEBUG" line, or switch to a custom check that
explicitly calls an abort()?

[1] 
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/assert-macro-assert-wassert#remarks


Thanks,
Evgeny Kotkov