Re: [PATCH] don't use regexp in commit.c

[Aris Adamantiadis <aris@xxxxxxxxxxxx>]

Hi Bernhard,

Thanks for you feedback. I have a few questions about your patch ; You
enumerate a good list of bugs,but why remove the broken code when you
can fix it ?
Did you run into bug caused by it ?

ssh_init() call may be indeed omitted by caller, but if you look it's
the first function being called by ssh_connect.
Is there a specific reason you prefered to remove it ?

Thanks,

Aris

Le 11/02/11 23:02, Bernhard R. Link a écrit :
> Remove the use of regexec in commit.c.
> 
> This fixes several problems of the old code by removing it:
>  - ssh_regex_init not checking malloc() result for NULL.
>  - connect.c setting _POSIX_C_SOURCE too late to have effect.
>  - this was most likely the reason config.h was not included
>    before the system headers (so this is added back)
>  - ssh_regex_init printing error message to stderro but
>    documentation saying it is "set".
>  - ssh_regex_init set only called by ssh_init, which documents
>    itself it may "be omitted if your program is not multithreaded"
>    (and i_regex not tested for NULL in connect.c)
> 
> ---
>  Not tested at all...
> 
>  include/libssh/priv.h |    2 -
>  src/connect.c         |   76 +++++++++++++-----------------------------------
>  src/init.c            |    3 --
>  3 files changed, 21 insertions(+), 60 deletions(-)
> 
> diff --git a/include/libssh/priv.h b/include/libssh/priv.h
> index 66f24e2..4dbc445 100644
> --- a/include/libssh/priv.h
> +++ b/include/libssh/priv.h
> @@ -172,8 +172,6 @@ void ssh_packet_set_callbacks(ssh_session session, ssh_packet_callbacks callback
>  void ssh_packet_set_default_callbacks(ssh_session session);
>  void ssh_packet_process(ssh_session session, uint8_t type);
>  /* connect.c */
> -int ssh_regex_init(void);
> -void ssh_regex_finalize(void);
>  socket_t ssh_connect_host(ssh_session session, const char *host,const char
>          *bind_addr, int port, long timeout, long usec);
>  socket_t ssh_connect_host_nonblocking(ssh_session session, const char *host,
> diff --git a/src/connect.c b/src/connect.c
> index ec0a2ca..b962c78 100644
> --- a/src/connect.c
> +++ b/src/connect.c
> @@ -21,6 +21,8 @@
>   * MA 02111-1307, USA.
>   */
>  
> +#include "config.h"
> +
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <stdio.h>
> @@ -78,14 +80,6 @@
>  #error "Your system must have getaddrinfo()"
>  #endif
>  
> -#ifdef HAVE_REGCOMP
> -/* don't declare gnu extended regexp's */
> -#ifndef _POSIX_C_SOURCE
> -#define _POSIX_C_SOURCE
> -#endif
> -#include <regex.h>
> -#endif /* HAVE_REGCOMP */
> -
>  #ifdef _WIN32
>  void ssh_sock_set_nonblocking(socket_t sock) {
>    u_long nonblocking = 1;
> @@ -118,53 +112,27 @@ void ssh_sock_set_blocking(socket_t sock) {
>  
>  #endif /* _WIN32 */
>  
> -#ifdef HAVE_REGCOMP
> -#define IPEXPR    "^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$"
> -
> -static regex_t *ip_regex = NULL;
> -
> -/** @internal
> - * @brief initializes and compile the regexp to be used for IP matching
> - * @returns -1 on error (and error message is set)
> - * @returns 0 on success
> - */
> -int ssh_regex_init(){
> -  if(ip_regex==NULL){
> -    int err;
> -    regex_t *regex=malloc(sizeof (regex_t));
> -    ZERO_STRUCTP(regex);
> -    err = regcomp(regex, IPEXPR, REG_EXTENDED | REG_NOSUB);
> -    if(err != 0){
> -      char buffer[128];
> -      regerror(err,regex,buffer,sizeof(buffer));
> -      fprintf(stderr,"Error while compiling regular expression : %s\n",buffer);
> -      SAFE_FREE(regex);
> -      return -1;
> -    }
> -    ip_regex=regex;
> -  }
> -  return 0;
> -}
> -
> -/** @internal
> - * @brief clean up the IP regexp
> - */
> -void ssh_regex_finalize(){
> -  if(ip_regex){
> -    regfree(ip_regex);
> -    SAFE_FREE(ip_regex);
> +static int isipaddr(const char *hostname) {
> +  int number = 0, gotnumber = 0, block = 0;
> +
> +  for ( ; *hostname != '\0' ; hostname++) {
> +    if (*hostname == '.') {
> +      if (!gotnumber || block > 2 || number > 255)
> +	      return 0;
> +      block++;
> +      gotnumber = 0;
> +      number = 0;
> +    } else if (*hostname >= '0' && *hostname <= '9') {
> +      gotnumber = 1;
> +      if (number > 25)
> +        return 0;
> +      number = number*10 + (*hostname - '0');
> +    } else
> +      return 0;
>    }
> +  return block == 3 && gotnumber && number <= 255;
>  }
>  
> -#else /* HAVE_REGCOMP */
> -int ssh_regex_init(){
> -  return 0;
> -}
> -void ssh_regex_finalize(){
> -}
> -#endif
> -
> -
>  static int ssh_connect_socket_close(socket_t s){
>  #ifdef _WIN32
>    return closesocket(s);
> @@ -194,13 +162,11 @@ static int getai(ssh_session session, const char *host, int port, struct addrinf
>      hints.ai_flags=AI_NUMERICSERV;
>  #endif
>    }
> -#ifdef HAVE_REGCOMP
> -  if(regexec(ip_regex,host,0,NULL,0) == 0){
> +  if(isipaddr(host)){
>      /* this is an IP address */
>      ssh_log(session,SSH_LOG_PACKET,"host %s matches an IP address",host);
>      hints.ai_flags |= AI_NUMERICHOST;
>    }
> -#endif
>    return getaddrinfo(host, service, &hints, ai);
>  }
>  
> diff --git a/src/init.c b/src/init.c
> index 385cb7a..7b29d2b 100644
> --- a/src/init.c
> +++ b/src/init.c
> @@ -57,8 +57,6 @@ int ssh_init(void) {
>      return -1;
>    if(ssh_socket_init())
>      return -1;
> -  if(ssh_regex_init())
> -    return -1;
>    return 0;
>  }
>  
> @@ -74,7 +72,6 @@ int ssh_init(void) {
>   */
>  int ssh_finalize(void) {
>    ssh_threads_finalize();
> -  ssh_regex_finalize();
>    ssh_crypto_finalize();
>    ssh_socket_cleanup();
>  #ifdef HAVE_LIBGCRYPT