Project

General

Profile

Actions
Copy link

Bug #2560

closed

buffer_path_simplify sometimes has a wrong state internally

Added by TheJH over 10 years ago. Updated over 8 years ago.

Status:
Fixed
Priority:
Low
Category:
-
Target version:
1.4.40
ASK QUESTIONS IN Forums:

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).

Actions
Copy link
 #1

Updated by stbuehler over 10 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.

Actions
Copy link
 #2

Updated by stbuehler over 9 years ago

  • Target version changed from 1.4.36 to 1.4.37
Actions
Copy link
 #3

Updated by stbuehler about 9 years ago

  • Target version changed from 1.4.37 to 1.4.38
Actions
Copy link
 #4

Updated by stbuehler about 9 years ago

  • Target version changed from 1.4.38 to 1.4.39
Actions
Copy link
 #5

Updated by stbuehler almost 9 years ago

  • Target version changed from 1.4.39 to 1.4.40
Actions
Copy link
 #6

Updated by gstrauss over 8 years ago

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

Actions
Copy link
 #7

Updated by stbuehler over 8 years ago

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

Actions
Copy link
 #8

Updated by gstrauss over 8 years 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.

Actions
Copy link
 #9

Updated by stbuehler over 8 years 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.

Actions
Copy link
 #10

Updated by stbuehler over 8 years ago

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

Applied in changeset r3120.

Actions
Copy link

Also available in: Atom