Bug in md5 unit?

[Tommi]

This is a piece of code of md5 unit:

Code: [Select]

function MDFile(const Filename: RawByteString; const Version: TMDVersion; const BufSize: PtrUInt): TMDDigest;
var
  F: File;
  Buf: Pchar;
  Context: TMDContext;
  Count: Cardinal;
  ofm: Longint;
begin
  MDInit(Context, Version);

  Assign(F, Filename);
  {$push}{$i-}
  ofm := FileMode;
  FileMode := 0;
  Reset(F, 1);
  {$pop}

  if IOResult = 0 then
  begin
    GetMem(Buf, BufSize);
    repeat
      BlockRead(F, Buf^, Bufsize, Count);
      if Count > 0 then
        MDUpdate(Context, Buf^, Count);
    until Count < BufSize;
    FreeMem(Buf, BufSize);
    Close(F);
  end;

  MDFinal(Context, Result);
  FileMode := ofm;

The problem is that if IOResult is <> 0 then md5 is not computed but the function produces a result anyway, without raising any exception.

I suggest this:

Code: [Select]

[...]
  a:=IOResult;
  if a = 0 then
  begin
    GetMem(Buf, BufSize);
    repeat
      BlockRead(F, Buf^, Bufsize, Count);
      if Count > 0 then
        MDUpdate(Context, Buf^, Count);
    until Count < BufSize;
    FreeMem(Buf, BufSize);
    Close(F);
    MDFinal(Context, Result);
  end
  else
  begin
     for a := 0 to length(Result)-1 do Result[a]:=0;
  end;
  FileMode := ofm;
end;
In this way, if opening file fails,the result is a string of zeros and the caller can detect the issue.

Moreover I see that Reset and BlockRead cannot read path+filenames longer than 256 character. But this case in linux happens often because, when you use an external hard disk it is usually mounted with UUID in the path, and so a lot of characters are wasted.

Thank you

[Bart]

I remember seeing a bugreport about this (or another hash function), but I was unable to find it in the bugtracker.
Or maybe it was discussed on fpc-devel ML?

Bart

[Tommi]

Problems corrected, now it works:

Code: [Select]

function MDFile(const Filename: RawByteString; const Version: TMDVersion; const BufSize: PtrUInt): TMDDigest;
var
  F: THandle;
  Buf: Pchar;
  Context: TMDContext;
  Count: Cardinal;
  a:integer;
  ok:boolean;
begin
  ok:=false;
  MDInit(Context, Version);
  try
    F:=FileOpen(Filename,fmOpenRead);
    GetMem(Buf, BufSize);
    repeat
      Count:=FileRead(F, Buf^, Bufsize);
      if Count > 0 then
        MDUpdate(Context, Buf^, Count);
    until Count < BufSize;
    FreeMem(Buf, BufSize);
    fileClose(F);
    MDFinal(Context, Result);
    ok:=true;
  except
    ok:=false;
  end;
  if not ok then for a := 0 to length(Result)-1 do Result[a]:=0;
end;

Now accepts long filenames, and if some problem happens reading files, the result will be a zeroed string and not a deceptive result.
I would be proud if you would implement this change into new releases.

[wp]

Many developers do not read the forum. Please file a regular bug report (for the FPC project, https://gitlab.com/freepascal.org/fpc/source/-/issues) so that the relevant people get to see it.

[Bart]

Reported as issue #41194.

Bart

[Thaddy]

Small remarks:
- Use allocmem() and Result := Default(TMDDigest);
- Use PByte, PChar is ambiguous.
E.g.:

Code: Pascal  [Select][+][-]

1. function MDFile(const Filename: RawByteString; const Version: TMDVersion; const BufSize: PtrUInt): TMDDigest;
2. var
3.   F: File of byte;
4.   Buf: PByte = nil;
5.   Context: TMDContext;
6.   Count: Cardinal;
7.   ok:integer = 0;
8. begin
9. {$push}
10. {$I-}
11.   Result := Default(TMDDigest);
12.   Assign(F, Filename);
13.   FileMode := 0;
14.   Reset(F);
15.   Ok:= IOResult;
16.   if Ok = 0 then
17.   begin
18.     { initialized to zero's }
19.     Buf :=AllocMem(BufSize);
20.     MDInit(Context, Version);
21.     try
22.       repeat
23.         BlockRead(F, Buf^, Bufsize, count);
24.         if Count > 0 then
25.           MDUpdate(Context, Buf^, Count);
26.       until Count < BufSize;
27.       Ok := IOResult;
28.     finally
29.       FreeMem(Buf, BufSize);
30.       MDFinal(Context, Result);
31.     end;
32.     Close(F);
33.   end;
34. {$pop}
35. { do not hide IO errors, but propagate them for proper exception handling }
36.  if Ok <> 0 then
37.    raise EInOutError.CreateFmt('IO error %d',[Ok]);
38. end;

Frankly, that whole unit needs revision.

[Bart]

Why not simpler, and avoiding try..finally?
(IIRC blockread won't cause an IOException)

Code: Pascal  [Select][+][-]

1. function MDFile(const Filename: UnicodeString; const Version: TMDVersion; const BufSize: PtrUInt): TMDDigest;
2. var
3.   F: File;
4.   Buf: Pchar;
5.   Context: TMDContext;
6.   Count: Cardinal;
7.   ofm: Longint;
8. begin
9.   MDInit(Context, Version);
10.  
11.   Assign(F, Filename);
12.   {$push}{$i-}
13.   ofm := FileMode;
14.   FileMode := 0;
15.   Reset(F, 1);
16.   {$pop}
17.  
18.   if IOResult = 0 then
19.   begin
20.     GetMem(Buf, BufSize);
21.     repeat
22.       BlockRead(F, Buf^, Bufsize, Count);
23.       if Count > 0 then
24.         MDUpdate(Context, Buf^, Count);
25.     until Count < BufSize;
26.     FreeMem(Buf, BufSize);
27.     Close(F);
28.   end
29.   else
30.     Result := Default(TMDDigest);
31.  
32.   MDFinal(Context, Result);
33.   FileMode := ofm;
34. end;

Bart

[Thaddy]

BlockRead does not normally cause an IO exception, that is true.
Still, even you simple version requires PByte instead of PChar.
Probably caused by misinterpreting char * in some original C code.

[Bart]

I don't think the nature of the buffer matters here.
Blockread is agnostic of what it reas into the buffer, it may be chars, bytes, records, aliens or whatnot.

Bart

[Bart]

Reported as issue #41194.

And already fixed by Michael.

Bart