TFPSList.Pack is completely broken (patch)
Original Reporter info from Mantis: Michalis @michaliskambi
-
Reporter name: Michalis Kamburelis
Original Reporter info from Mantis: Michalis @michaliskambi
- Reporter name: Michalis Kamburelis
Description:
- TFPSList.Pack implementation happily assumes that list item size is the same as the Pointer size on current architecture. That's because it checks "assigned(pointer(psrc^))" to see if item should be kept. Needless to say, this is incorrect in many cases (and in some cases, like LongInt, it may be incorrect on 64-bit architectures but accidentally work on 32-bit architectures).
Attached testcase demonstrates how Pack on a list of bytes removes only some of the bytes --- only when it sees 4 following bytes (tested on 32-bit architecture, on 64-bit Pack will probably not do anything).
$ fpc pack_invalid_size.pas
$ ./pack_invalid_size
0: 0
1: 0
2: 0
3: 1
4: 2
5: 0
6: 3
7: 4
-
Also, implementation of TFPSList.Pack does inc(psrc), which means that it moves the psrc by 1 byte. So it doesn't really work even when list item has size = pointer size (4 or 8 bytes usually). Can be tested by replacing Byte with LongInt in pack_invalid_size.pas testcase, and observing that now the result is different but also wrong: list is empty at the end.
-
Also, it seems that it moves items by direct System.Move, without bothering to call Deref or zero memory. So it will probably cause troubles in case of types that require Initialize/Finalize.
I would advice to just remove the current Pack method. It seems broken beyond hope. I also don't see much use for this method. Eventually, maybe a simple and clean (without any pointer magic) TFPGObjectList.Pack can be implemented later as a replacement.
Mantis conversion info:
- Mantis ID: 21636
- OS: Debian GNU/Linux
- OS Build: (testing)
- Platform: i386
- Fixed in version: 3.0.0
- Fixed in revision: 21039 (#6b93cbcb)