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

2023-03-30 Thread Evgeny Kotkov via dev 
Yann Ylavic  writes:

> 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

Yes, something along these lines, although I was thinking we could maybe
get away with just adding a small helper that covers all cases, as in the
attached patch.


Thanks,
Evgeny Kotkov
Index: encoding/apr_base64.c
===
--- encoding/apr_base64.c   (revision 1908804)
+++ encoding/apr_base64.c   (working copy)
@@ -20,7 +20,6 @@
  * ugly 'len' functions, which is quite a nasty cost.
  */
 
-#undef NDEBUG /* always abort() on assert()ion failure */
 #include 
 
 #include "apr_base64.h"
@@ -28,6 +27,17 @@
 #include "apr_xlate.h"
 #endif/* APR_CHARSET_EBCDIC */
 
+#if APR_HAVE_STDLIB_H
+#include 
+#endif
+
+#define verify_condition(cond) do { \
+if (!(cond)) {  \
+assert(0 && #cond); \
+abort();\
+}   \
+} while (0)
+
 /* Above APR_BASE64_ENCODE_MAX length the encoding can't fit in an int >= 0 */
 #define APR_BASE64_ENCODE_MAX 1610612733
 
@@ -124,7 +134,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);
+verify_condition(nprbytes <= APR_BASE64_DECODE_MAX);
 
 return (int)(((nprbytes + 3u) / 4u) * 3u + 1u);
 }
@@ -161,7 +171,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);
+verify_condition(nprbytes <= APR_BASE64_DECODE_MAX);
 nbytesdecoded = (int)(((nprbytes + 3u) / 4u) * 3u);
 
 bufout = (unsigned char *) bufplain;
@@ -206,7 +216,7 @@ static const char basis_64[] =
 
 APR_DECLARE(int) apr_base64_encode_len(int len)
 {
-assert(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
+verify_condition(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
 
 return ((len + 2) / 3 * 4) + 1;
 }
@@ -219,7 +229,7 @@ APR_DECLARE(int) apr_base64_encode(char *encoded,
 int i;
 char *p;
 
-assert(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
+verify_condition(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
 
 p = encoded;
 for (i = 0; i < len - 2; i += 3) {
@@ -258,7 +268,7 @@ APR_DECLARE(int) apr_base64_encode_binary(char *en
 int i;
 char *p;
 
-assert(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
+verify_condition(len >= 0 && len <= APR_BASE64_ENCODE_MAX);
 
 p = encoded;
 for (i = 0; i < len - 2; i += 3) {
@@ -292,7 +302,7 @@ APR_DECLARE(char *) apr_pbase64_encode(apr_pool_t
 char *encoded;
 apr_size_t len = strlen(string);
 
-assert(len <= (apr_size_t)APR_BASE64_ENCODE_MAX);
+verify_condition(len <= (apr_size_t)APR_BASE64_ENCODE_MAX);
 encoded = (char *) apr_palloc(p, apr_base64_encode_len((int)len));
 apr_base64_encode(encoded, string, (int)len);

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

2023-03-29 Thread 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: svn commit: r1902206 - /apr/apr/trunk/encoding/apr_base64.c

2023-03-29 Thread 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

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

2022-10-17 Thread Yann Ylavic 
On Mon, Jul 25, 2022 at 12:43 PM Ruediger Pluem  wrote:
>
>
>
> On 6/23/22 8:49 PM, Ruediger Pluem wrote:
> >
> >
> > On 6/23/22 5:12 PM, yla...@apache.org wrote:
> >> Author: ylavic
> >> Date: Thu Jun 23 15:12:47 2022
> >> New Revision: 1902206
> >
> >
> >> @@ -275,16 +284,17 @@ APR_DECLARE(int) apr_base64_encode_binar
> >>  }
> >>
> >>  *p++ = '\0';
> >> -return (int)(p - encoded);
> >> +return (unsigned int)(p - encoded);
> >>  }
> >>
> >>  APR_DECLARE(char *) apr_pbase64_encode(apr_pool_t *p, const char *string)
> >>  {
> >>  char *encoded;
> >> -int l = strlen(string);
> >> +apr_size_t len = strlen(string);
> >>
> >> -encoded = (char *) apr_palloc(p, apr_base64_encode_len(l));
> >> -apr_base64_encode(encoded, string, l);
> >> +assert(len <= (apr_size_t)APR_INT32_MAX);
> >
> > Shouldn't this be INT_MAX or APR_BASE64_ENCODE_MAX?
>
> Any update on this comment? I guess INT_MAX or APR_INT32_MAX is mood given 
> the result of the discussion in this thread, but it
> probably should be APR_BASE64_ENCODE_MAX?

Good point, r1904666.

>
> >
> >> +encoded = (char *) apr_palloc(p, apr_base64_encode_len((int)len));
> >> +apr_base64_encode(encoded, string, (int)len);
> >>
> >>  return encoded;
> >>  }


Regards;
Yann.

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

2022-07-25 Thread Ruediger Pluem 



On 6/23/22 8:49 PM, Ruediger Pluem wrote:
> 
> 
> On 6/23/22 5:12 PM, yla...@apache.org wrote:
>> Author: ylavic
>> Date: Thu Jun 23 15:12:47 2022
>> New Revision: 1902206
>
> 
>> @@ -275,16 +284,17 @@ APR_DECLARE(int) apr_base64_encode_binar
>>  }
>>  
>>  *p++ = '\0';
>> -return (int)(p - encoded);
>> +return (unsigned int)(p - encoded);
>>  }
>>  
>>  APR_DECLARE(char *) apr_pbase64_encode(apr_pool_t *p, const char *string)
>>  {
>>  char *encoded;
>> -int l = strlen(string);
>> +apr_size_t len = strlen(string);
>>  
>> -encoded = (char *) apr_palloc(p, apr_base64_encode_len(l));
>> -apr_base64_encode(encoded, string, l);
>> +assert(len <= (apr_size_t)APR_INT32_MAX);
> 
> Shouldn't this be INT_MAX or APR_BASE64_ENCODE_MAX?

Any update on this comment? I guess INT_MAX or APR_INT32_MAX is mood given the 
result of the discussion in this thread, but it
probably should be APR_BASE64_ENCODE_MAX?

> 
>> +encoded = (char *) apr_palloc(p, apr_base64_encode_len((int)len));
>> +apr_base64_encode(encoded, string, (int)len);
>>  
>>  return encoded;
>>  }
>>
>>
>>
> 

Regards

Rüdiger

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

2022-07-25 Thread Ruediger Pluem 



On 6/24/22 11:56 AM, Yann Ylavic wrote:
> On Thu, Jun 23, 2022 at 8:50 PM Ruediger Pluem  wrote:
>>
>> On 6/23/22 5:12 PM, yla...@apache.org wrote:
>>>
>>> +/* Above APR_BASE64_ENCODE_MAX length the encoding can't fit in an int >= 
>>> 0 */
>>> +#define APR_BASE64_ENCODE_MAX 1610612733
>>> +
>>> +/* Above APR_BASE64_DECODE_MAX length the decoding can't fit in an int >= 
>>> 0 */
>>> +#define APR_BASE64_DECODE_MAX 2863311524u
>>> +
>>
>> Doesn't this depend on the storage size of int on the respective 
>> architecture and thus
>> should be derived from INT_MAX?
> 
> There is no APR_INT_MAX unfortunately, I could do something like:
> 
> #if APR_HAVE_STDINT_H /* C99, but maintainer-mode is C89 */
> #include 
> #define APR_BASE64_LEN_MAX INT_MAX
> #else
> #define APR_BASE64_LEN_MAX APR_INT32_MAX
> #endif
> 
> and use APR_BASE64_LEN_MAX instead of APR_INT32_MAX here and there,
> but I doubt there are any architectures (we care about) where
> sizeof(int) != 4.
> I don't think we support < 32bit archs, do we?
> For >= 32bit archs, of the 5 data models (LP32, ILP32, ILP64, LLP64
> and LP64), only LP32 (i.e. WIN16 API, Apple Macintosh) and ILP64 ([1]
> mentions HAL Computer Systems port of Solaris to the SPARC64) don't
> have 32bit ints, and I don't think we care about those either.
> 
> So we should be safe assuming ints are 32bit?

Given your explanations about I tend to say yes. Thanks for the details.

Regards

Rüdiger

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

2022-06-24 Thread Yann Ylavic 
On Fri, Jun 24, 2022 at 11:56 AM Yann Ylavic  wrote:
>
> On Thu, Jun 23, 2022 at 8:50 PM Ruediger Pluem  wrote:
> >
> > On 6/23/22 5:12 PM, yla...@apache.org wrote:
> > >
> > > +/* Above APR_BASE64_ENCODE_MAX length the encoding can't fit in an int 
> > > >= 0 */
> > > +#define APR_BASE64_ENCODE_MAX 1610612733
> > > +
> > > +/* Above APR_BASE64_DECODE_MAX length the decoding can't fit in an int 
> > > >= 0 */
> > > +#define APR_BASE64_DECODE_MAX 2863311524u
> > > +
> >
> > Doesn't this depend on the storage size of int on the respective 
> > architecture and thus
> > should be derived from INT_MAX?
>
> There is no APR_INT_MAX unfortunately

By the way, I don't see much value in the
APR_{U,}INT{8,16,32,64}_{MIN,MAX} definitions we have, those values
have well known min/max (i.e. fixed).
I'd rather we define APR_{U,}{CHAR,SHORT,INT,LONG,LONGLONG}_MAX ones,
but without C99 (stdint.h, though most if not all platforms should
have this header now) it may be tricky (if not impossible) to define
them portably..

>
> Regards;
> Yann.

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

2022-06-24 Thread Yann Ylavic 
On Thu, Jun 23, 2022 at 8:50 PM Ruediger Pluem  wrote:
>
> On 6/23/22 5:12 PM, yla...@apache.org wrote:
> >
> > +/* Above APR_BASE64_ENCODE_MAX length the encoding can't fit in an int >= 
> > 0 */
> > +#define APR_BASE64_ENCODE_MAX 1610612733
> > +
> > +/* Above APR_BASE64_DECODE_MAX length the decoding can't fit in an int >= 
> > 0 */
> > +#define APR_BASE64_DECODE_MAX 2863311524u
> > +
>
> Doesn't this depend on the storage size of int on the respective architecture 
> and thus
> should be derived from INT_MAX?

There is no APR_INT_MAX unfortunately, I could do something like:

#if APR_HAVE_STDINT_H /* C99, but maintainer-mode is C89 */
#include 
#define APR_BASE64_LEN_MAX INT_MAX
#else
#define APR_BASE64_LEN_MAX APR_INT32_MAX
#endif

and use APR_BASE64_LEN_MAX instead of APR_INT32_MAX here and there,
but I doubt there are any architectures (we care about) where
sizeof(int) != 4.
I don't think we support < 32bit archs, do we?
For >= 32bit archs, of the 5 data models (LP32, ILP32, ILP64, LLP64
and LP64), only LP32 (i.e. WIN16 API, Apple Macintosh) and ILP64 ([1]
mentions HAL Computer Systems port of Solaris to the SPARC64) don't
have 32bit ints, and I don't think we care about those either.

So we should be safe assuming ints are 32bit?

Regards;
Yann.

[1] https://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models

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

2022-06-23 Thread Ruediger Pluem 



On 6/23/22 5:12 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Thu Jun 23 15:12:47 2022
> New Revision: 1902206
> 
> URL: http://svn.apache.org/viewvc?rev=1902206=rev
> Log:
> apr_base64: Make sure encoding/decoding lengths fit in an int >= 0.
> 
> The (old) API of apr_base64 functions has always used int for representing
> lengths and it does not return errors. Make sure to abort() if the provided
> data don't fit.
> 
> * encoding/apr_base64.c():
>   #define APR_BASE64_ENCODE_MAX and APR_BASE64_DECODE_MAX as the hard length
>   limits for encoding and decoding respectively.
> 
> * encoding/apr_base64.c(apr_base64_encode_len, apr_base64_encode,
> apr_base64_encode_binary, apr_pbase64_encode):
>   abort() if the given length is above APR_BASE64_ENCODE_MAX.
> 
> * encoding/apr_base64.c(apr_base64_decode_len, apr_base64_decode,
> apr_base64_decode_binary, apr_pbase64_decode):
>   abort() if the given plain buffer length is above APR_BASE64_DECODE_MAX.
> 
> 
> Modified:
> apr/apr/trunk/encoding/apr_base64.c
> 
> Modified: apr/apr/trunk/encoding/apr_base64.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/encoding/apr_base64.c?rev=1902206=1902205=1902206=diff
> ==
> --- apr/apr/trunk/encoding/apr_base64.c (original)
> +++ apr/apr/trunk/encoding/apr_base64.c Thu Jun 23 15:12:47 2022
> @@ -20,11 +20,20 @@
>   * ugly 'len' functions, which is quite a nasty cost.
>   */
>  
> +#undef NDEBUG /* always abort() on assert()ion failure */
> +#include 
> +
>  #include "apr_base64.h"
>  #if APR_CHARSET_EBCDIC
>  #include "apr_xlate.h"
>  #endif   /* APR_CHARSET_EBCDIC */
>  
> +/* Above APR_BASE64_ENCODE_MAX length the encoding can't fit in an int >= 0 
> */
> +#define APR_BASE64_ENCODE_MAX 1610612733
> +
> +/* Above APR_BASE64_DECODE_MAX length the decoding can't fit in an int >= 0 
> */
> +#define APR_BASE64_DECODE_MAX 2863311524u
> +

Doesn't this depend on the storage size of int on the respective architecture 
and thus
should be derived from INT_MAX?

>  /* ck but it's fast and const should make it shared text page. */
>  static const unsigned char pr2six[256] =
>  {

> @@ -275,16 +284,17 @@ APR_DECLARE(int) apr_base64_encode_binar
>  }
>  
>  *p++ = '\0';
> -return (int)(p - encoded);
> +return (unsigned int)(p - encoded);
>  }
>  
>  APR_DECLARE(char *) apr_pbase64_encode(apr_pool_t *p, const char *string)
>  {
>  char *encoded;
> -int l = strlen(string);
> +apr_size_t len = strlen(string);
>  
> -encoded = (char *) apr_palloc(p, apr_base64_encode_len(l));
> -apr_base64_encode(encoded, string, l);
> +assert(len <= (apr_size_t)APR_INT32_MAX);

Shouldn't this be INT_MAX or APR_BASE64_ENCODE_MAX?

> +encoded = (char *) apr_palloc(p, apr_base64_encode_len((int)len));
> +apr_base64_encode(encoded, string, (int)len);
>  
>  return encoded;
>  }
> 
> 
> 

Regards

Rüdiger