Re: [VOTE] Merge svn-auth-x509 branch to trunk?

[Ben Reser]

On 8/7/14 12:16 PM, Branko Čibej wrote:
> On 07.08.2014 19:03, Ben Reser wrote:
>> This appears to be because pointers are unsigned and apr_size_t is signed.
> 
> You mean the other way around, surely.

Yes, thinko.

>> Guess we can just cast the pointer arithmetic to apr_size_t.  So they end up
>> looking like this respectively:
>>
>> if (*len > (apr_size_t)(end - *p))
>> if (len != (apr_size_t)(end - p))
> 
> There's a reason why ptrdiff_t exists. Maybe we should use it? I'm not fond of
> adding casts to code to silence warnings that could be fixed by using the
> correct type throughout.

I expected that change would just shift the warnings around but it removes the
warnings entirely.  I'm still not sure it's right.  We're setting a ptrdiff_t
with the value from an apr_size_t.  Shouldn't that result in a possible loss of
data since we're setting a signed value (that should be the same size) with an
unsigned value?  At least I was under the impression that ptrdiff_t and size_t
were both 32-bits on 32-bit platforms and 64-bits on 64-bit platforms.

The casts seem safer to me because I know that the cast is safe because the
difference from the two pointers can never be bigger than an apr_size_t because
our buffer size is provided via an apr_size_t.

Index: subversion/libsvn_subr/x509.h
===================================================================
--- subversion/libsvn_subr/x509.h       (revision 1616533)
+++ subversion/libsvn_subr/x509.h       (working copy)
@@ -84,7 +84,7 @@
  */
 typedef struct _x509_buf {
   int tag;
-  apr_size_t len;
+  ptrdiff_t len;
   const unsigned char *p;
 } x509_buf;
 
Index: subversion/libsvn_subr/x509parse.c
===================================================================
--- subversion/libsvn_subr/x509parse.c  (revision 1616533)
+++ subversion/libsvn_subr/x509parse.c  (working copy)
@@ -67,7 +67,7 @@
  */
 static svn_error_t *
 asn1_get_len(const unsigned char **p, const unsigned char *end,
-             apr_size_t *len)
+             ptrdiff_t *len)
 {
   if ((end - *p) < 1)
     return svn_error_create(SVN_ERR_ASN1_OUT_OF_DATA, NULL, NULL);
@@ -106,7 +106,7 @@
 
 static svn_error_t *
 asn1_get_tag(const unsigned char **p,
-             const unsigned char *end, apr_size_t *len, int tag)
+             const unsigned char *end, ptrdiff_t *len, int tag)
 {
   if ((end - *p) < 1)
     return svn_error_create(SVN_ERR_ASN1_OUT_OF_DATA, NULL, NULL);
@@ -122,7 +122,7 @@
 static svn_error_t *
 asn1_get_int(const unsigned char **p, const unsigned char *end, int *val)
 {
-  apr_size_t len;
+  ptrdiff_t len;
 
   SVN_ERR(asn1_get_tag(p, end, &len, ASN1_INTEGER));
 
@@ -146,7 +146,7 @@
 x509_get_version(const unsigned char **p, const unsigned char *end, int *ver)
 {
   svn_error_t *err;
-  apr_size_t len;
+  ptrdiff_t len;
 
   err = asn1_get_tag(p, end, &len,
                      ASN1_CONTEXT_SPECIFIC | ASN1_CONSTRUCTED | 0);
@@ -220,7 +220,7 @@
 x509_get_alg(const unsigned char **p, const unsigned char *end, x509_buf * alg)
 {
   svn_error_t *err;
-  apr_size_t len;
+  ptrdiff_t len;
 
   err = asn1_get_tag(p, end, &len, ASN1_CONSTRUCTED | ASN1_SEQUENCE);
   if (err)
@@ -272,7 +272,7 @@
               x509_name * cur, apr_pool_t *result_pool)
 {
   svn_error_t *err;
-  apr_size_t len;
+  ptrdiff_t len;
   const unsigned char *end2;
   x509_buf *oid;
   x509_buf *val;
@@ -363,7 +363,7 @@
   svn_error_t *err;
   apr_status_t ret;
   int tag;
-  apr_size_t len;
+  ptrdiff_t len;
   char *date;
   apr_time_exp_t xt = { 0 };
   char tz;
@@ -450,7 +450,7 @@
                apr_pool_t *scratch_pool)
 {
   svn_error_t *err;
-  apr_size_t len;
+  ptrdiff_t len;
 
   err = asn1_get_tag(p, end, &len, ASN1_CONSTRUCTED | ASN1_SEQUENCE);
   if (err)
@@ -475,7 +475,7 @@
 x509_get_sig(const unsigned char **p, const unsigned char *end, x509_buf * sig)
 {
   svn_error_t *err;
-  apr_size_t len;
+  ptrdiff_t len;
 
   sig->tag = **p;
 
@@ -536,7 +536,7 @@
              const unsigned char *end)
 {
   svn_error_t *err;
-  apr_size_t len;
+  ptrdiff_t len;
 
   if (*p == end)
     return SVN_NO_ERROR;
@@ -567,7 +567,7 @@
 
   while (*p < end)
     {
-      apr_size_t ext_len;
+      ptrdiff_t ext_len;
       const unsigned char *ext_start, *sna_end;
       err = asn1_get_tag(p, end, &ext_len, ASN1_CONSTRUCTED | ASN1_SEQUENCE);
       if (err)
@@ -1029,7 +1029,7 @@
                     apr_pool_t *scratch_pool)
 {
   svn_error_t *err;
-  apr_size_t len;
+  ptrdiff_t len;
   const unsigned char *p;
   const unsigned char *end;
   x509_cert *crt;