stripslashes() improvements

21 years ago by Alexander Valyalkin — view source — reply

unread

Today I checked stripslashes() function.
My version of the function if faster, more clearer and bugs free (I hope
:))
Below you can see the source and unified diff:

============source=============
PHPAPI void php_stripslashes(char *str, size_t *len TSRMLS_DC)
{
char *s, t;
size_t l;
size_t dst_len; / length the of stripped string */

 if (len != NULL) l = dst_len = *len;
 else l = dst_len = strlen(str);
 if (l < 2) return; /* there is no characters to strip */
 s = t = str;

 if (PG(magic_quotes_sybase)) { /* sybase magic_quotes ( '' -> ', \0 ->

NULL) */
while (l > 1) {
if (*t == ''' && *(t + 1) == ''') {
*s++ = ''';
t += 2;
l--;
dst_len--;
} else if (*t == '\' && *(t + 1) == '0') {
*s++ = '\0';
t += 2;
l--;
dst_len--;
} else *s++ = t++;
l--;
}
} else { / ordinary magic_qoutes (not sybase) ( \ -> , ' -> ', "
-> ", \0 -> NULL) */
while (l > 1) {
if (*t == '\') {
t++;
switch (*t) {
case '\' :
case ''' :
case '"' :
*s++ = *t++; dst_len--; break;
case '0' : *s++ = '\0'; t++; dst_len--; break;
default : *s++ = '\'; *s++ = *t++; break;
}
l -= 2;
} else {
*s++ = *t++;
l--;
}
}
}
if (l == 1) *s++ = t; / copy the last symbol */
if (len != NULL) len = dst_len; / set length of the stripped string
*/
*s = '\0';
}
============source=============

unified diff:
====================cut===================
--- string.c Thu May 13 20:44:32 2004
+++ string_new.c Tue Jun 08 16:49:26 2004
@@ -2156,73 +2156,54 @@
/* {{{ php_stripslashes

be careful, this edits the string in-place */
-PHPAPI void php_stripslashes(char *str, int *len TSRMLS_DC)
+PHPAPI void php_stripslashes(char *str, size_t *len TSRMLS_DC)
{
char *s, *t;

int l;
if (len != NULL) {
```
  l = *len;
```
} else {
```
  l = strlen(str);
```
}
s = str;
t = str;

size_t l;
size_t dst_len; /* length the of stripped string */

if (PG(magic_quotes_sybase)) {
```
  while (l > 0) {
```
```
      if (*t == '\'') {
```

          if ((l > 0) && (t[1] == '\'')) {

```
              t++;
```
```
              if (len != NULL)
```
```
                  (*len)--;
```

if (len != NULL) l = dst_len = *len;
else l = dst_len = strlen(str);
if (l < 2) return; /* there is no characters to strip */
s = t = str;
if (PG(magic_quotes_sybase)) { /* sybase magic_quotes ( '' -> ', \0
-> NULL) */
```
   while (l > 1) {
```

       if (*t == '\'' && *(t + 1) == '\'') {

```
           *s++ = '\'';
```
```
           t += 2;
               l--;
```

```
          }
```
```
          *s++ = *t++;
```

      } else if (*t == '\\' && l > 0 && t[1] == '0') {

```
           dst_len--;
```

       } else if (*t == '\\' && *(t + 1) == '0') {
               *s++='\0';
               t += 2;

```
              if (len != NULL)
```

                  (*len)--;
               l--;

```
      } else {
```
```
          *s++ = *t++;
```
```
      }
```

```
           dst_len--;
```

       } else *s++ = *t++;
       l--;
   }

```
  *s = '\0';
```
```
  return;
```
}
while (l > 0) {

} else { /* ordinary magic_qoutes (not sybase) ( \ -> , ' -> ', "
-> ", \0 -> NULL) */

   while (l > 1) {
   if (*t == '\\') {

      t++;                /* skip the slash */

```
      if (len != NULL)
```
```
          (*len)--;
```
```
      l--;
```
```
      if (l > 0) {
```
```
          if (*t == '0') {
```

              *s++='\0';
               t++;

```
          } else {
```

              *s++ = *t++;    /* preserve the next character */

```
          }
```
```
          l--;
```

```
           switch (*t) {
```
```
               case '\\' :
```
```
               case '\'' :
```
```
               case '"' :
```

                   *s++ = *t++; dst_len--; break;

               case '0' : *s++ = '\0'; t++; dst_len--; break;

               default : *s++ = '\\'; *s++ = *t++; break;
       }

```
           l -= 2;
   } else {
```

      if (s != t) {
           *s++ = *t++;

```
      } else {
```
```
          s++;
```
```
          t++;
```
```
      }
       l--;
   }
```
}
if (s != t) {
```
  *s = '\0';
```
}

if (l == 1) *s++ = t; / copy the last symbol */
if (len != NULL) len = dst_len; / set length of the stripped string
*/
s = '\0';
}
/ }}} */
=======================cut====================

--
Using Opera's revolutionary e-mail client: http://www.opera.com/m2/

21 years ago by Derick Rethans — view source — reply

unread

Today I checked stripslashes() function.
My version of the function if faster, more clearer and bugs free (I hope
:))

You'll have to proof that by writing testcases, for example try it with
the test cases in the current source and write new ones for things that
we don't have a test case for yet.

-PHPAPI void php_stripslashes(char *str, int *len TSRMLS_DC)
+PHPAPI void php_stripslashes(char *str, size_t *len TSRMLS_DC)

You can't just change the API call, that will break things.

regards,
Derick

21 years ago by Alexander Valyalkin — view source — reply

unread

On Tue, 8 Jun 2004 16:03:19 +0200 (CEST), Derick Rethans derick@php.net
wrote:

You'll have to proof that by writing testcases, for example try it with
the test cases in the current source and write new ones for things that
we don't have a test case for yet.

-PHPAPI void php_stripslashes(char *str, int *len TSRMLS_DC)
+PHPAPI void php_stripslashes(char *str, size_t *len TSRMLS_DC)

You can't just change the API call, that will break things.

regards,
Derick

Ok. First of all, my version of the stripslashes() solves following bugs:
#9437
#19947
#27848

I have successfully tested my code with test case from current source
/ext/standard/tests/strings/add-and-stripslashes.phpt

And here is my testcase, related to mentioned bugs:

<?php
echo "Stripslashes test: ";
ini_set('magic_quotes_sybase', 0);
$s = 'c:\windows\system32';
$s1 = '\';
if ($s == stripslashes($s) && $s1 == stripslashes($s1)) echo "OK\n";
else echo "FAILED\n";
?>

Here is corrected code with standart API call:
====================cut======================
PHPAPI void php_stripslashes(char *str, int *len TSRMLS_DC)
{
char *s, t;
size_t l;
size_t dst_len; / length of the stripped string */

 if (len != NULL) l = dst_len = (size_t) *len;
 else l = dst_len = strlen(str);
 if (l < 2) return; /* there is no characters to strip */
 s = t = str;

 if (PG(magic_quotes_sybase)) { /* sybase magic_quotes ( '' -> ', \0 ->

NULL) */
while (l > 1) {
if (*t == ''' && *(t + 1) == ''') {
*s++ = ''';
t += 2;
l--;
dst_len--;
} else if (*t == '\' && *(t + 1) == '0') {
*s++ = '\0';
t += 2;
l--;
dst_len--;
} else *s++ = t++;
l--;
}
} else { / ordinary magic_qoutes (not sybase) ( \ -> , ' -> ', "
-> ", \0 -> NULL) */
while (l > 1) {
if (*t == '\') {
t++;
switch (*t) {
case '\' :
case ''' :
case '"' :
*s++ = *t++; dst_len--; break;
case '0' : *s++ = '\0'; t++; dst_len--; break;
default : *s++ = '\'; *s++ = *t++; break;
}
l -= 2;
} else {
*s++ = *t++;
l--;
}
}
}
if (l == 1) *s++ = t; / copy the last symbol */
if (len != NULL) len = (int) dst_len; / set length of the stripped
string */
*s = '\0';
}
====================cut======================

diff:
====================cut======================
--- string.c Thu May 13 20:44:32 2004
+++ string_new.c Wed Jun 09 13:47:21 2004
@@ -2159,70 +2159,51 @@
PHPAPI void php_stripslashes(char *str, int *len TSRMLS_DC)
{
char *s, *t;

int l;
if (len != NULL) {
```
  l = *len;
```
} else {
```
  l = strlen(str);
```
}
s = str;
t = str;

size_t l;
size_t dst_len; /* length of the stripped string */

if (PG(magic_quotes_sybase)) {
```
  while (l > 0) {
```
```
      if (*t == '\'') {
```

          if ((l > 0) && (t[1] == '\'')) {

```
              t++;
```
```
              if (len != NULL)
```
```
                  (*len)--;
```

if (len != NULL) l = dst_len = (size_t) *len;
else l = dst_len = strlen(str);
if (l < 2) return; /* there is no characters to strip */
s = t = str;
if (PG(magic_quotes_sybase)) { /* sybase magic_quotes ( '' -> ', \0
-> NULL) */
```
   while (l > 1) {
```

       if (*t == '\'' && *(t + 1) == '\'') {

```
           *s++ = '\'';
```
```
           t += 2;
               l--;
```

```
          }
```
```
          *s++ = *t++;
```

      } else if (*t == '\\' && l > 0 && t[1] == '0') {

```
           dst_len--;
```

       } else if (*t == '\\' && *(t + 1) == '0') {
               *s++='\0';
               t += 2;

```
              if (len != NULL)
```

                  (*len)--;
               l--;

```
      } else {
```
```
          *s++ = *t++;
```
```
      }
```

```
           dst_len--;
```

       } else *s++ = *t++;
       l--;
   }

```
  *s = '\0';
```
```
  return;
```
}
while (l > 0) {

} else { /* ordinary magic_qoutes (not sybase) ( \ -> , ' -> ', "
-> ", \0 -> NULL) */

   while (l > 1) {
   if (*t == '\\') {

      t++;                /* skip the slash */

```
      if (len != NULL)
```
```
          (*len)--;
```
```
      l--;
```
```
      if (l > 0) {
```
```
          if (*t == '0') {
```

              *s++='\0';
               t++;

```
          } else {
```

              *s++ = *t++;    /* preserve the next character */

```
          }
```
```
          l--;
```

```
           switch (*t) {
```
```
               case '\\' :
```
```
               case '\'' :
```
```
               case '"' :
```

                   *s++ = *t++; dst_len--; break;

               case '0' : *s++ = '\0'; t++; dst_len--; break;

               default : *s++ = '\\'; *s++ = *t++; break;
       }

```
           l -= 2;
   } else {
```

      if (s != t) {
           *s++ = *t++;

```
      } else {
```
```
          s++;
```
```
          t++;
```
```
      }
       l--;
   }
```
}
if (s != t) {
```
  *s = '\0';
```
}

if (l == 1) *s++ = t; / copy the last symbol */
if (len != NULL) len = (int) dst_len; / set length of the stripped
string */
s = '\0';
}
/ }}} */
====================cut======================
--
Using Opera's revolutionary e-mail client: http://www.opera.com/m2/

21 years ago by Antony Dovgal — view source — reply

unread

On Wed, 09 Jun 2004 13:52:42 +0300
"Alexander Valyalkin" valyala@tut.by wrote:

On Tue, 8 Jun 2004 16:03:19 +0200 (CEST), Derick Rethans
derick@php.net wrote:
Ok. First of all, my version of the stripslashes() solves following
bugs:
#9437
#19947
#27848

All these bugs were marked as 'bogus' (yeah, the first one is bogus too,
dunno why it's 'closed').
So, personally I can't understand what are you trying to fix, if nothing
is broken.

WBR,
Antony Dovgal aka tony2001
tony2001@phpclub.net || antony@dovgal.com

21 years ago by Alexander Valyalkin — view source — reply

unread

On Wed, 9 Jun 2004 15:00:30 +0400, Antony Dovgal tony2001@phpclub.net
wrote:

On Wed, 09 Jun 2004 13:52:42 +0300
"Alexander Valyalkin" valyala@tut.by wrote:

On Tue, 8 Jun 2004 16:03:19 +0200 (CEST), Derick Rethans
derick@php.net wrote:
Ok. First of all, my version of the stripslashes() solves following
bugs:
#9437
#19947
#27848

All these bugs were marked as 'bogus' (yeah, the first one is bogus too,
dunno why it's 'closed').
So, personally I can't understand what are you trying to fix, if nothing
is broken.

Read CLOSELY mentioned bug reports. Dont pay attention to the 'Closed'
status of the bugs. It is wrong IHMO.

--
Using Opera's revolutionary e-mail client: http://www.opera.com/m2/

21 years ago by Antony Dovgal — view source — reply

unread

On Wed, 09 Jun 2004 14:17:33 +0300
"Alexander Valyalkin" valyala@tut.by wrote:

On Wed, 9 Jun 2004 15:00:30 +0400, Antony Dovgal
tony2001@phpclub.net wrote:

On Wed, 09 Jun 2004 13:52:42 +0300
"Alexander Valyalkin" valyala@tut.by wrote:

On Tue, 8 Jun 2004 16:03:19 +0200 (CEST), Derick Rethans
derick@php.net wrote:
Ok. First of all, my version of the stripslashes() solves following
bugs:
#9437
#19947
#27848

All these bugs were marked as 'bogus' (yeah, the first one is bogus
too, dunno why it's 'closed').
So, personally I can't understand what are you trying to fix, if
nothing is broken.

Read CLOSELY mentioned bug reports. Dont pay attention to the 'Closed'
status of the bugs. It is wrong IHMO.

If you think it's wrong - prove it.
There are quite detailed reasons why those bugs were considered to
be bogus.

I'm ok if you're going to improve stripslashes() efficiency and/or to
make it just faster, this could be very useful addon. But I doubt
it needs some kind of'fixing', 'cos it works ok ATM, as I know.

WBR,
Antony Dovgal aka tony2001
tony2001@phpclub.net || antony@dovgal.com

21 years ago by Alexander Valyalkin — view source — reply

unread

On Wed, 9 Jun 2004 15:56:10 +0400, Antony Dovgal tony2001@phpclub.net
wrote:

If you think it's wrong - prove it.
There are quite detailed reasons why those bugs were considered to
be bogus.

I'm ok if you're going to improve stripslashes() efficiency and/or to
make it just faster, this could be very useful addon. But I doubt
it needs some kind of'fixing', 'cos it works ok ATM, as I know.

WBR,
Antony Dovgal aka tony2001
tony2001@phpclub.net || antony@dovgal.com

Ok, my version of stripslashes() is faster, clearer, works correctly with
old
tests and solves mentioned bugs. Are you still doubt? Try to compare the
old
(current) code to new one.

Here is improved version of my code. Unnesessary [dst_len] variable has
been
removed.
================cut===============
PHPAPI void php_stripslashes(char *str, int *len TSRMLS_DC)
{
char *s, *t;
size_t l;

 if (len != NULL) l = (size_t) *len;
 else l = strlen(str);
 if (l < 2) return; /* there is no characters to strip */
 s = t = str;

 if (PG(magic_quotes_sybase)) { /* sybase magic_quotes ( '' -> ', \0 ->

NULL) */
while (l > 1) {
if (*t == ''' && *(t + 1) == ''') {
*s++ = ''';
t += 2;
l--;
} else if (*t == '\' && *(t + 1) == '0') {
*s++ = '\0';
t += 2;
l--;
} else *s++ = t++;
l--;
}
} else { / ordinary magic_qoutes (not sybase) ( \ -> , ' -> ', "
-> ", \0 -> NULL) */
while (l > 1) {
if (*t == '\') {
t++;
switch (*t) {
case '\' :
case ''' :
case '"' :
*s++ = *t++; break;
case '0' : *s++ = '\0'; t++; break;
default : *s++ = '\'; *s++ = *t++; break;
}
l -= 2;
} else {
*s++ = *t++;
l--;
}
}
}
if (l == 1) *s++ = t; / copy the last symbol */
if (len != NULL) len = (int) (s - str); / set length of the stripped
string */
*s = '\0';
}
================cut===============

unified diff:
================cut===============
--- string.c Thu May 13 20:44:32 2004
+++ string_new.c Thu Jun 10 10:40:58 2004
@@ -2159,70 +2159,48 @@
PHPAPI void php_stripslashes(char *str, int *len TSRMLS_DC)
{
char *s, *t;

int l;
if (len != NULL) {
```
  l = *len;
```
} else {
```
  l = strlen(str);
```
}
s = str;
t = str;

size_t l;

if (PG(magic_quotes_sybase)) {
```
  while (l > 0) {
```
```
      if (*t == '\'') {
```

          if ((l > 0) && (t[1] == '\'')) {

```
              t++;
```
```
              if (len != NULL)
```
```
                  (*len)--;
```

if (len != NULL) l = (size_t) *len;
else l = strlen(str);
if (l < 2) return; /* there is no characters to strip */
s = t = str;
if (PG(magic_quotes_sybase)) { /* sybase magic_quotes ( '' -> ', \0
-> NULL) */
```
   while (l > 1) {
```

       if (*t == '\'' && *(t + 1) == '\'') {

```
           *s++ = '\'';
```
```
           t += 2;
               l--;
```

```
          }
```
```
          *s++ = *t++;
```

      } else if (*t == '\\' && l > 0 && t[1] == '0') {

       } else if (*t == '\\' && *(t + 1) == '0') {
               *s++='\0';
               t += 2;

```
              if (len != NULL)
```

                  (*len)--;
               l--;

```
      } else {
```
```
          *s++ = *t++;
```
```
      }
```

       } else *s++ = *t++;
       l--;
   }

```
  *s = '\0';
```
```
  return;
```
}
while (l > 0) {

} else { /* ordinary magic_qoutes (not sybase) ( \ -> , ' -> ', "
-> ", \0 -> NULL) */

   while (l > 1) {
   if (*t == '\\') {

      t++;                /* skip the slash */

```
      if (len != NULL)
```
```
          (*len)--;
```
```
      l--;
```
```
      if (l > 0) {
```
```
          if (*t == '0') {
```

              *s++='\0';
               t++;

```
          } else {
```

              *s++ = *t++;    /* preserve the next character */

```
          }
```
```
          l--;
```

```
           switch (*t) {
```
```
               case '\\' :
```
```
               case '\'' :
```
```
               case '"' :
```
```
                   *s++ = *t++; break;
```

               case '0' : *s++ = '\0'; t++; break;

               default : *s++ = '\\'; *s++ = *t++; break;
       }

```
           l -= 2;
   } else {
```

      if (s != t) {
           *s++ = *t++;

```
      } else {
```
```
          s++;
```
```
          t++;
```
```
      }
       l--;
   }
```
}
if (s != t) {
```
  *s = '\0';
```
}

if (l == 1) *s++ = t; / copy the last symbol */
if (len != NULL) len = (int) (s - str); / set length of the
stripped string */
s = '\0';
}
/ }}} */

================cut===============

--
Using Opera's revolutionary e-mail client: http://www.opera.com/m2/

21 years ago by Antony Dovgal — view source — reply

unread

On Thu, 10 Jun 2004 10:50:09 +0300
"Alexander Valyalkin" valyala@tut.by wrote:

On Wed, 9 Jun 2004 15:56:10 +0400, Antony Dovgal
tony2001@phpclub.net wrote:

If you think it's wrong - prove it.
There are quite detailed reasons why those bugs were considered to
be bogus.

I'm ok if you're going to improve stripslashes() efficiency and/or
to make it just faster, this could be very useful addon. But I doubt
it needs some kind of'fixing', 'cos it works ok ATM, as I know.

Ok, my version of stripslashes() is faster, clearer, works correctly
with old
tests and solves mentioned bugs. Are you still doubt? Try to compare
the old
(current) code to new one.

Faster is ok, but you'd better read what Rasmus said.
And again, trying to fix BOGUS bugs you're going wrong way.

WBR,
Antony Dovgal aka tony2001
tony2001@phpclub.net || antony@dovgal.com

21 years ago by Andre Cerqueira — view source — reply

unread

The 2 first bugs where really bogus, but imho the last one did make sense.

If stripslashes does "Un-quote string quoted with addslashes()", how
addslashes would come up with:

c:\windows\system32
from:
c:windowssystem32
?

Maybe its to late for a change, now that most people are used to this
behavior, but im still not convinced that it was really supposed to be
this way (can anyone explain "why"?).

I noticed 3 different thoughts:

a) stripslashes() were meant to strip all slashes
This confused me, probably i misinterpreted...
Wouldnt it be like calling strtr($string, '\', '')?

b) stripslashes() were meant to strip all slashes but the ones followed
by a slash (if any)
This is how it works now
Though it makes (stripslashes(addslashes(string)) == string) true, it
doesnt make (addslashes(stripslashes(string)) == string) necessarily true.

c) stripslashes() were meant to strip all slashes added by addslashes
This is what the manual says.

At bug #27848, iliaa gave a "proof why it works" assuming stripslashes
will only be called after addslashes.
That would make the third bug bogus, ok, but probably Alexander
Valyalkin would be right also, and it wouldnt be a problem to do things
his way.

I probably dont know as much as anyone here, this is just what i understood.
If im wrong, can someone explain where and why?

Antony Dovgal wrote:

On Thu, 10 Jun 2004 10:50:09 +0300
"Alexander Valyalkin" valyala@tut.by wrote:

On Wed, 9 Jun 2004 15:56:10 +0400, Antony Dovgal
tony2001@phpclub.net wrote:

If you think it's wrong - prove it.
There are quite detailed reasons why those bugs were considered to
be bogus.

I'm ok if you're going to improve stripslashes() efficiency and/or
to make it just faster, this could be very useful addon. But I doubt
it needs some kind of'fixing', 'cos it works ok ATM, as I know.

Ok, my version of stripslashes() is faster, clearer, works correctly
with old
tests and solves mentioned bugs. Are you still doubt? Try to compare
the old
(current) code to new one.

Faster is ok, but you'd better read what Rasmus said.
And again, trying to fix BOGUS bugs you're going wrong way.

WBR,
Antony Dovgal aka tony2001
tony2001@phpclub.net || antony@dovgal.com

21 years ago by Rasmus Lerdorf — view source — reply

unread

The 2 first bugs where really bogus, but imho the last one did make sense.

If stripslashes does "Un-quote string quoted with addslashes()", how
addslashes would come up with:

c:\windows\system32
from:
c:windowssystem32

It can't! How would it? Nobody has ever stated that addslashes() can
undo a stripslashes() call. That's like expecting there to be an untrim()
call to match trim(), for example.

a) stripslashes() were meant to strip all slashes
This confused me, probably i misinterpreted...
Wouldnt it be like calling strtr($string, '\', '')?

Not quite, stripslashes() understands the sybase style of escaping ('')
and it is faster than strtr for that case.

b) stripslashes() were meant to strip all slashes but the ones followed
by a slash (if any)
This is how it works now
Though it makes (stripslashes(addslashes(string)) == string) true, it
doesnt make (addslashes(stripslashes(string)) == string) necessarily true.

Correct. The goal of stripslashes() was simply to get rid of the set of
slashes added by magic_quotes for display purposes and it achieves this
quite nicely.

c) stripslashes() were meant to strip all slashes added by addslashes
This is what the manual says.

And it does. But the manual doesn't say that it only removes slashes
added by addslashes.

-Rasmus

21 years ago by Stefan Esser — view source — reply

unread

Hi,

this is not directly releated. I looked into addslashes yesterday and
thought that it would be much better to scan the string 2 times. The
first time to find out how much extra bytes one needs, and the second
time to do the actual replacing. So we do not need to allocate and
oversized block and do not need any kind of realloc. This realloc is an
absolute performance killer.

My patch, which has to be cleaned had the following benchmark results

210000 x addslashes() on different strings (7 alternating strings)

PHP5 RC3 - standard 12,x seconds
PHP5 RC3 - my patch 7,6 seconds

The performance boost was about 4,5 seconds which is more than 33%.

Stefan Esser

21 years ago by Rasmus Lerdorf — view source — reply

unread

this is not directly releated. I looked into addslashes yesterday and
thought that it would be much better to scan the string 2 times. The
first time to find out how much extra bytes one needs, and the second
time to do the actual replacing. So we do not need to allocate and
oversized block and do not need any kind of realloc. This realloc is an
absolute performance killer.

Yeah, that's a good idea.

-Rasmus

21 years ago by Andre Cerqueira — view source — reply

unread

But the manual doesn't say that it only removes slashes
added by addslashes.

"Returns a string with backslashes stripped off. (' becomes ' and so
on.) Double backslashes (\) are made into a single backslash ()."

Yes, it doesn't say, but i think anyone could see that implied if one
didn't have previous knowledge of what it really does.

I understand it works exactly how its supposed to, but why is it
supposed to work this way?
Inspired in something from another project?
Probably what I'm thinking as the right behavior, is what someone that
hasn't put enough thought on it would think hehe
To sum up: why was the real way choosen?

Rasmus Lerdorf wrote:

The 2 first bugs where really bogus, but imho the last one did make sense.

If stripslashes does "Un-quote string quoted with addslashes()", how
addslashes would come up with:

c:\windows\system32
from:
c:windowssystem32

It can't! How would it? Nobody has ever stated that addslashes() can
undo a stripslashes() call. That's like expecting there to be an untrim()
call to match trim(), for example.

a) stripslashes() were meant to strip all slashes
This confused me, probably i misinterpreted...
Wouldnt it be like calling strtr($string, '\', '')?

Not quite, stripslashes() understands the sybase style of escaping ('')
and it is faster than strtr for that case.

b) stripslashes() were meant to strip all slashes but the ones followed
by a slash (if any)
This is how it works now
Though it makes (stripslashes(addslashes(string)) == string) true, it
doesnt make (addslashes(stripslashes(string)) == string) necessarily true.

Correct. The goal of stripslashes() was simply to get rid of the set of
slashes added by magic_quotes for display purposes and it achieves this
quite nicely.

c) stripslashes() were meant to strip all slashes added by addslashes
This is what the manual says.

And it does. But the manual doesn't say that it only removes slashes
added by addslashes.

-Rasmus

21 years ago by Rasmus Lerdorf — view source — reply

unread

But the manual doesn't say that it only removes slashes
added by addslashes.

"Returns a string with backslashes stripped off. (' becomes ' and so
on.) Double backslashes (\) are made into a single backslash ()."

Yes, it doesn't say, but i think anyone could see that implied if one
didn't have previous knowledge of what it really does.

I understand it works exactly how its supposed to, but why is it
supposed to work this way?
Inspired in something from another project?
Probably what I'm thinking as the right behavior, is what someone that
hasn't put enough thought on it would think hehe
To sum up: why was the real way choosen?

I saw no real-world reason not to do it this way. Functions that trim or
strip stuff should trim or strip that stuff. If you are stripping tags,
you strip tags. If you are stripping slashes, you strip slashes. If you
want a function that only strips slashes in a certain context, then it
should be called something like strip_context_slashes. Like
strip_sql_slashes.

And before you start arguing that addslashes breaks that rule, it is
pretty clear that anything that adds has to have some sort of context
associated with it.

-Rasmus

21 years ago by Andre Cerqueira — view source — reply

unread

Thanks for replying

Understood, but do you agree with me that the manual have a better
chance to give a wrong idea than the right one on this?
And that is giving me second thoughts about how bad it would be to
change stripslashes.

I think the manual could imply 2 things:
a) stripslashes should only be used with results of addslashes
For un-quoting addslashes: True
...and any other use would not make sense: True?

b) stripslashes removes only slashes that addslashes could have put
False.

If the first is true, Alexander way would be true to both, just not
compatible with scripts that used stripslashes with strings that didn't
went through addslashes.
Would you use it for any other reason than stripping added slashes?
If yes, there is no possibility of changing it; If no, why bother change it?

Now i notice why this discussion is pointless.
Sorry, but thanks

Rasmus Lerdorf wrote:

But the manual doesn't say that it only removes slashes
added by addslashes.

"Returns a string with backslashes stripped off. (' becomes ' and so
on.) Double backslashes (\) are made into a single backslash ()."

Yes, it doesn't say, but i think anyone could see that implied if one
didn't have previous knowledge of what it really does.

I understand it works exactly how its supposed to, but why is it
supposed to work this way?
Inspired in something from another project?
Probably what I'm thinking as the right behavior, is what someone that
hasn't put enough thought on it would think hehe
To sum up: why was the real way choosen?

I saw no real-world reason not to do it this way. Functions that trim or
strip stuff should trim or strip that stuff. If you are stripping tags,
you strip tags. If you are stripping slashes, you strip slashes. If you
want a function that only strips slashes in a certain context, then it
should be called something like strip_context_slashes. Like
strip_sql_slashes.

And before you start arguing that addslashes breaks that rule, it is
pretty clear that anything that adds has to have some sort of context
associated with it.

-Rasmus

21 years ago by Alexander Valyalkin — view source — reply

unread

On Thu, 10 Jun 2004 11:56:26 +0400, Antony Dovgal tony2001@phpclub.net
wrote:

Ok, my version of stripslashes() is faster, clearer, works correctly
with old
tests and solves mentioned bugs. Are you still doubt? Try to compare
the old
(current) code to new one.

Faster is ok, but you'd better read what Rasmus said.
And again, trying to fix BOGUS bugs you're going wrong way.

I always thought that fixing ANY (not only critical) bugs is
right way...

--
Using Opera's revolutionary e-mail client: http://www.opera.com/m2/

21 years ago by Derick Rethans — view source — reply

unread

On Thu, 10 Jun 2004 11:56:26 +0400, Antony Dovgal tony2001@phpclub.net
wrote:

Ok, my version of stripslashes() is faster, clearer, works correctly
with old
tests and solves mentioned bugs. Are you still doubt? Try to compare
the old
(current) code to new one.

Faster is ok, but you'd better read what Rasmus said.
And again, trying to fix BOGUS bugs you're going wrong way.

I always thought that fixing ANY (not only critical) bugs is
right way...

The meaning of BOGUS means clearly that it "i s n o t a b u g" so
you can't fix a bug that is no bug.

Derick

21 years ago by moshe doron — view source — reply

unread

Ok. First of all, my version of the stripslashes() solves following
bugs:
#9437
#19947
#27848

All these bugs were marked as 'bogus' (yeah, the first one is bogus too,
dunno why it's 'closed').
So, personally I can't understand what are you trying to fix, if nothing
is broken.

in addition, i want to remind that when magic mode is on, this function been
used by each script to manipulate the $_REQUEST vars so playing with this
peace of code have

big codes breaking potential.
big security problems potential.

--/moshe.

WBR,
Antony Dovgal aka tony2001
tony2001@phpclub.net || antony@dovgal.com

21 years ago by Alexander Valyalkin — view source — reply

unread

On Wed, 9 Jun 2004 14:20:01 +0200, Moshe Doron mosdoron@netvision.net.il
wrote:

in addition, i want to remind that when magic mode is on, this function
been
used by each script to manipulate the $_REQUEST vars so playing with this
peace of code have

big codes breaking potential.

big security problems potential.

--/moshe.

I agree with you, but anyone here can audit my code for code breaking and
security problems.

--
Using Opera's revolutionary e-mail client: http://www.opera.com/m2/

21 years ago by Jevon Wright — view source — reply

unread

<?php
echo "Stripslashes test: ";
ini_set('magic_quotes_sybase', 0);
$s = 'c:\windows\system32';
$s1 = '\';
if ($s == stripslashes($s) && $s1 == stripslashes($s1)) echo "OK\n";
else echo "FAILED\n";
?>

'c:\windows\system32' becomes 'c:\windows\system32' in memory (the slashes
are stripped).
stripslashes() will make it 'c:windowssystem32'. Thus the above tests will
fail.
This isn't a bug? (Doesn't magic_quotes_sybase only override
magic_quotes_gpc which affects GPC data?)

Jevon

----- Original Message -----
From: "Alexander Valyalkin" valyala@tut.by
To: internals@lists.php.net
Sent: Wednesday, June 09, 2004 10:52 PM
Subject: Re: [PHP-DEV] stripslashes() improvements

On Tue, 8 Jun 2004 16:03:19 +0200 (CEST), Derick Rethans derick@php.net
wrote:

You'll have to proof that by writing testcases, for example try it with
the test cases in the current source and write new ones for things that
we don't have a test case for yet.

-PHPAPI void php_stripslashes(char *str, int *len TSRMLS_DC)
+PHPAPI void php_stripslashes(char *str, size_t *len TSRMLS_DC)

You can't just change the API call, that will break things.

regards,
Derick

Ok. First of all, my version of the stripslashes() solves following bugs:
#9437
#19947
#27848

I have successfully tested my code with test case from current source
/ext/standard/tests/strings/add-and-stripslashes.phpt

And here is my testcase, related to mentioned bugs:

<?php
echo "Stripslashes test: ";
ini_set('magic_quotes_sybase', 0);
$s = 'c:\windows\system32';
$s1 = '\';
if ($s == stripslashes($s) && $s1 == stripslashes($s1)) echo "OK\n";
else echo "FAILED\n";
?>

Here is corrected code with standart API call:
====================cut======================
PHPAPI void php_stripslashes(char *str, int *len TSRMLS_DC)
{
char *s, t;
size_t l;
size_t dst_len; / length of the stripped string */
 if (len != NULL) l = dst_len = (size_t) *len;
 else l = dst_len = strlen(str);
 if (l < 2) return; /* there is no characters to strip */
 s = t = str;

 if (PG(magic_quotes_sybase)) { /* sybase magic_quotes ( '' -> ',

\0 ->

NULL) */
while (l > 1) {
if (*t == ''' && *(t + 1) == ''') {
*s++ = ''';
t += 2;
l--;
dst_len--;
} else if (*t == '\' && *(t + 1) == '0') {
*s++ = '\0';
t += 2;
l--;
dst_len--;
} else *s++ = t++;
l--;
}
} else { / ordinary magic_qoutes (not sybase) ( \ -> , ' -> ', "
-> ", \0 -> NULL) */
while (l > 1) {
if (*t == '\') {
t++;
switch (*t) {
case '\' :
case ''' :
case '"' :
*s++ = *t++; dst_len--; break;
case '0' : *s++ = '\0'; t++; dst_len--; break;
default : *s++ = '\'; *s++ = *t++; break;
}
l -= 2;
} else {
*s++ = *t++;
l--;
}
}
}
if (l == 1) *s++ = t; / copy the last symbol */
if (len != NULL) len = (int) dst_len; / set length of the stripped
string */
*s = '\0';
}
====================cut======================

diff:
====================cut======================
--- string.c Thu May 13 20:44:32 2004
+++ string_new.c Wed Jun 09 13:47:21 2004
@@ -2159,70 +2159,51 @@
PHPAPI void php_stripslashes(char *str, int *len TSRMLS_DC)
{
char *s, *t;
int l;

if (len != NULL) {
  l = *len;
} else {
  l = strlen(str);
}

s = str;

t = str;
size_t l;

size_t dst_len; /* length of the stripped string */
if (PG(magic_quotes_sybase)) {
  while (l > 0) {
      if (*t == '\'') {
          if ((l > 0) && (t[1] == '\'')) {
              t++;
              if (len != NULL)
                  (*len)--;
if (len != NULL) l = dst_len = (size_t) *len;

else l = dst_len = strlen(str);

if (l < 2) return; /* there is no characters to strip */

s = t = str;

if (PG(magic_quotes_sybase)) { /* sybase magic_quotes ( '' -> ', \0
-> NULL) */
   while (l > 1) {
       if (*t == '\'' && *(t + 1) == '\'') {
           *s++ = '\'';
           t += 2;
               l--;
          }
          *s++ = *t++;
      } else if (*t == '\\' && l > 0 && t[1] == '0') {
           dst_len--;
       } else if (*t == '\\' && *(t + 1) == '0') {
               *s++='\0';
               t += 2;
              if (len != NULL)
                  (*len)--;
               l--;
      } else {
          *s++ = *t++;
      }
           dst_len--;
       } else *s++ = *t++;
       l--;
   }
  *s = '\0';
  return;
}

while (l > 0) {
} else { /* ordinary magic_qoutes (not sybase) ( \ -> , ' -> ', "
-> ", \0 -> NULL) */
   while (l > 1) {
   if (*t == '\\') {
      t++;                /* skip the slash */
      if (len != NULL)
          (*len)--;
      l--;
      if (l > 0) {
          if (*t == '0') {
              *s++='\0';
               t++;
          } else {
              *s++ = *t++;    /* preserve the next character */
          }
          l--;
           switch (*t) {
               case '\\' :
               case '\'' :
               case '"' :
                   *s++ = *t++; dst_len--; break;
               case '0' : *s++ = '\0'; t++; dst_len--; break;
               default : *s++ = '\\'; *s++ = *t++; break;
       }
           l -= 2;
   } else {
      if (s != t) {
           *s++ = *t++;
      } else {
          s++;
          t++;
      }
       l--;
   }
}
if (s != t) {
  *s = '\0';
}
if (l == 1) *s++ = t; / copy the last symbol */

if (len != NULL) len = (int) dst_len; / set length of the stripped
string */

s = '\0';
}
/ }}} */
====================cut======================
--
Using Opera's revolutionary e-mail client: http://www.opera.com/m2/

21 years ago by Alexander Valyalkin — view source — reply

unread

<?php
echo "Stripslashes test: ";
ini_set('magic_quotes_sybase', 0);
$s = 'c:\windows\system32';
$s1 = '\';
if ($s == stripslashes($s) && $s1 == stripslashes($s1)) echo "OK\n";
else echo "FAILED\n";
?>

'c:\windows\system32' becomes 'c:\windows\system32' in memory (the
slashes are stripped).

I know it :)

stripslashes() will make it 'c:windowssystem32'. Thus the above tests
will fail.

Yes. The current version of stripslashes will fail on the testcase. And my
version
of stripslashes successfully passes it. Read bugreports #9437, #19947,
#27848 and
see sources of my stripslashes code to understand it behavior.

This isn't a bug? (Doesn't magic_quotes_sybase only override
magic_quotes_gpc which affects GPC data?)

Jevon

--
Using Opera's revolutionary e-mail client: http://www.opera.com/m2/

21 years ago by Rasmus Lerdorf — view source — reply

unread

Uh, what are you guys talking about? That test case is bogus.

$s = 'c:\windows\system32';

This means that the base string we are working with is c:\windows\system32
here before any sort of stripslashes. If you then call stripslashes() on
that, it will strip these slashes! That's exactly what the function is
designed to do and what it is supposed to do. If your version does not
strip these slashes, then it is not stripslashes(). stripslashes() strips
all slashes, not just slashes before ', " and \ very much by design.

I suppose it might be useful to have a version of stripslashes() which
only strips slashes if they come before ', " or \ but this function
wouldn't be stripslashes() and certainly shouldn't replace the existing
stripslashes function because doing so would break all sorts of stuff.

-Rasmus

<?php
echo "Stripslashes test: ";
ini_set('magic_quotes_sybase', 0);
$s = 'c:\windows\system32';
$s1 = '\';
if ($s == stripslashes($s) && $s1 == stripslashes($s1)) echo "OK\n";
else echo "FAILED\n";
?>

'c:\windows\system32' becomes 'c:\windows\system32' in memory (the
slashes are stripped).

I know it :)

stripslashes() will make it 'c:windowssystem32'. Thus the above tests
will fail.

Yes. The current version of stripslashes will fail on the testcase. And my
version
of stripslashes successfully passes it. Read bugreports #9437, #19947,
#27848 and
see sources of my stripslashes code to understand it behavior.

This isn't a bug? (Doesn't magic_quotes_sybase only override
magic_quotes_gpc which affects GPC data?)

Jevon

--
Using Opera's revolutionary e-mail client: http://www.opera.com/m2/

21 years ago by Alexander Valyalkin — view source — reply

unread

On Thu, 10 Jun 2004 00:47:09 -0700 (PDT), Rasmus Lerdorf rasmus@php.net
wrote:

Uh, what are you guys talking about? That test case is bogus.

$s = 'c:\windows\system32';

This means that the base string we are working with is
c:\windows\system32
here before any sort of stripslashes. If you then call stripslashes() on
that, it will strip these slashes! That's exactly what the function is
designed to do and what it is supposed to do. If your version does not
strip these slashes, then it is not stripslashes(). stripslashes()
strips
all slashes, not just slashes before ', " and \ very much by design.

I suppose it might be useful to have a version of stripslashes() which
only strips slashes if they come before ', " or \ but this function
wouldn't be stripslashes() and certainly shouldn't replace the existing
stripslashes function because doing so would break all sorts of stuff.

-Rasmus

I think it will be very useful to add optional parameter to stripslashes()
to strip only slashes which have been added by addslashes() (before ", ',
\ and NULL).

--
Using Opera's revolutionary e-mail client: http://www.opera.com/m2/