Issue-#109 Bounds checking in Kernel.MakeFileName

[Robert]

The first listing shows some test cases with the current code. The second listing shows the same tests with the proposed code below.
For these tests name is declared as "ARRAY 10 OF CHAR".

With the old code line 9 is incorrect, and line 13 crashed.

Code: Select all

       name          type   ->   name

   0   Fred          xYz         Fred.xyz
   1   Fred.         xYz         Fred
   2   Fred.abc      xYz         Fred.abc
   3   Fred                      Fred.odc
   4   Fred.                     Fred
   5   Fred.abc                  Fred.abc

   6   Alice.abc                 Alice.abc
   7   Alice         xYz         Alice.xyz
   8   Robert.ab                 Robert.ab
   9   Robert        xY          Robert

  10   Alice         pqr         Alice.pqr
  11   Robert        pqr         Robert
  12   Robert                    Robert
  13   Alice         pqrs        

Code: Select all

       name          type   ->   name

   0   Fred          xYz         Fred.xyz
   1   Fred.         xYz         Fred
   2   Fred.abc      xYz         Fred.abc
   3   Fred                      Fred.odc
   4   Fred.                     Fred
   5   Fred.abc                  Fred.abc

   6   Alice.abc                 Alice.abc
   7   Alice         xYz         Alice.xyz
   8   Robert.ab                 Robert.ab
   9   Robert        xY          Robert.xy

  10   Alice         pqr         Alice.pqr
  11   Robert        pqr         Robert
  12   Robert                    Robert
  13   Alice         pqrs        Alice

Code: Select all

	PROCEDURE MakeFileName* (VAR name: ARRAY OF CHAR; type: ARRAY OF CHAR);
		VAR i, j, extLen: INTEGER; ext: ARRAY 8 OF CHAR; ch: CHAR;
	BEGIN
		i := 0;
		WHILE (name[i] # 0X) & (name[i] # ".") DO INC(i) END;
		IF name[i] = "." THEN
			IF name[i + 1] = 0X THEN name[i] := 0X END
		ELSE
			IF type = "" THEN extLen := 3 ELSE extLen := LEN(type$) END;
			IF i < LEN(name) - extLen - 1 THEN
				IF type = "" THEN ext := docType ELSE ext := type$ END;
				name[i] := "."; INC(i); j := 0; ch := ext[0];
				WHILE ch # 0X DO
					name[i] := Lower(ch); INC(i); INC(j); ch := ext[j]
				END;
				name[i] := 0X
			END
		END
	END MakeFileName;

[Josef Templ]

To me, it doesn't give sense to strip the extension in case of string overflow.
Just remove the length check and everything gets simple and right.
You can also turn the length check into an ASSERT, if you like.
It is clearly the intention that string overflow does not occur in practice
because name is large enough. This can be derived from the fact that string overflow checking
did not work correctly in the 1.6 version.

Also, there is a bug in your proposal, I think:
If name contains a "." and the length is not sufficient, name will stay unmodified no matter
how the part after the "." in name looks like. This leads to an unexpected extension.

This would be my proposal:

Code: Select all

      ELSE
         IF type = "" THEN extLen := LEN(docType$); ext := docType ELSE extLen := LEN(type$); ext := type$ END;
         name[i] := "."; INC(i); j := 0; ch := ext[0];
         WHILE ch # 0X DO
             name[i] := Lower(ch); INC(i); INC(j); ch := ext[j]
          END;
          name[i] := 0X
      END

I have not yet tested any of the solutions.

- Josef

[Robert]

To me, it doesn't give sense to strip the extension in case of string overflow.

I agree that a lot of this does not make sense to me. What I was trying to do was correct the existing bounds checking without altering the existing behaviour.

Personally I am equally happy to correct or remove the bounds checking. What I don't like is keeping faulty bounds checking.

Also, there is a bug in your proposal, I think:
If name contains a "." and the length is not sufficient, name will stay unmodified no matter
how the part after the "." in name looks like. This leads to an unexpected extension.

I don't quite understand. Can you give an explicit example of inputs that might cause this problem, and I will add it to my test cases and see what happens - but probably not before tomorrow night.

Regards
Robert

[Josef Templ]

Sorry, I mixed things up.
The proposal is OK and has a minimum of changes of the original behavior.

- Josef

[Robert]

Ivan

I think we can vote on this now - I first raised this issue on 21-January.

[Josef Templ]

I have created the issue branch and committed the changes as proposed by Robert.
The two identical IFs (IF type = "" ...) have been merged into one
because they belong together logically.

For the changes see http://redmine.blackboxframework.org/pr ... b02938df3e.
For me this would be ready for voting now.

- Josef

[Robert]

If you move the type assignment outside the IF clause, as you suggest, you can simplify it by removing the variable extLen.

Code: Select all

IF type = "" THEN ext := docType ELSE ext := type$ END;
      IF i < LEN(name) - LEN(ext$) - 1 THEN

[Josef Templ]

Good point, thanks.
Removed.

see http://redmine.blackboxframework.org/pr ... 2f00dae9b1.

- Josef

[Ivan Denisov]

I made the voting. It will be very good if you provide some test example which demonstrating the problem.

[Robert]

I made the voting. It will be very good if you provide some test example which demonstrating the problem.

I don't suppose there is a problem; there is only a potential problem if the routine is called with a string that is either too short, or nearly too short.

Examples of both these situations are in the first post above; lines 9 & 13.