2

I have problem inserting sql results into TStringGrid.I have following code:

 var i:Integer;
 begin
   SqlQuery1.SQL.Text:= 'SELECT * FROM `users`'; 
   SqlQuery1.Open;
   MySql55Connection1.Open;
   i:= 0;
   while not SQLQUERY1.EOF do
   begin
     i:= i+1;
     StringGrid1.Cells[0,i]:= SqlQuery1.FieldByName('Username').AsString;
     StringGrid1.Cells[1,i]:= SqlQuery1.FieldByName('Password').AsString;
     StringGrid1.Cells[2,i]:= SqlQuery1.FieldByName('id').AsString;
   end;
 end;

So in my database only one line. But program adding a lot of copies of this line in StringGrid and it causes error(Index out of bounds).

Johan
  • 74,508
  • 24
  • 191
  • 319
Andrew Zitsew
  • 45
  • 1
  • 6

1 Answers1

4

Danger
It appears you are storing passwords in plain text form in a database.
This is an extremely bad idea.
Never store passwords in a database.
Use salted hashes instead.
See: How do I hash a string with Delphi?

There are a couple of other problems in your code:

  • You don't ensure that the stringgrid has enough rows to hold your data.
  • You're not moving to the next line in the query.
  • You're opening the query before the connection is open.
  • You're using FieldByName inside a loop, this is going to be very slow.

Simple solution
Use a DBGrid.

If you insist on using a StringGrid
I suggest refactoring the code like so:

 var 
   i,a:Integer;
   FUsername, FPasswordHash, Fid, FSalt: TField;
 begin
   if not(MySQl55Connection.Active) then MySql55Connection1.Open;
   SqlQuery1.SQL.Text:= 'SELECT * FROM users';  //only use backticks on reserved words.
   SqlQuery1.Open;
   FUsername:= SqlQuery1.FieldByName('Username');
   //do not use plain text passwords!!
   FPasswordHash:= SQLQuery1.FieldByName('SaltedPasswordHashUsingSHA256');
   FId:= SqlQuery1.FieldByName('id');
   FSalt:= SQLQuery1.FieldByName('SaltUsingCryptoRandomFunction');
   a:= StringGrid1.FixedRowCount;
   if SQLQuery1.RecordCount = -1 then StringGrid1.RowCount = 100 //set it to something reasonable.  
   else StringGrid1.RowCount:= a + SQLQuery1.RecordCount;
   //SQLQuery1.DisableControls 
   try
     i:= StringGrid1.FixedRowCount;
     while not(SQLQuery1.EOF) do begin
       if i >= StringGrid1.RowCount then StringGrid1.RowCount:= i;
       StringGrid1.Cells[0,i]:= FUserName.AsString;
       StringGrid1.Cells[1,i]:= FPasswordHash.AsString;
       StringGrid1,Cells[3,i]:= FSaltInHex.AsString;
       StringGrid1.Cells[2,i]:= FId.AsString;
       SQLQuery1.Next;  //get next row.
       Inc(i);
     end; {while}
   finally
     //just in case you want to do endupdate or close the SQLQuery or do SQLQuery1.EnableControls 
   end;
 end;

Basic security example
Here's how to hash a password:

Download Lockbox3.
Put a THash on your form and set the hash property to SHA-512.
Use the following code to produce a hash result.

function StringToHex(const input: string): AnsiString;
var
  NumBytes, i: Integer;
  B: Byte;
  W: word;
  Wa: array[0..1] of byte absolute W;
begin
  NumBytes := input.length * SizeOf(Char);
  SetLength(Result, NumBytes * 2);
  for i := 1 to NumBytes do begin
    if SizeOf(Char) = 1 then begin
      B:= Byte(input[i]);
      BinToHex(@B, @Result[(I*2)+1], 1);
    end else begin
      W:= Word(input[i]);
      BinToHex(@Wa[0], @Result[(i*4+0)],1);
      BinToHex(@Wa[1], @Result[(i*4+1)],1);
    end; {else}  
  end;
end;

function TForm1.HashPassword(var Password: string; const Salt: string): string;
var
  KillPassword: pbyte;
begin
  Hash1.HashString(StringToHex(Password)+StringToHex(Salt));
  KillPassword:= PByte(@Password[1]);
  FillChar(KillPassword^, Length(Password)*SizeOf(Char), #0); //remove password from memory.  
  Password:= ''; //Now free password.
end;

function GenerateSalt( ByteCount: integer = 32): string;
var
  Buffer: TMemoryStream;
begin
  Buffer := TMemoryStream.Create;
  try
    Buffer.Size := ByteCount;
    RandomFillStream( Buffer);
    result := Stream_to_Base64( Buffer);
  finally
    Buffer.Free
  end;
end;

This is the minimum amount of work you can get away with whilst still having things secure.
Do not think that your passwords are unimportant because you just have a toy database, because people reuse passwords and thus your toy passwords end up being the same passwords used for online banking and such.
People are lazy....

Community
  • 1
  • 1
Johan
  • 74,508
  • 24
  • 191
  • 319
  • you were faster! Well then two things to add: 1: `RecordCount` may just return `-1`for SQL queries. Then you would have to calculate it by an extra loop. 2: you should reduce screen flicker slow-down by calling StringGrid.Rows.BeginUpdate and http://www.freepascal.org/docs-html/fcl/db/tdataset.disablecontrols.html in try-finally block – Arioch 'The Apr 12 '16 at 15:47
  • @Arioch'The, the for loop will not execute when `recordcount = -1`. – Johan Apr 12 '16 at 15:58
  • @Johan but it SHOULD execute, because it is NORMAL to return -1 for SQL queries no matter how many rows there really would be. RecordCount is ONLY required when dealing with local ISAM databases like DBF and Paradox, never required when dealing with remote SQL. PS didn't we already discussed it few months ago ? – Arioch 'The Apr 12 '16 at 16:03
  • you should use **SELECT COUNT(*)** because you can not trust **SQLQuery1.RecordCount** – moskito-x Apr 12 '16 at 16:08
  • @moskito-x no, that would be wrong too. After he would call COUNT and before he would fill the grid in another SELECT someone else can add more rows into the table. You should call dataset.last then WITHOUT CLOSING IT iterate back to dataset.BOF counting rows, then WITHOUT CLOSING IT set the grid row count and fill it iterating forward until EOF. That all to be done with suppressing screen flickering – Arioch 'The Apr 12 '16 at 16:17
  • @Arioch'The : COUNT(*) should be part of the First and only SELECT – moskito-x Apr 12 '16 at 16:20
  • I doubt it would work, aggregating functions are collapsing many rows into a singular result. Unless GroupBy is used. Maybe MYSQL would allow that non standard trick, don't know – Arioch 'The Apr 12 '16 at 16:23
  • @Johan i don't remember how in LCL but at least in VCL TStringGrid itself does not have BeginUpdate methods, but its Columns and Rows stringlists do. Also if SQLQuery is TDataSet then DisableControls better be called over it – Arioch 'The Apr 12 '16 at 16:26
  • @moskito-x **if not(MySQl55Connection.Active) then MySql55Connection1.Open;** exactly that. He wants to open the connection, not close it. Though i would remove If-then and just would call mysqlconnection.active := true; let the component itself check if it was already open or not yet – Arioch 'The Apr 12 '16 at 16:32
  • One should also close the query itself after the loop – Arioch 'The Apr 12 '16 at 16:35
  • @Johan `if not(MySQl55Connection.Active) then MySql55Connection1.Open;` is **OK** . I've got something confused :-( sorry for that. – moskito-x Apr 12 '16 at 16:57
  • I dropped my half-made answer not to recreate it from yours. I am a consultant now, big fees and no responsibility :-P – Arioch 'The Apr 12 '16 at 17:05
  • BTW `with StringGrid1 do RowCount := RowCount +1;` would be bad performance wise. If you want to avoid double loop then TList approach would be better: start with reasonable minimum like 16 rows, inside the loop do `if RowCount <=i then RowCount := RowCount * 2` and shrink back to `i+1` after the loop. Still i think double loop and *one-time* set of the grid length would be faster. Rule of thumb being that several changes in non-visual components are usually faster than single trip-around with OS visual layers – Arioch 'The Apr 12 '16 at 17:12
  • @Arioch'The : How can I do that ?? – moskito-x Apr 12 '16 at 17:14
  • @moskito-x, usually there is a delay of a few hours before you can accept an answer. This is to give other people time to come up with a better answer. In due course a checkmark (in gray) will appear. You can click on that and thus accept the answer as one that answered your question satisfactory. – Johan Apr 12 '16 at 17:17
  • @Johan : sorry I'm not OP ;-) – moskito-x Apr 12 '16 at 17:20
  • @moskito-x damn! You got me here! :-D and i misled Johan then :-( – Arioch 'The Apr 12 '16 at 17:26
  • @AndrewZitsew MEMO stands for text BLOB. I guess for your fields you better use VARCHAR(n) (MySQL lacks it so you would have to use CHAR instead) SQL type than BLOB... Try to change those column datatypes from blobs to strings or at least convert them to strings in the query itself - http://stackoverflow.com/questions/15368753 – Arioch 'The Apr 13 '16 at 09:26
  • Also if your problem is representing data then maybe the correct approach would be to adjust DB grid or use more advanced DB grid? JVCL was ported to Lazarus for example. CodeTyphon comes with all many types of extra components for example. I think u made a wrong turn in the beginning of the path. You better keep using db-aware controls, because they would not crash your app with out-of-memory on 100 million rows query, nor would they wait few minutes before displaying first 10 rows of 1M query like ur StringGrid FetchAll approach would. http://www.catb.org/esr/faqs/smart-questions.html#goal – Arioch 'The Apr 13 '16 at 09:30