Patch avoid call strlen repeatedly in loop.

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Patch avoid call strlen repeatedly in loop.

Ranier Vilela
Hi,
Please can anybody review and commit this patch.

Thanks.

Ranier Vilela

--- \dll\postgresql-12.0\a\backend\libpq\auth.c Mon Sep 30 17:06:55 2019
+++ auth.c Fri Nov 08 14:27:17 2019
@@ -1815,6 +1815,7 @@
  char ident_user[IDENT_USERNAME_MAX + 1];
  pgsocket sock_fd = PGINVALID_SOCKET; /* for talking to Ident server */
  int rc; /* Return code from a locally called function */
+ int ident_query_len;
  bool ident_return;
  char remote_addr_s[NI_MAXHOST];
  char remote_port[NI_MAXSERV];
@@ -1913,7 +1914,7 @@
  }
 
  /* The query we send to the Ident server */
- snprintf(ident_query, sizeof(ident_query), "%s,%s\r\n",
+ ident_query_len = snprintf(ident_query, sizeof(ident_query), "%s,%s\r\n",
  remote_port, local_port);
 
  /* loop in case send is interrupted */
@@ -1921,7 +1922,7 @@
  {
  CHECK_FOR_INTERRUPTS();
 
- rc = send(sock_fd, ident_query, strlen(ident_query), 0);
+ rc = send(sock_fd, ident_query, ident_query_len, 0);
  } while (rc < 0 && errno == EINTR);
 
  if (rc < 0)
@@ -3053,6 +3054,8 @@
  char   *receive_buffer = (char *) &radius_recv_pack;
  int32 service = pg_hton32(RADIUS_AUTHENTICATE_ONLY);
  uint8   *cryptvector;
+ int secretlen;
+ int passwdlen;
  int encryptedpasswordlen;
  uint8 encryptedpassword[RADIUS_MAX_PASSWORD_LENGTH];
  uint8   *md5trailer;
@@ -3125,10 +3128,12 @@
  memcpy(cryptvector, secret, strlen(secret));
 
  /* for the first iteration, we use the Request Authenticator vector */
+    secretlen = strlen(secret);
+    passwdlen = strlen(passwd);
  md5trailer = packet->vector;
  for (i = 0; i < encryptedpasswordlen; i += RADIUS_VECTOR_LENGTH)
  {
- memcpy(cryptvector + strlen(secret), md5trailer, RADIUS_VECTOR_LENGTH);
+ memcpy(cryptvector + secretlen, md5trailer, RADIUS_VECTOR_LENGTH);
 
  /*
  * .. and for subsequent iterations the result of the previous XOR
@@ -3136,7 +3141,7 @@
  */
  md5trailer = encryptedpassword + i;
 
- if (!pg_md5_binary(cryptvector, strlen(secret) + RADIUS_VECTOR_LENGTH, encryptedpassword + i))
+ if (!pg_md5_binary(cryptvector, secretlen + RADIUS_VECTOR_LENGTH, encryptedpassword + i))
  {
  ereport(LOG,
  (errmsg("could not perform MD5 encryption of password")));
@@ -3147,7 +3152,7 @@
 
  for (j = i; j < i + RADIUS_VECTOR_LENGTH; j++)
  {
- if (j < strlen(passwd))
+ if (j < passwdlen)
  encryptedpassword[j] = passwd[j] ^ encryptedpassword[j];
  else
  encryptedpassword[j] = '\0' ^ encryptedpassword[j];
@@ -3329,7 +3334,7 @@
  * Verify the response authenticator, which is calculated as
  * MD5(Code+ID+Length+RequestAuthenticator+Attributes+Secret)
  */
- cryptvector = palloc(packetlength + strlen(secret));
+ cryptvector = palloc(packetlength + secretlen);
 
  memcpy(cryptvector, receivepacket, 4); /* code+id+length */
  memcpy(cryptvector + 4, packet->vector, RADIUS_VECTOR_LENGTH); /* request
@@ -3338,10 +3343,10 @@
  if (packetlength > RADIUS_HEADER_LENGTH) /* there may be no
  * attributes at all */
  memcpy(cryptvector + RADIUS_HEADER_LENGTH, receive_buffer + RADIUS_HEADER_LENGTH, packetlength - RADIUS_HEADER_LENGTH);
- memcpy(cryptvector + packetlength, secret, strlen(secret));
+ memcpy(cryptvector + packetlength, secret, secretlen);
 
  if (!pg_md5_binary(cryptvector,
-   packetlength + strlen(secret),
+   packetlength + secretlen,
    encryptedpassword))
  {
  ereport(LOG,

auth.c.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Patch avoid call strlen repeatedly in loop.

Mark Dilger-4


On 11/8/19 9:41 AM, Ranier VF wrote:

> --- \dll\postgresql-12.0\a\backend\libpq\auth.c Mon Sep 30 17:06:55 2019
> +++ auth.c Fri Nov 08 14:27:17 2019
> @@ -1815,6 +1815,7 @@
>   char ident_user[IDENT_USERNAME_MAX + 1];
>   pgsocket sock_fd = PGINVALID_SOCKET; /* for talking to Ident server */
>   int rc; /* Return code from a locally called function */
> + int ident_query_len;
>   bool ident_return;
>   char remote_addr_s[NI_MAXHOST];
>   char remote_port[NI_MAXSERV];
> @@ -1913,7 +1914,7 @@
>   }
>  
>   /* The query we send to the Ident server */
> - snprintf(ident_query, sizeof(ident_query), "%s,%s\r\n",
> + ident_query_len = snprintf(ident_query, sizeof(ident_query), "%s,%s\r\n",
>   remote_port, local_port);
>  
>   /* loop in case send is interrupted */
> @@ -1921,7 +1922,7 @@
>   {
>   CHECK_FOR_INTERRUPTS();
>  
> - rc = send(sock_fd, ident_query, strlen(ident_query), 0);
> + rc = send(sock_fd, ident_query, ident_query_len, 0);

Hello Ranier,

In general, writing a string with snprintf and then calling strlen on
that same string is not guaranteed to give the same lengths.  You can
easily construct a case where they differ:

     char foo[3] = {0};
     int foolen;
     foolen = snprintf(foo, sizeof(foo), "%s", "xxxxxxxx");
     printf("strlen(foo) = %u, foolen = %u, foo = '%s'\n", strlen(foo),
foolen, foo);

Using standard snprintf (and not pg_snprintf), I get:

     strlen(foo) = 2, foolen = 8, foo = 'xx'

Perhaps an analysis of the surrounding code would prove that in all
cases this particular snprintf will return the same result that
strlen(ident_query) would return, but I don't care to do the analysis.
I think the way it is coded is easier to read, and probably more robust
against future changes, even if your proposed change happens to be safe
today.

As for calling strlen(ident_query) just once, caching that result, and
then looping, I don't immediately see a problem, but I also don't expect
that loop to run more than one iteration except under unusual instances.
  Do you find that send() gets interrupted a lot?  Is
strlen(ident_query) taking long enough to be significant compared to how
long send() takes?

A bit more information about the performance problem you are
encountering might make it easier to understand the motivation for this
patch.

--
Mark Dilger


Reply | Threaded
Open this post in threaded view
|

RE: Patch avoid call strlen repeatedly in loop.

Ranier Vilela


________________________________________
De: Mark Dilger <[hidden email]>
Enviado: sábado, 9 de novembro de 2019 00:12
Para: Ranier VF; [hidden email]
Assunto: Re: Patch avoid call strlen repeatedly in loop.



On 11/8/19 9:41 AM, Ranier VF wrote:

> --- \dll\postgresql-12.0\a\backend\libpq\auth.c       Mon Sep 30 17:06:55 2019
> +++ auth.c    Fri Nov 08 14:27:17 2019
> @@ -1815,6 +1815,7 @@
>       char            ident_user[IDENT_USERNAME_MAX + 1];
>       pgsocket        sock_fd = PGINVALID_SOCKET; /* for talking to Ident server */
>       int                     rc;                             /* Return code from a locally called function */
> +     int                     ident_query_len;
>       bool            ident_return;
>       char            remote_addr_s[NI_MAXHOST];
>       char            remote_port[NI_MAXSERV];
> @@ -1913,7 +1914,7 @@
>       }
>
>       /* The query we send to the Ident server */
> -     snprintf(ident_query, sizeof(ident_query), "%s,%s\r\n",
> +     ident_query_len = snprintf(ident_query, sizeof(ident_query), "%s,%s\r\n",
>                        remote_port, local_port);
>
>       /* loop in case send is interrupted */
> @@ -1921,7 +1922,7 @@
>       {
>               CHECK_FOR_INTERRUPTS();
>
> -             rc = send(sock_fd, ident_query, strlen(ident_query), 0);
> +             rc = send(sock_fd, ident_query, ident_query_len, 0);

Hello Ranier,

In general, writing a string with snprintf and then calling strlen on
that same string is not guaranteed to give the same lengths.  You can
easily construct a case where they differ:

     char foo[3] = {0};
     int foolen;
     foolen = snprintf(foo, sizeof(foo), "%s", "xxxxxxxx");
     printf("strlen(foo) = %u, foolen = %u, foo = '%s'\n", strlen(foo),
foolen, foo);

Using standard snprintf (and not pg_snprintf), I get:

     strlen(foo) = 2, foolen = 8, foo = 'xx'

Perhaps an analysis of the surrounding code would prove that in all
cases this particular snprintf will return the same result that
strlen(ident_query) would return, but I don't care to do the analysis.
I think the way it is coded is easier to read, and probably more robust
against future changes, even if your proposed change happens to be safe
today.

As for calling strlen(ident_query) just once, caching that result, and
then looping, I don't immediately see a problem, but I also don't expect
that loop to run more than one iteration except under unusual instances.
  Do you find that send() gets interrupted a lot?  Is
strlen(ident_query) taking long enough to be significant compared to how
long send() takes?

A bit more information about the performance problem you are
encountering might make it easier to understand the motivation for this
patch.

--
Mark Dilger


Reply | Threaded
Open this post in threaded view
|

RE: Patch avoid call strlen repeatedly in loop.

Ranier Vilela
In reply to this post by Mark Dilger-4
Hi Mark,
"In general, writing a string with snprintf and then calling strlen on
that same string is not guaranteed to give the same lengths.  You can
easily construct a case where they differ:

     char foo[3] = {0};
     int foolen;
     foolen = snprintf(foo, sizeof(foo), "%s", "xxxxxxxx");
     printf("strlen(foo) = %u, foolen = %u, foo = '%s'\n", strlen(foo),
foolen, foo);

Using standard snprintf (and not pg_snprintf), I get:

     strlen(foo) = 2, foolen = 8, foo = 'xx'"

Well, I've been using snprintf, no problem for several years now.
But what you reported, I would easily solve with an assert.

assert(foolen == strlen(foo));

To make sure things would stay under control.

"I think the way it is coded is easier to read, and probably more robust
against future changes, even if your proposed change happens to be safe
today."

I find it amazing that software I admire so much, such as PostgreSQL, makes extensive and heavy use of functions like strlen.
Speed makes a lot of difference, for some people it is above safety.
Maybe that's why PostgreSQL loses some battles against MySQL.
Not using strlen is for educational purposes as well. Allowing is to encourage use!
So stupid things such as:
#define CheckComplicatedStuff (a, b) (strlen (a) > strlen (b))
for (;;) {
      if CheckComplicatedStuff (x, y) {
         break;
      }
}
They start to contaminate all the code.
Using features like strlen, the programmer begins to create easy shortcuts, but in the end, they are very slow.

Maybe that's why I have things in my code like:
char sql [4096];
PQexec (cn, sql);

While MySQL for example, would look like this:
char sql [4096];
int sql_len;
sql_len = snprintf (sql, sizeof (sql), "INSERT ...");
mysql_real_query (cn, sql, sql_len);

"A bit more information about the performance problem you are
encountering might make it easier to understand the motivation for this
patch."
My motivation? Speed.
Win from MySQL, always.

Anyway I'm redoing the patch with your suggestion.
What about other functions that make extensive use of strlen?

Thank you.
Ranier Vilela

________________________________________
De: Mark Dilger <[hidden email]>
Enviado: sábado, 9 de novembro de 2019 00:12
Para: Ranier VF; [hidden email]
Assunto: Re: Patch avoid call strlen repeatedly in loop.



On 11/8/19 9:41 AM, Ranier VF wrote:

> --- \dll\postgresql-12.0\a\backend\libpq\auth.c       Mon Sep 30 17:06:55 2019
> +++ auth.c    Fri Nov 08 14:27:17 2019
> @@ -1815,6 +1815,7 @@
>       char            ident_user[IDENT_USERNAME_MAX + 1];
>       pgsocket        sock_fd = PGINVALID_SOCKET; /* for talking to Ident server */
>       int                     rc;                             /* Return code from a locally called function */
> +     int                     ident_query_len;
>       bool            ident_return;
>       char            remote_addr_s[NI_MAXHOST];
>       char            remote_port[NI_MAXSERV];
> @@ -1913,7 +1914,7 @@
>       }
>
>       /* The query we send to the Ident server */
> -     snprintf(ident_query, sizeof(ident_query), "%s,%s\r\n",
> +     ident_query_len = snprintf(ident_query, sizeof(ident_query), "%s,%s\r\n",
>                        remote_port, local_port);
>
>       /* loop in case send is interrupted */
> @@ -1921,7 +1922,7 @@
>       {
>               CHECK_FOR_INTERRUPTS();
>
> -             rc = send(sock_fd, ident_query, strlen(ident_query), 0);
> +             rc = send(sock_fd, ident_query, ident_query_len, 0);

Hello Ranier,

In general, writing a string with snprintf and then calling strlen on
that same string is not guaranteed to give the same lengths.  You can
easily construct a case where they differ:

     char foo[3] = {0};
     int foolen;
     foolen = snprintf(foo, sizeof(foo), "%s", "xxxxxxxx");
     printf("strlen(foo) = %u, foolen = %u, foo = '%s'\n", strlen(foo),
foolen, foo);

Using standard snprintf (and not pg_snprintf), I get:

     strlen(foo) = 2, foolen = 8, foo = 'xx'

Perhaps an analysis of the surrounding code would prove that in all
cases this particular snprintf will return the same result that
strlen(ident_query) would return, but I don't care to do the analysis.
I think the way it is coded is easier to read, and probably more robust
against future changes, even if your proposed change happens to be safe
today.

As for calling strlen(ident_query) just once, caching that result, and
then looping, I don't immediately see a problem, but I also don't expect
that loop to run more than one iteration except under unusual instances.
  Do you find that send() gets interrupted a lot?  Is
strlen(ident_query) taking long enough to be significant compared to how
long send() takes?

A bit more information about the performance problem you are
encountering might make it easier to understand the motivation for this
patch.

--
Mark Dilger