Recent

Author Topic: PosSetEx bug  (Read 1081 times)

dje

  • Full Member
  • ***
  • Posts: 134
PosSetEx bug
« on: December 09, 2022, 05:15:24 am »
Im using FPC 3.2.2. The code for PosSetEx doesn't quite match the code for PosEx

I checked the code for PosSetEx which is identical to my installed version:
https://gitlab.com/freepascal.org/fpc/source/-/blob/main/packages/rtl-objpas/src/inc/strutils.pp

So, the issue is the following two lines "should" return the same value:
Code: Pascal  [Select][+][-]
  1.   WriteLn(PosEx('p', 'freepascal', -1000));
  2.   WriteLn(PosSetEx(['p'], 'freepascal', -1000));
Except I get:
Code: Pascal  [Select][+][-]
  1. 0
  2. -891

The PosEx function includes this code which returns 0 for an Offset outside the string length:
Code: Pascal  [Select][+][-]
  1. if (Offset < 1) or (Offset > SizeUInt(Length(S))) then exit(0);  

While the PosSetEx function only checks the upper bounds:
Code: Pascal  [Select][+][-]
  1.    i:=length(s); j:=count;
  2.    if j>i then begin result:=0; exit; end;  

I would think the PosSetEx should also check the lower bounds. ie:
Code: Pascal  [Select][+][-]
  1.  if (j < 1) or (j>i) then begin result:=0; exit; end;


marcov

  • Administrator
  • Hero Member
  • *
  • Posts: 11383
  • FPC developer.
Re: PosSetEx bug
« Reply #1 on: December 09, 2022, 11:32:59 am »
I wonder if it really useful to spend the two cycles for obvious wrong input.

Bart

  • Hero Member
  • *****
  • Posts: 5275
    • Bart en Mariska's Webstek
Re: PosSetEx bug
« Reply #2 on: December 09, 2022, 03:37:09 pm »
If I understand the code correctlyt then, if count < 0 then j will become count (negative) and s[j] will be accessed, but the index (j) is out of bounds.
That may potentially be dangerous.

Bart

KodeZwerg

  • Hero Member
  • *****
  • Posts: 2007
  • Fifty shades of code.
    • Delphi & FreePascal
Re: PosSetEx bug
« Reply #3 on: December 09, 2022, 03:52:04 pm »
I wonder if it really useful to spend the two cycles for obvious wrong input.
But it is a bug, I agree to OP. Wrong input or not. When I do read the documentation it should return 0.
Quote
Description
PosSetEx returns the position in s of the first found character which is in the set c, and starts searching at character position Count. If none of the characters in c is found in s, then 0 is returned.

Errors
None.
In his example it returns a very strange result, dont you think that too?
I mean it find something but probably not at that negative adress :P
« Last Edit: Tomorrow at 31:76:97 xm by KodeZwerg »

marcov

  • Administrator
  • Hero Member
  • *
  • Posts: 11383
  • FPC developer.
Re: PosSetEx bug
« Reply #4 on: December 09, 2022, 04:25:00 pm »
I wonder if it really useful to spend the two cycles for obvious wrong input.
But it is a bug,

However, is it a code or a documentation bug ? When I wrote it, I definitely didn't consider negative input logical, as it serves no purpose whatsoever.

Bart

  • Hero Member
  • *****
  • Posts: 5275
    • Bart en Mariska's Webstek
Re: PosSetEx bug
« Reply #5 on: December 09, 2022, 04:41:21 pm »
If you consider it a documentation issue, then it should be stated that the behavoiur is undefined with values for count <= 0.
To me it would make more sense to have it behave lik PosEx with invalid count parameters.

Bart

dje

  • Full Member
  • ***
  • Posts: 134
Re: PosSetEx bug
« Reply #6 on: December 09, 2022, 04:57:02 pm »
RPosEx is interesting. It handles negitive offsets, but in a strange way.

Offsets are cardinal, so, neg values are treated as positive, and discarded with the max bound check. But, zero offset are converted to pointers:
Code: Pascal  [Select][+][-]
  1.      p:=@s[offs];
  2.      p2:=@s[1];
And then discarded via this code since @s[0] < @s[1]
Code: Pascal  [Select][+][-]
  1. while (p2<=p)

So, that means PosEx and RPosEx handle zero and neg numbers by returning zero.

Although, RPosEx looks more like an accidental side effect.
« Last Edit: December 09, 2022, 04:58:41 pm by dje »

marcov

  • Administrator
  • Hero Member
  • *
  • Posts: 11383
  • FPC developer.
Re: PosSetEx bug
« Reply #7 on: December 09, 2022, 05:30:26 pm »
It is. RPos and the set variant are based on older Modula2 routines, where the main type (Cardinal) is unsigned.

Maybe posex got fixed for some Delphi compatibility measure, but for non delphi compat routines, I think it is wiser to simply adapt the documentation.

KodeZwerg

  • Hero Member
  • *****
  • Posts: 2007
  • Fifty shades of code.
    • Delphi & FreePascal
Re: PosSetEx bug
« Reply #8 on: December 09, 2022, 05:40:34 pm »
I definitely didn't consider negative input logical
Always await the unexpected :-* else we wouldn't have this thread  O:-)
as it serves no purpose whatsoever.
Just because I say my opinion, it is a bug, does not mean that I am not on your side, I fully agree to you that it is ill to call it that way.

About where that bug might be, I am clueless since I am not the inventor of that routine.
Whatever he/she might have thought it should do, it should be in balance with it's documentation.
So one of them is wrong, my personal opinion, it is the method since negative index for a string can never be good.
« Last Edit: Tomorrow at 31:76:97 xm by KodeZwerg »

dje

  • Full Member
  • ***
  • Posts: 134
Re: PosSetEx bug
« Reply #9 on: December 09, 2022, 05:59:02 pm »
The Delphi doc's for PosEx state:
Quote
If SubStr is not found or Offset is invalid (greater than the length of S or less than 1), then the result is 0.
https://docwiki.embarcadero.com/Libraries/Sydney/en/System.StrUtils.PosEx

I understand that PosSetEx is not in Delphi, but shouldn't it follow the same logic as PosEx?.

On, a side note, I was hoping one day the function RPosSetEx would be added to match the RPosEx  :)
« Last Edit: December 09, 2022, 06:02:19 pm by dje »

dje

  • Full Member
  • ***
  • Posts: 134
Re: PosSetEx bug
« Reply #10 on: December 09, 2022, 06:06:19 pm »
O, BTW. This thread was started while I'm re-coding some of these routines for HighSpeed Pascal on the Atari ST. I wanted my routines to be the same as FPC,  hence why I'm looking at these boundary cases.

Another interesting note is: HighSpeed Pascal returns 1 for Pos(EmptyStr, 'Pascal')

Ie: Its opinion is an empty string can be found at the start of all strings. Wow.

I guess standards matter.
« Last Edit: December 09, 2022, 06:08:44 pm by dje »

Bart

  • Hero Member
  • *****
  • Posts: 5275
    • Bart en Mariska's Webstek
Re: PosSetEx bug
« Reply #11 on: December 09, 2022, 06:41:22 pm »
Maybe posex got fixed for some Delphi compatibility measure, but for non delphi compat routines, I think it is wiser to simply adapt the documentation.

So, we really want this?
Code: Pascal  [Select][+][-]
  1. {$mode objfpc}
  2. {$h+}
  3. uses
  4.   StrUtils;
  5. var
  6.   Res: SizeInt;
  7. begin
  8.   Res := PosSetEx(['1'], '2', Low(Integer));
  9.   writeln('Res = ',Res);
  10. end.
Code: [Select]
C:\Users\Bart\LazarusProjecten\ConsoleProjecten>fpc test.pas
Free Pascal Compiler version 3.3.1 [2022/10/11] for i386
Copyright (c) 1993-2022 by Florian Klaempfl and others
Target OS: Win32 for i386
Compiling test.pas
Linking test.exe
10 lines compiled, 0.5 sec, 69808 bytes code, 4420 bytes data

C:\Users\Bart\LazarusProjecten\ConsoleProjecten>test
An unhandled exception occurred at $0040D7FF:
EAccessViolation: Access violation
  $0040D7FF

Bart

 

TinyPortal © 2005-2018