Board index » delphi » Indy TIdHTTP.DoRequest - ProcessPath

Indy TIdHTTP.DoRequest - ProcessPath

In the ProcessPath procedure there is an infinite loop on a path given like
the following:

/nowsmforms/../../images/

The first pass chunks the path down to:   /../images/
The subsequent passes don't do anything since:

i = 3 after i := Pos('./', APath);
while loop doesn't do anything (APath[i-1] = '.')
i = 0 after i := RPos('/', APath, i-3);  //Starts from char 0 so does
nothing
delete(APath, i, Pos('../', APath) + 2 - i) = delete('/../images/', 0, 2 +
2 - 0)

unfortunately Delete with a 0 index doesn't work??

Comments from System.pas  _Delete:
{       if index not in [1 .. Length(s)] do nothing     }

Does this work differently in different Delphi versions?  I'm using D5
Enterprise Update Pack 1.

Regardless, infinite loops are generally bad and there is one here.

If it is supposed to be akin to Pos then RPos should not return a 0 as a
possible return value.  An easy way to fix this is to change the loop from:

for i := LStartPos downto 1 do begin

to:

for i := LStartPos downto 0 do begin

and set result := i + 1;

I'm not sure about the sideeffects, but I'll try it with a bunch of urls and
let you know.

-Brian

ProcessPath code:

  procedure ProcessPath(Var APath: String);
  Var
    i: Integer;
  begin
    if Length(APath) = 0 then begin
      APath := FURI.Path;
    end
    else begin
      if APath[1] <> '/' then begin
        APath := FURI.Path + APath;
      end;
      repeat
        i := Pos('./', APath);
        if i = 0 then begin
          break;
        end;
        while (i > 1) and (APath[i - 1] <> '.') do begin
          Delete(APath, i, 2);
          i := Pos('./', APath);
        end;
        if i = 0 then begin
          break;
        end;

        i := RPos('/', APath, i - 3);
        delete(APath, i, Pos('../', APath) + 2 - i);
      until false;
    end;

    FURI.Path := APath;
  end;

 

Re:Indy TIdHTTP.DoRequest - ProcessPath


Ok...my first attempt at a fix doesn't really work.  RPos is used in many
other places that are affected...namely in the IdURI unit.  It really is an
odd function, if it returns 0 you don't know if it is successful or
not...ugly.

So I'll just hack something in the ProcessPath function for now.  Anyone
from the Indy team care to comment/advise?

Thanks,
Brian

Quote
"Brian Moelk" <bmo...@brainendeavor.com> wrote in message

news:3b672dfa$1_1@dnews...
Quote
> In the ProcessPath procedure there is an infinite loop on a path given
like
> the following:

> /nowsmforms/../../images/

> The first pass chunks the path down to:   /../images/
> The subsequent passes don't do anything since:

> i = 3 after i := Pos('./', APath);
> while loop doesn't do anything (APath[i-1] = '.')
> i = 0 after i := RPos('/', APath, i-3);  //Starts from char 0 so does
> nothing
> delete(APath, i, Pos('../', APath) + 2 - i) = delete('/../images/', 0, 2 +
> 2 - 0)

> unfortunately Delete with a 0 index doesn't work??

> Comments from System.pas  _Delete:
> {       if index not in [1 .. Length(s)] do nothing     }

> Does this work differently in different Delphi versions?  I'm using D5
> Enterprise Update Pack 1.

> Regardless, infinite loops are generally bad and there is one here.

> If it is supposed to be akin to Pos then RPos should not return a 0 as a
> possible return value.  An easy way to fix this is to change the loop
from:

> for i := LStartPos downto 1 do begin

> to:

> for i := LStartPos downto 0 do begin

> and set result := i + 1;

> I'm not sure about the sideeffects, but I'll try it with a bunch of urls
and
> let you know.

> -Brian

> ProcessPath code:

>   procedure ProcessPath(Var APath: String);
>   Var
>     i: Integer;
>   begin
>     if Length(APath) = 0 then begin
>       APath := FURI.Path;
>     end
>     else begin
>       if APath[1] <> '/' then begin
>         APath := FURI.Path + APath;
>       end;
>       repeat
>         i := Pos('./', APath);
>         if i = 0 then begin
>           break;
>         end;
>         while (i > 1) and (APath[i - 1] <> '.') do begin
>           Delete(APath, i, 2);
>           i := Pos('./', APath);
>         end;
>         if i = 0 then begin
>           break;
>         end;

>         i := RPos('/', APath, i - 3);
>         delete(APath, i, Pos('../', APath) + 2 - i);
>       until false;
>     end;

>     FURI.Path := APath;
>   end;

Re:Indy TIdHTTP.DoRequest - ProcessPath


I'm not sure that such path is possible except in the case when you are
trying to go downin into the servers directory structoure which is not
accessible by default. Becouse this is an incorrect path this couses that
infinite loop.

Doychin

Quote
"Brian Moelk" <bmo...@brainendeavor.com> wrote in message

news:3b673967$1_2@dnews...
Quote
> Ok...my first attempt at a fix doesn't really work.  RPos is used in many
> other places that are affected...namely in the IdURI unit.  It really is
an
> odd function, if it returns 0 you don't know if it is successful or
> not...ugly.

> So I'll just hack something in the ProcessPath function for now.  Anyone
> from the Indy team care to comment/advise?

> Thanks,
> Brian

> "Brian Moelk" <bmo...@brainendeavor.com> wrote in message
> news:3b672dfa$1_1@dnews...
> > In the ProcessPath procedure there is an infinite loop on a path given
> like
> > the following:

> > /nowsmforms/../../images/

> > The first pass chunks the path down to:   /../images/
> > The subsequent passes don't do anything since:

> > i = 3 after i := Pos('./', APath);
> > while loop doesn't do anything (APath[i-1] = '.')
> > i = 0 after i := RPos('/', APath, i-3);  //Starts from char 0 so does
> > nothing
> > delete(APath, i, Pos('../', APath) + 2 - i) = delete('/../images/', 0, 2
+
> > 2 - 0)

> > unfortunately Delete with a 0 index doesn't work??

> > Comments from System.pas  _Delete:
> > {       if index not in [1 .. Length(s)] do nothing     }

> > Does this work differently in different Delphi versions?  I'm using D5
> > Enterprise Update Pack 1.

> > Regardless, infinite loops are generally bad and there is one here.

> > If it is supposed to be akin to Pos then RPos should not return a 0 as a
> > possible return value.  An easy way to fix this is to change the loop
> from:

> > for i := LStartPos downto 1 do begin

> > to:

> > for i := LStartPos downto 0 do begin

> > and set result := i + 1;

> > I'm not sure about the sideeffects, but I'll try it with a bunch of urls
> and
> > let you know.

> > -Brian

> > ProcessPath code:

> >   procedure ProcessPath(Var APath: String);
> >   Var
> >     i: Integer;
> >   begin
> >     if Length(APath) = 0 then begin
> >       APath := FURI.Path;
> >     end
> >     else begin
> >       if APath[1] <> '/' then begin
> >         APath := FURI.Path + APath;
> >       end;
> >       repeat
> >         i := Pos('./', APath);
> >         if i = 0 then begin
> >           break;
> >         end;
> >         while (i > 1) and (APath[i - 1] <> '.') do begin
> >           Delete(APath, i, 2);
> >           i := Pos('./', APath);
> >         end;
> >         if i = 0 then begin
> >           break;
> >         end;

> >         i := RPos('/', APath, i - 3);
> >         delete(APath, i, Pos('../', APath) + 2 - i);
> >       until false;
> >     end;

> >     FURI.Path := APath;
> >   end;

Re:Indy TIdHTTP.DoRequest - ProcessPath


Well....there are several reasons why this should be fixed:

1) that particular input is possible, it actually happened that's why I
posted it
2) you should code defensively
3) the function intends to fix up relative paths so it should be able to
deal with them
4) the function should not have an infinite loop based on *any* string
input; since it takes a string and does no validation of the input
5) it's a bug that eats cpu time and causes things to hang

Programmers can't make excuses for code that is buggy; it either works or it
doesn't.  Programmers should step up and take responsibility --- fix bugs
even if it's not their code, handle garbage input, compensate for their
users intellectual ability, whatever....it really doesn't matter as long as
the code works.

When it comes to fixing it or not, it's really a no brainer.  Fix it.

Like I said,  I hacked a fix in the process path function.  I added before
the delete:

if (i = 0) then
begin
    Inc(i);
end;

IMO, this is not the best way to fix it, I just didn't have the time (right
now) to go through the repercussions of fixing the RPos function.  RPos
should work as Pos does, return 0 if not found, etc.  It would have helped
if Delete was consistent with Copy, but oh well.

-Brian

Quote
"Doychin Bondzhev" <doyc...@dsoft-bg.com> wrote in message

news:3b67aa08_1@dnews...
Quote
> I'm not sure that such path is possible except in the case when you are
> trying to go downin into the servers directory structoure which is not
> accessible by default. Becouse this is an incorrect path this couses that
> infinite loop.

> Doychin

> "Brian Moelk" <bmo...@brainendeavor.com> wrote in message
> news:3b673967$1_2@dnews...
> > Ok...my first attempt at a fix doesn't really work.  RPos is used in
many
> > other places that are affected...namely in the IdURI unit.  It really is
> an
> > odd function, if it returns 0 you don't know if it is successful or
> > not...ugly.

> > So I'll just hack something in the ProcessPath function for now.  Anyone
> > from the Indy team care to comment/advise?

> > Thanks,
> > Brian

> > "Brian Moelk" <bmo...@brainendeavor.com> wrote in message
> > news:3b672dfa$1_1@dnews...
> > > In the ProcessPath procedure there is an infinite loop on a path given
> > like
> > > the following:

> > > /nowsmforms/../../images/

> > > The first pass chunks the path down to:   /../images/
> > > The subsequent passes don't do anything since:

> > > i = 3 after i := Pos('./', APath);
> > > while loop doesn't do anything (APath[i-1] = '.')
> > > i = 0 after i := RPos('/', APath, i-3);  file://Starts from char 0 so
does
> > > nothing
> > > delete(APath, i, Pos('../', APath) + 2 - i) = delete('/../images/', 0,
2
> +
> > > 2 - 0)

> > > unfortunately Delete with a 0 index doesn't work??

> > > Comments from System.pas  _Delete:
> > > {       if index not in [1 .. Length(s)] do nothing     }

> > > Does this work differently in different Delphi versions?  I'm using D5
> > > Enterprise Update Pack 1.

> > > Regardless, infinite loops are generally bad and there is one here.

> > > If it is supposed to be akin to Pos then RPos should not return a 0 as
a
> > > possible return value.  An easy way to fix this is to change the loop
> > from:

> > > for i := LStartPos downto 1 do begin

> > > to:

> > > for i := LStartPos downto 0 do begin

> > > and set result := i + 1;

> > > I'm not sure about the sideeffects, but I'll try it with a bunch of
urls
> > and
> > > let you know.

> > > -Brian

> > > ProcessPath code:

> > >   procedure ProcessPath(Var APath: String);
> > >   Var
> > >     i: Integer;
> > >   begin
> > >     if Length(APath) = 0 then begin
> > >       APath := FURI.Path;
> > >     end
> > >     else begin
> > >       if APath[1] <> '/' then begin
> > >         APath := FURI.Path + APath;
> > >       end;
> > >       repeat
> > >         i := Pos('./', APath);
> > >         if i = 0 then begin
> > >           break;
> > >         end;
> > >         while (i > 1) and (APath[i - 1] <> '.') do begin
> > >           Delete(APath, i, 2);
> > >           i := Pos('./', APath);
> > >         end;
> > >         if i = 0 then begin
> > >           break;
> > >         end;

> > >         i := RPos('/', APath, i - 3);
> > >         delete(APath, i, Pos('../', APath) + 2 - i);
> > >       until false;
> > >     end;

> > >     FURI.Path := APath;
> > >   end;

Re:Indy TIdHTTP.DoRequest - ProcessPath


one more thing....

This *is* possible if you are building a web browser with Indy and someone
has an "incorrect path" on their website that you happen to click on.  This
can happen *all* the time if you are using Indy as a client and depending on
webmasters (or even worse programmers  ;] ) for your links.

Quote
"Doychin Bondzhev" <doyc...@dsoft-bg.com> wrote in message

news:3b67aa08_1@dnews...
Quote
> I'm not sure that such path is possible except in the case when you are
> trying to go downin into the servers directory structoure which is not
> accessible by default. Becouse this is an incorrect path this couses that
> infinite loop.

> Doychin

> "Brian Moelk" <bmo...@brainendeavor.com> wrote in message
> news:3b673967$1_2@dnews...
> > Ok...my first attempt at a fix doesn't really work.  RPos is used in
many
> > other places that are affected...namely in the IdURI unit.  It really is
> an
> > odd function, if it returns 0 you don't know if it is successful or
> > not...ugly.

> > So I'll just hack something in the ProcessPath function for now.  Anyone
> > from the Indy team care to comment/advise?

> > Thanks,
> > Brian

> > "Brian Moelk" <bmo...@brainendeavor.com> wrote in message
> > news:3b672dfa$1_1@dnews...
> > > In the ProcessPath procedure there is an infinite loop on a path given
> > like
> > > the following:

> > > /nowsmforms/../../images/

> > > The first pass chunks the path down to:   /../images/
> > > The subsequent passes don't do anything since:

> > > i = 3 after i := Pos('./', APath);
> > > while loop doesn't do anything (APath[i-1] = '.')
> > > i = 0 after i := RPos('/', APath, i-3);  //Starts from char 0 so does
> > > nothing
> > > delete(APath, i, Pos('../', APath) + 2 - i) = delete('/../images/', 0,
2
> +
> > > 2 - 0)

> > > unfortunately Delete with a 0 index doesn't work??

> > > Comments from System.pas  _Delete:
> > > {       if index not in [1 .. Length(s)] do nothing     }

> > > Does this work differently in different Delphi versions?  I'm using D5
> > > Enterprise Update Pack 1.

> > > Regardless, infinite loops are generally bad and there is one here.

> > > If it is supposed to be akin to Pos then RPos should not return a 0 as
a
> > > possible return value.  An easy way to fix this is to change the loop
> > from:

> > > for i := LStartPos downto 1 do begin

> > > to:

> > > for i := LStartPos downto 0 do begin

> > > and set result := i + 1;

> > > I'm not sure about the sideeffects, but I'll try it with a bunch of
urls
> > and
> > > let you know.

> > > -Brian

> > > ProcessPath code:

> > >   procedure ProcessPath(Var APath: String);
> > >   Var
> > >     i: Integer;
> > >   begin
> > >     if Length(APath) = 0 then begin
> > >       APath := FURI.Path;
> > >     end
> > >     else begin
> > >       if APath[1] <> '/' then begin
> > >         APath := FURI.Path + APath;
> > >       end;
> > >       repeat
> > >         i := Pos('./', APath);
> > >         if i = 0 then begin
> > >           break;
> > >         end;
> > >         while (i > 1) and (APath[i - 1] <> '.') do begin
> > >           Delete(APath, i, 2);
> > >           i := Pos('./', APath);
> > >         end;
> > >         if i = 0 then begin
> > >           break;
> > >         end;

> > >         i := RPos('/', APath, i - 3);
> > >         delete(APath, i, Pos('../', APath) + 2 - i);
> > >       until false;
> > >     end;

> > >     FURI.Path := APath;
> > >   end;

Re:Indy TIdHTTP.DoRequest - ProcessPath


Actually the correct fix has to be

      i := RPos('/', APath, i - 3);
      if i = 0 then begin
        break;
      end;
      delete(APath, i, Pos('../', APath) + 2 - i);

If the returned path is invalid it will produce some error and exception.
--
Doychin Bondzhev - Team Indy
doyc...@dsoft-bg.com
DdSoft-Bulgaria
http://www.dsoft-bg.com

Quote
"Brian Moelk" <bmo...@brainendeavor.com> wrote in message

news:3b680adc_2@dnews...
Quote
> Well....there are several reasons why this should be fixed:

> 1) that particular input is possible, it actually happened that's why I
> posted it
> 2) you should code defensively
> 3) the function intends to fix up relative paths so it should be able to
> deal with them
> 4) the function should not have an infinite loop based on *any* string
> input; since it takes a string and does no validation of the input
> 5) it's a bug that eats cpu time and causes things to hang

> Programmers can't make excuses for code that is buggy; it either works or
it
> doesn't.  Programmers should step up and take responsibility --- fix bugs
> even if it's not their code, handle garbage input, compensate for their
> users intellectual ability, whatever....it really doesn't matter as long
as
> the code works.

> When it comes to fixing it or not, it's really a no brainer.  Fix it.

> Like I said,  I hacked a fix in the process path function.  I added before
> the delete:

> if (i = 0) then
> begin
>     Inc(i);
> end;

> IMO, this is not the best way to fix it, I just didn't have the time
(right
> now) to go through the repercussions of fixing the RPos function.  RPos
> should work as Pos does, return 0 if not found, etc.  It would have helped
> if Delete was consistent with Copy, but oh well.

> -Brian

> "Doychin Bondzhev" <doyc...@dsoft-bg.com> wrote in message
> news:3b67aa08_1@dnews...
> > I'm not sure that such path is possible except in the case when you are
> > trying to go downin into the servers directory structoure which is not
> > accessible by default. Becouse this is an incorrect path this couses
that
> > infinite loop.

> > Doychin

> > "Brian Moelk" <bmo...@brainendeavor.com> wrote in message
> > news:3b673967$1_2@dnews...
> > > Ok...my first attempt at a fix doesn't really work.  RPos is used in
> many
> > > other places that are affected...namely in the IdURI unit.  It really
is
> > an
> > > odd function, if it returns 0 you don't know if it is successful or
> > > not...ugly.

> > > So I'll just hack something in the ProcessPath function for now.
Anyone
> > > from the Indy team care to comment/advise?

> > > Thanks,
> > > Brian

> > > "Brian Moelk" <bmo...@brainendeavor.com> wrote in message
> > > news:3b672dfa$1_1@dnews...
> > > > In the ProcessPath procedure there is an infinite loop on a path
given
> > > like
> > > > the following:

> > > > /nowsmforms/../../images/

> > > > The first pass chunks the path down to:   /../images/
> > > > The subsequent passes don't do anything since:

> > > > i = 3 after i := Pos('./', APath);
> > > > while loop doesn't do anything (APath[i-1] = '.')
> > > > i = 0 after i := RPos('/', APath, i-3);  file://Starts from char 0
so
> does
> > > > nothing
> > > > delete(APath, i, Pos('../', APath) + 2 - i) = delete('/../images/',
0,
> 2
> > +
> > > > 2 - 0)

> > > > unfortunately Delete with a 0 index doesn't work??

> > > > Comments from System.pas  _Delete:
> > > > {       if index not in [1 .. Length(s)] do nothing     }

> > > > Does this work differently in different Delphi versions?  I'm using
D5
> > > > Enterprise Update Pack 1.

> > > > Regardless, infinite loops are generally bad and there is one here.

> > > > If it is supposed to be akin to Pos then RPos should not return a 0
as
> a
> > > > possible return value.  An easy way to fix this is to change the
loop
> > > from:

> > > > for i := LStartPos downto 1 do begin

> > > > to:

> > > > for i := LStartPos downto 0 do begin

> > > > and set result := i + 1;

> > > > I'm not sure about the sideeffects, but I'll try it with a bunch of
> urls
> > > and
> > > > let you know.

> > > > -Brian

> > > > ProcessPath code:

> > > >   procedure ProcessPath(Var APath: String);
> > > >   Var
> > > >     i: Integer;
> > > >   begin
> > > >     if Length(APath) = 0 then begin
> > > >       APath := FURI.Path;
> > > >     end
> > > >     else begin
> > > >       if APath[1] <> '/' then begin
> > > >         APath := FURI.Path + APath;
> > > >       end;
> > > >       repeat
> > > >         i := Pos('./', APath);
> > > >         if i = 0 then begin
> > > >           break;
> > > >         end;
> > > >         while (i > 1) and (APath[i - 1] <> '.') do begin
> > > >           Delete(APath, i, 2);
> > > >           i := Pos('./', APath);
> > > >         end;
> > > >         if i = 0 then begin
> > > >           break;
> > > >         end;

> > > >         i := RPos('/', APath, i - 3);
> > > >         delete(APath, i, Pos('../', APath) + 2 - i);
> > > >       until false;
> > > >     end;

> > > >     FURI.Path := APath;
> > > >   end;

Re:Indy TIdHTTP.DoRequest - ProcessPath


bmo...@brainendeavor.com (Brian Moelk) wrote in <3b680adc_2@dnews>:

Quote
>Programmers can't make excuses for code that is buggy; it either works or it
>doesn't.  Programmers should step up and take responsibility --- fix bugs

Before you go jumping on anyone lets jump a few messages back:

Quote
>Ok...my first attempt at a fix doesn't really work.  RPos is used in many
>other places that are affected...namely in the IdURI unit.  It really is an
>odd function, if it returns 0 you don't know if it is successful or
>not...ugly.

First question - Are you a C programmer by chance? Delphi strings are 1
based. NOT 0 based. So nothing is wrong with RPos. As a matter of fact it was
done this way to mimic Pos. Which if you look at the help in Delphi for Pos
you will see it also returns a 0. 0 is not valid index, so its quite easy to
know if it was successful or not. Nothing ugly about it.

From the Delphi help on Pos:
Returns the index value of the first character in a specified substring that
occurs in a given string.

Unit

System

Category

string handling routines

function Pos(Substr: string; S: string): Integer;

Description

Pos searches for a substring, Substr, in a string, S. Substr and S are
string-type expressions.

Pos searches for Substr within S and returns an integer value that is the
index of the first character of Substr within S. Pos is case-sensitive. If
Substr is not found, Pos returns zero.

--
Chad Z. Hower (Kudzu) - http://www.pbe.com/Kudzu/
Current Location: Erie, PA
      "Programming is an art form that fights back"

Other Threads