Bug 163691 - Use std::copy() instead of memcpy()
Summary: Use std::copy() instead of memcpy()
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:25.8.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks: Dev-related
  Show dependency treegraph
 
Reported: 2024-10-30 17:33 UTC by Hossein
Modified: 2025-03-28 13:48 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Hossein 2024-10-30 17:33:19 UTC
Description:
In LibreOffice core, there are many places where a block of memory is copied from one place to another. In this case, two functions are used:

1. Older C style memcpy(), which is defined in <string.h> (alternatively <cstring>) and is used as:

memcpy( to, from, count ); // to and from are void* pointers
https://en.cppreference.com/w/cpp/string/byte/memcpy

2. Newer C++ style std::copy, which is defined in <algorithm> and is used with pointers and iterators as:

std::copy( from, from + size, to ); // pointers
std::copy( from_iter_first, from_iter_last, to_iter_first ); // iterators
https://en.cppreference.com/w/cpp/algorithm/copy 

The goal here is to refactor the code and replace the usages of memcpy() with equivalent std::copy().

Finding instances:
You can find instances of memcpy() usage in C++ code with:

$ git grep -w memcpy *.hxx *.cxx

Currently there are 720 instances of using memcpy in the code.

Notes:
1. Correctly setting the parameters is important, as they are different in std::copy() and memcpy().

2. You have to make sure that the code behavior does not change with your patch. If classes with user-defined copy constructor are used instead of basic data types, you have to make sure that std::copy which invokes copy constructor does not make any difference.

Example:
You can see the two std::copy() calls (commented) equivalent to memcpy() in this code snippet:

#include <iostream>
#include <vector>
#include <cstring>
#include <algorithm>

int main()
{
        std::vector<int> from {1,2,3,4,5}, to(5);

        std::memcpy(to.data(), from.data(), 5 * sizeof(int));
//      std::copy(from.data(), from.data() + 5, to.data());
//      std::copy(from.begin(), from.end(), to.begin());

        for (int num : to)
                std::cout << num << " ";
        std::cout << "\n";

        return 0;
}
Comment 1 Mike Kaganski 2024-10-31 05:15:06 UTC
Also, memmove (usable with overlapping memory) may be replaced with either std::copy (when end of dest range is inside of source range, i.e. copying to the left), or std::copy_backward (when start of dest range is inside of source range, i.e. copying to the right).

Note also, that e.g. cppreference states, that "implementations of std::copy ... use bulk copy functions such as std::memmove", so not perfectly identical to memcpy, which can't work on overlapping ranges, but is most efficient.
Comment 2 Alin Andrei Abahnencei 2024-12-20 19:08:45 UTC
Hey! I provided a patch for this:
https://gerrit.libreoffice.org/c/core/+/178954
Comment 3 Commit Notification 2024-12-20 21:43:14 UTC
Alin Andrei Abahnencei committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/8391bd0ea4fb3a7f4cad4149ec63c980eab5808d

tdf#163691 Use std::copy() instead of memmove()

It will be available in 25.8.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 4 Hossein 2025-01-21 12:55:55 UTC
This is a multi-hacker EasyHack. Please do not assign it to yourself, but rather start working on it.
Comment 5 Kukee 2025-03-11 16:41:35 UTC
I provided a patch for this:
https://gerrit.libreoffice.org/c/core/+/182787
Comment 6 Commit Notification 2025-03-11 19:57:53 UTC
Kukee Thoo committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/bff3d755c8c11721054f4ff40a3d5f723b0c6b96

tdf#163691 Use std::copy() instead of memcpy()

It will be available in 25.8.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 7 Commit Notification 2025-03-18 13:27:13 UTC
Akshay Kumar Dubey committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/90490e7436ab8d823994b50472622d1c83fa118e

tdf#163691 Use std::copy() instead of memcpy().

It will be available in 25.8.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 8 Haokun Wu 2025-03-25 04:08:13 UTC
Patch submitted to Gerrit: https://gerrit.libreoffice.org/c/core/+/183284
Replaced `memcpy` with `std::copy` in `basmgr.cxx` and `image.cxx`.
Comment 9 Kukee 2025-03-26 09:46:49 UTC
I provided a patch for this:
https://gerrit.libreoffice.org/c/core/+/183305
Comment 10 Commit Notification 2025-03-26 11:26:04 UTC
Kukee Thoo committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/d40097edc58d93d823d2892e0b12a8ce43669b83

tdf#163691 Use std::copy() instead of memcpy() in docpasswordhelper.cxx

It will be available in 25.8.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 11 Commit Notification 2025-03-27 00:23:40 UTC
Haokun Wu committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/fa357e4a0a44471637373302c4391e6b0c2f1a20

tdf#163691 Replace memcpy with std::copy in basmgr.cxx and image.cxx

It will be available in 25.8.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 12 Commit Notification 2025-03-28 13:48:43 UTC
Devashish Gupta committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/1d04721870ab60cb6d66d99dc62ee21eee02232f

tdf#163691 Replace memcpy / memmove with std::copy / std::copy_backward

It will be available in 25.8.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.