Project

General

Profile

Bug #2560

buffer_path_simplify sometimes has a wrong state internally

Added by TheJH about 3 years ago. Updated about 1 year ago.

Status:
Fixed
Priority:
Low
Assignee:
-
Category:
-
Target version:
Start date:
2014-03-05
Due date:
% Done:

100%

Missing in 1.5.x:
No

Description

buffer_path_simplify sometimes has a wrong state internally. Have a look at these code snippets:

    char c, pre1;
    [...]
    unsigned short pre;
    [...]
        pre1 = c;
        pre  = (pre << 8) | pre1;
        c    = *walk;

As you can see, the (usually) signed char pre1 is OR-ed into pre – but this causes pre1 to be sign-extended. So if the last two chars were '.' and '\xff', pre1 will contain '\xff\xff' instead of the expected '.\xff'.

This patch should fix that:

diff --git a/src/buffer.c b/src/buffer.c
index 1199164..8cc2c3d 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -923,7 +923,8 @@ int buffer_urldecode_query(buffer *url) {
 int buffer_path_simplify(buffer *dest, buffer *src)
 {
        int toklen;
- char c, pre1;
+       char c;
+       unsigned char pre1;
        char *start, *slash, *walk, *out;
        unsigned short pre;

Luckily, this does not change the behavior of this function (that's why I'm setting the priority to low).

Associated revisions

Revision 3120 (diff)
Added by stbuehler about 1 year ago

[buffer] refactor buffer_path_simplify (fixes #2560)

There actually was one bug: if the input consisted only of spaces,
it would read one byte too much.

`pre` was splitted into `pre2` and (already existing) `pre1` - the two
characters which were read before the current one in `c`.

Restructuring the loop eliminated some code before the loop, which was
similar to the one at the end of the loop.

From: Stefan Bühler <>

Revision 0a61fdec (diff)
Added by stbuehler about 1 year ago

[buffer] refactor buffer_path_simplify (fixes #2560)

There actually was one bug: if the input consisted only of spaces,
it would read one byte too much.

`pre` was splitted into `pre2` and (already existing) `pre1` - the two
characters which were read before the current one in `c`.

Restructuring the loop eliminated some code before the loop, which was
similar to the one at the end of the loop.

From: Stefan Bühler <>

git-svn-id: svn://svn.lighttpd.net/lighttpd/branches/lighttpd-1.4.x@3120 152afb58-edef-0310-8abb-c4023f1b3aa9

History

#1 Updated by stbuehler about 3 years ago

  • Target version set to 1.4.36

buffer_path_simplify is kinda ugly. Would love to fix this.. probably should add some unit testing for buffer.c before.

#2 Updated by stbuehler over 1 year ago

  • Target version changed from 1.4.36 to 1.4.37

#3 Updated by stbuehler over 1 year ago

  • Target version changed from 1.4.37 to 1.4.38

#4 Updated by stbuehler over 1 year ago

  • Target version changed from 1.4.38 to 1.4.39

#5 Updated by stbuehler about 1 year ago

  • Target version changed from 1.4.39 to 1.4.40

#6 Updated by gstrauss about 1 year ago

Can this 2-line patch be applied and this ticket closed?

#7 Updated by stbuehler about 1 year ago

If it doesn't change anything, why modify it? If something is broken I'd like to have unit tests.

#8 Updated by gstrauss about 1 year ago

The broken intermediate behavior is posted in the initial issue report above.

This "happens" not to affect the result because 'pre' is used in two places and results in a relevant match only for "/." and "..", i.e. when the lower 8 bits is '.', which is 0x2E and part of 7-bit ASCII and does not have the high bit set.

It is fortunate, but not intentional, that 'pre' "happens" to not match when 'pre' is corrupted due to sign extension and 'pre' is not actually the value it is expected to be. This should be fixed, even if the result currently happens to work out ok.

Please fix and close this ticket, or if you're still not convinced that the sloppy code should be fixed, then at least close this ticket unless there is a reason to keep it open.

#9 Updated by stbuehler about 1 year ago

I want to cleanup buffer_path_simplify, but I do not want to touch it without unit tests, as it is a really ugly piece of code, and I fear it will break even on small changes which might look harmless and good in the first place.

#10 Updated by stbuehler about 1 year ago

  • Status changed from Patch Pending to Fixed
  • % Done changed from 0 to 100

Applied in changeset r3120.

Also available in: Atom