[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
Awfa wrote: Thanks for taking a look at the build. I will do a new PR addressing the issues. https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
bulbazord wrote: > I got an email that the build failed and I should revert because of this: > https://lab.llvm.org/buildbot/#/builders/68/builds/72623 > > @bulbazord can you or someone with write permissions revert this PR so I have > time to triage the issue? As David said, no need since that was unrelated to your change. Jonas makes a good point in his review though (that I totally missed when I reviewed), please open a new PR that addresses his feedback. https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
DavidSpickett wrote: Please address @JDevlieghere's comments in a new PR. https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
DavidSpickett wrote: > @bulbazord can you or someone with write permissions revert this PR so I have > time to triage the issue? The next build is green, those DAP tests do fail once in a while so it's unrelated to your change. https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
DavidSpickett wrote: > So a good follow up PR would be to list it in that document. It's self > explanatory but still, weird that it's not there. https://github.com/llvm/llvm-project/pull/89357 https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
Awfa wrote: I got an email that the build failed and I should revert because of this: https://lab.llvm.org/buildbot/#/builders/68/builds/72623 @bulbazord can you or someone with write permissions revert this PR so I have time to triage the issue? https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -3419,7 +3419,7 @@ bool GDBRemoteCommunicationClient::GetFileExists(
}
bool GDBRemoteCommunicationClient::CalculateMD5(
-const lldb_private::FileSpec _spec, uint64_t , uint64_t ) {
+const lldb_private::FileSpec _spec, uint64_t , uint64_t ) {
JDevlieghere wrote:
If we're changing the interface anyway, could we have this return an
`std::optional` to match what `md5_contents` returns?
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1197,6 +1197,32 @@ Status Platform::PutFile(const FileSpec , const
FileSpec ,
if (!source_file)
return Status(source_file.takeError());
Status error;
+
+ bool requires_upload = true;
+ uint64_t dest_md5_low, dest_md5_high;
+ bool success = CalculateMD5(destination, dest_md5_low, dest_md5_high);
+ if (!success) {
+LLDB_LOGF(log, "[PutFile] couldn't get md5 sum of destination");
+ } else {
+auto local_md5 = llvm::sys::fs::md5_contents(source.GetPath());
JDevlieghere wrote:
It's not obvious from the right hand side what the return value is. Case in
point, it returns an `ErrorOr` and if I hadn't looked it up I
wouldn't have noticed we're not actually logging the error.
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
github-actions[bot] wrote: @Awfa Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our [build bots](https://lab.llvm.org/buildbot/). If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail [here](https://llvm.org/docs/MyFirstTypoFix.html#myfirsttypofix-issues-after-landing-your-pr). If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of [LLVM development](https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy). You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
https://github.com/bulbazord closed https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
bulbazord wrote: > Someone will have to merge for me because it says "Only those with write > access to this repository can merge pull requests." > > Am I supposed to request write access somewhere before making these PRs? I > haven't contributed before. First time contributors aren't expected to have commit access, so no worries there. I approved it so I can land this change for you. Let's keep an eye on the build bots in case this inadvertently introduces any regressions. :) https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
Awfa wrote: Someone will have to merge for me because it says "Only those with write access to this repository can merge pull requests." Am I supposed to request write access somewhere before making these PRs? I haven't contributed before. https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
emaste wrote: Ok, submitted https://github.com/llvm/llvm-project/issues/89271 for the MD5 migration. I agree that issue does not block this change. https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
https://github.com/bulbazord approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
DavidSpickett wrote: @bulbazord please give the final approval if it looks good to you. We can sort out moving from MD5 on an issue. https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
DavidSpickett wrote: If it were a GDB packet it would be in https://sourceware.org/gdb/current/onlinedocs/gdb.html/Host-I_002fO-Packets.html#Host-I_002fO-Packets but I see no sign of it, or in GDB's sources. The first appearance of `vFile:MD5` is e0f8f574c7f41f5b61ec01aa290d52fc55a3f3c9 though it is not documented as one of our extensions in https://github.com/llvm/llvm-project/blob/main/lldb/docs/lldb-platform-packets.txt. So a good follow up PR would be to list it in that document. It's self explanatory but still, weird that it's not there. This means we could add `vFile:betterHash` (or a more general `hash:`) and try sending it to the remote. If the remote responds that it doesn't support it, we ask it for MD5. Though we may want a way to say don't use MD5 even if the remote would accept it, if security is the concern (a bit like ssh key formats work). In the case of this specific PR, it's fixing code that should have worked all along but didn't. So the issue of MD5 collision is worth looking at, but I wouldn't let it block this. @emaste, since you know about the shortcomings of MD5, could you open an issue for this? And I'll add to that the internal lldb details. https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
Awfa wrote: > MD5 is insufficient for claiming that files are identical; how do we migrate > this to a secure hash? Is there an attack vector you're concerned about? Or are you wary of workflow friction when a file won't upload to the remote platform because the hashes accidently collide? https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
emaste wrote: MD5 is insufficient for claiming that files are identical; how do we migrate this to a secure hash? https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1197,6 +1197,34 @@ Status Platform::PutFile(const FileSpec , const
FileSpec ,
if (!source_file)
return Status(source_file.takeError());
Status error;
+
+ bool requires_upload = true;
+ {
Awfa wrote:
Just removed the scoping - so the md5 vars are part of the main scope.
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1184,7 +1184,7 @@ bool Platform::IsCompatibleArchitecture(const ArchSpec
,
Status Platform::PutFile(const FileSpec , const FileSpec ,
uint32_t uid, uint32_t gid) {
Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(log, "[PutFile] Using block by block transfer\n");
+ LLDB_LOGF(log, "[PutFile] Using block by block transfer");
Awfa wrote:
I decided to leave it out of this PR
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
https://github.com/Awfa updated https://github.com/llvm/llvm-project/pull/88812
>From 095696411172034f80233f1722e293c1f458d67f Mon Sep 17 00:00:00 2001
From: Anthony Ha
Date: Mon, 15 Apr 2024 19:34:19 +
Subject: [PATCH 1/3] [lldb] Skip remote PutFile when MD5 hashes equal
---
.../gdb-server/PlatformRemoteGDBServer.cpp| 9 +
.../gdb-server/PlatformRemoteGDBServer.h | 3 ++
.../GDBRemoteCommunicationClient.cpp | 36 +--
lldb/source/Target/Platform.cpp | 30 +++-
.../GDBRemoteCommunicationClientTest.cpp | 23
5 files changed, 98 insertions(+), 3 deletions(-)
diff --git
a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
index 88f1ad15b6b485..d9f3e40019cf29 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
@@ -684,6 +684,15 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
+bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec _spec,
+ uint64_t , uint64_t ) {
+ if (IsConnected()) {
+// Note that high/low are switched in the gdb remote communication client
+return m_gdb_client_up->CalculateMD5(file_spec, high, low);
+ }
+ return false;
+}
+
void PlatformRemoteGDBServer::CalculateTrapHandlerSymbolNames() {
m_trap_handlers.push_back(ConstString("_sigtramp"));
}
diff --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
index 638f7db5ef800c..d83fc386f59427 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
@@ -146,6 +146,9 @@ class PlatformRemoteGDBServer : public Platform, private
UserIDResolver {
void CalculateTrapHandlerSymbolNames() override;
+ bool CalculateMD5(const FileSpec _spec, uint64_t ,
+uint64_t ) override;
+
const lldb::UnixSignalsSP () override;
size_t ConnectToWaitingProcesses(lldb_private::Debugger ,
diff --git
a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 1f6116967a4f64..8c79d5fecf12fe 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -3433,8 +3433,40 @@ bool GDBRemoteCommunicationClient::CalculateMD5(
return false;
if (response.Peek() && *response.Peek() == 'x')
return false;
-low = response.GetHexMaxU64(false, UINT64_MAX);
-high = response.GetHexMaxU64(false, UINT64_MAX);
+
+// GDBRemoteCommunicationServerCommon::Handle_vFile_MD5 concatenates low
and
+// high hex strings. We can't use response.GetHexMaxU64 because that can't
+// handle the concatenated hex string. What would happen is parsing the low
+// would consume the whole response packet - which is a bug. Instead, we
get
+// the byte string for each low and high hex separately, and parse them.
+//
+// An alternate way to handle this is to change the server to put a
+// delimiter between the low/high parts, and change the client to parse the
+// delimiter. However, we choose not to do this so existing lldb-servers
+// don't have to be patched
+
+// Get low part
+auto part = response.GetStringRef().substr(response.GetFilePos(),
+ sizeof(uint64_t) * 2);
+if (part.size() != sizeof(uint64_t) * 2)
+ return false;
+response.SetFilePos(response.GetFilePos() + part.size());
+
+bool conversionErrored = part.getAsInteger(16, low);
+if (conversionErrored)
+ return false;
+
+// Get high part
+part = response.GetStringRef().substr(response.GetFilePos(),
+ sizeof(uint64_t) * 2);
+if (part.size() != sizeof(uint64_t) * 2)
+ return false;
+response.SetFilePos(response.GetFilePos() + part.size());
+
+conversionErrored = part.getAsInteger(16, high);
+if (conversionErrored)
+ return false;
+
return true;
}
return false;
diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp
index 4ce290dfbe035f..cdbafb17a5df4d 100644
--- a/lldb/source/Target/Platform.cpp
+++ b/lldb/source/Target/Platform.cpp
@@ -1184,7 +1184,7 @@ bool Platform::IsCompatibleArchitecture(const ArchSpec
,
Status Platform::PutFile(const FileSpec , const FileSpec ,
uint32_t uid, uint32_t gid) {
Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(log, "[PutFile] Using block by block transfer\n");
+ LLDB_LOGF(log, "[PutFile] Using block
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
https://github.com/bulbazord commented: Looking better! Let's get the comments and style sorted out, but I think this is looking good :) https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -684,6 +684,14 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
+bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec _spec,
+ uint64_t , uint64_t ) {
+ if (!IsConnected()) {
+return false;
+ }
bulbazord wrote:
The LLVM coding standards suggest not having curly braces for 1 line bodies:
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1197,6 +1197,34 @@ Status Platform::PutFile(const FileSpec , const
FileSpec ,
if (!source_file)
return Status(source_file.takeError());
Status error;
+
+ bool requires_upload = true;
+ {
bulbazord wrote:
I see. I wouldn't say it's a bad thing to create a new scope if you want to
contain the lifetime of these variables to their own scope, but in LLDB (and
more broadly LLVM) this is mostly used for RAII. The most common instance of
this is when one wants to acquire a lock but does not want the lock to span the
lifetime of the method body.
For consistency I would say either put this code in the main body, put it in a
small static function, or put it in a lambda.
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
DavidSpickett wrote: @weliveindetail I think this might fix the problems you were having remote debugging clang-repl (I can't seem to find the actual Discourse thread). https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1197,6 +1197,34 @@ Status Platform::PutFile(const FileSpec , const
FileSpec ,
if (!source_file)
return Status(source_file.takeError());
Status error;
+
+ bool requires_upload = true;
+ {
DavidSpickett wrote:
If you really need a new scope, a small static function in the same file is the
usual way. In this case it's fine to have them in the scope of the main
function I think, I don't think they overlap with any existing var.
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1184,7 +1184,7 @@ bool Platform::IsCompatibleArchitecture(const ArchSpec
,
Status Platform::PutFile(const FileSpec , const FileSpec ,
uint32_t uid, uint32_t gid) {
Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(log, "[PutFile] Using block by block transfer\n");
+ LLDB_LOGF(log, "[PutFile] Using block by block transfer");
DavidSpickett wrote:
Since (I assume) you can't push directly, I suggest leaving this out of this PR
and once this PR lands immediately put up a PR just to change that \n.
It's maybe a bit strict but PRs are free :)
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -3433,8 +3433,40 @@ bool GDBRemoteCommunicationClient::CalculateMD5( return false; if (response.Peek() && *response.Peek() == 'x') return false; -low = response.GetHexMaxU64(false, UINT64_MAX); -high = response.GetHexMaxU64(false, UINT64_MAX); + +// GDBRemoteCommunicationServerCommon::Handle_vFile_MD5 concatenates low and +// high hex strings. We can't use response.GetHexMaxU64 because that can't +// handle the concatenated hex string. What would happen is parsing the low +// would consume the whole response packet - which is a bug. Instead, we get DavidSpickett wrote: Maybe a small wording change then "which is a bug" could be "which would give incorrect results". "is a bug" sounds a lot like we have a bug still. https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -684,6 +684,15 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
+bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec _spec,
+ uint64_t , uint64_t ) {
+ if (IsConnected()) {
Awfa wrote:
I'll change just this instance to meet your suggestion
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
https://github.com/Awfa updated https://github.com/llvm/llvm-project/pull/88812
>From 095696411172034f80233f1722e293c1f458d67f Mon Sep 17 00:00:00 2001
From: Anthony Ha
Date: Mon, 15 Apr 2024 19:34:19 +
Subject: [PATCH 1/2] [lldb] Skip remote PutFile when MD5 hashes equal
---
.../gdb-server/PlatformRemoteGDBServer.cpp| 9 +
.../gdb-server/PlatformRemoteGDBServer.h | 3 ++
.../GDBRemoteCommunicationClient.cpp | 36 +--
lldb/source/Target/Platform.cpp | 30 +++-
.../GDBRemoteCommunicationClientTest.cpp | 23
5 files changed, 98 insertions(+), 3 deletions(-)
diff --git
a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
index 88f1ad15b6b485..d9f3e40019cf29 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
@@ -684,6 +684,15 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
+bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec _spec,
+ uint64_t , uint64_t ) {
+ if (IsConnected()) {
+// Note that high/low are switched in the gdb remote communication client
+return m_gdb_client_up->CalculateMD5(file_spec, high, low);
+ }
+ return false;
+}
+
void PlatformRemoteGDBServer::CalculateTrapHandlerSymbolNames() {
m_trap_handlers.push_back(ConstString("_sigtramp"));
}
diff --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
index 638f7db5ef800c..d83fc386f59427 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
@@ -146,6 +146,9 @@ class PlatformRemoteGDBServer : public Platform, private
UserIDResolver {
void CalculateTrapHandlerSymbolNames() override;
+ bool CalculateMD5(const FileSpec _spec, uint64_t ,
+uint64_t ) override;
+
const lldb::UnixSignalsSP () override;
size_t ConnectToWaitingProcesses(lldb_private::Debugger ,
diff --git
a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 1f6116967a4f64..8c79d5fecf12fe 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -3433,8 +3433,40 @@ bool GDBRemoteCommunicationClient::CalculateMD5(
return false;
if (response.Peek() && *response.Peek() == 'x')
return false;
-low = response.GetHexMaxU64(false, UINT64_MAX);
-high = response.GetHexMaxU64(false, UINT64_MAX);
+
+// GDBRemoteCommunicationServerCommon::Handle_vFile_MD5 concatenates low
and
+// high hex strings. We can't use response.GetHexMaxU64 because that can't
+// handle the concatenated hex string. What would happen is parsing the low
+// would consume the whole response packet - which is a bug. Instead, we
get
+// the byte string for each low and high hex separately, and parse them.
+//
+// An alternate way to handle this is to change the server to put a
+// delimiter between the low/high parts, and change the client to parse the
+// delimiter. However, we choose not to do this so existing lldb-servers
+// don't have to be patched
+
+// Get low part
+auto part = response.GetStringRef().substr(response.GetFilePos(),
+ sizeof(uint64_t) * 2);
+if (part.size() != sizeof(uint64_t) * 2)
+ return false;
+response.SetFilePos(response.GetFilePos() + part.size());
+
+bool conversionErrored = part.getAsInteger(16, low);
+if (conversionErrored)
+ return false;
+
+// Get high part
+part = response.GetStringRef().substr(response.GetFilePos(),
+ sizeof(uint64_t) * 2);
+if (part.size() != sizeof(uint64_t) * 2)
+ return false;
+response.SetFilePos(response.GetFilePos() + part.size());
+
+conversionErrored = part.getAsInteger(16, high);
+if (conversionErrored)
+ return false;
+
return true;
}
return false;
diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp
index 4ce290dfbe035f..cdbafb17a5df4d 100644
--- a/lldb/source/Target/Platform.cpp
+++ b/lldb/source/Target/Platform.cpp
@@ -1184,7 +1184,7 @@ bool Platform::IsCompatibleArchitecture(const ArchSpec
,
Status Platform::PutFile(const FileSpec , const FileSpec ,
uint32_t uid, uint32_t gid) {
Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(log, "[PutFile] Using block by block transfer\n");
+ LLDB_LOGF(log, "[PutFile] Using block
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1197,6 +1197,34 @@ Status Platform::PutFile(const FileSpec , const
FileSpec ,
if (!source_file)
return Status(source_file.takeError());
Status error;
+
+ bool requires_upload = true;
+ {
+uint64_t dest_md5_low, dest_md5_high;
+bool success = CalculateMD5(destination, dest_md5_low, dest_md5_high);
+if (!success) {
+ LLDB_LOGF(log, "[PutFile] couldn't get md5 sum of destination");
+} else {
+ auto local_md5 = llvm::sys::fs::md5_contents(source.GetPath());
+ if (!local_md5) {
+LLDB_LOGF(log, "[PutFile] couldn't get md5 sum of source");
+ } else {
+uint64_t local_md5_high, local_md5_low;
+std::tie(local_md5_high, local_md5_low) = local_md5->words();
Awfa wrote:
Will do
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -3433,8 +3433,40 @@ bool GDBRemoteCommunicationClient::CalculateMD5( return false; if (response.Peek() && *response.Peek() == 'x') return false; -low = response.GetHexMaxU64(false, UINT64_MAX); -high = response.GetHexMaxU64(false, UINT64_MAX); + +// GDBRemoteCommunicationServerCommon::Handle_vFile_MD5 concatenates low and +// high hex strings. We can't use response.GetHexMaxU64 because that can't +// handle the concatenated hex string. What would happen is parsing the low +// would consume the whole response packet - which is a bug. Instead, we get +// the byte string for each low and high hex separately, and parse them. +// +// An alternate way to handle this is to change the server to put a +// delimiter between the low/high parts, and change the client to parse the +// delimiter. However, we choose not to do this so existing lldb-servers +// don't have to be patched + +// Get low part +auto part = response.GetStringRef().substr(response.GetFilePos(), + sizeof(uint64_t) * 2); Awfa wrote: Will do https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1197,6 +1197,34 @@ Status Platform::PutFile(const FileSpec , const
FileSpec ,
if (!source_file)
return Status(source_file.takeError());
Status error;
+
+ bool requires_upload = true;
+ {
Awfa wrote:
This is how I write code to scope the variables `dest_md5_low`,
`dest_md5_high`, `success` away from the main scope of the function.
Is this not good to do in the llvm-project?
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1184,7 +1184,7 @@ bool Platform::IsCompatibleArchitecture(const ArchSpec
,
Status Platform::PutFile(const FileSpec , const FileSpec ,
uint32_t uid, uint32_t gid) {
Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(log, "[PutFile] Using block by block transfer\n");
+ LLDB_LOGF(log, "[PutFile] Using block by block transfer");
Awfa wrote:
When testing this change, and using `log enable lldb platform` to read the
logs, I just noticed this had a `\n` and removed it since `LLDB_LOGF` already
adds a new line, and it didn't seem like most other logs had `\n`.
Is this bad to have in the patch?
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -684,6 +684,15 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
+bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec _spec,
+ uint64_t , uint64_t ) {
+ if (IsConnected()) {
+// Note that high/low are switched in the gdb remote communication client
Awfa wrote:
Good point - I don't see a reason why it is swapped there. I'll change it so
it's just consistent.
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -684,6 +684,15 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
+bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec _spec,
+ uint64_t , uint64_t ) {
+ if (IsConnected()) {
Awfa wrote:
Should I change the other functions in `PlatformRemoteGDBServer` to do the
same? For example `PlatformRemoteGDBServer::GetFileExists` does the same
currently.
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -3433,8 +3433,40 @@ bool GDBRemoteCommunicationClient::CalculateMD5( return false; if (response.Peek() && *response.Peek() == 'x') return false; -low = response.GetHexMaxU64(false, UINT64_MAX); -high = response.GetHexMaxU64(false, UINT64_MAX); + +// GDBRemoteCommunicationServerCommon::Handle_vFile_MD5 concatenates low and +// high hex strings. We can't use response.GetHexMaxU64 because that can't +// handle the concatenated hex string. What would happen is parsing the low +// would consume the whole response packet - which is a bug. Instead, we get Awfa wrote: `response.GetHexMaxU64` consumes the response stream until a non-valid hex character is encountered. This is just the wrong function to use because the server returns both the low/high parts concatenated. When it was used, what would happen is `low = response.GetHexMaxU64...` would consume the whole packet, when it should just be consuming the first half of it. This also leads to `high` being set to 0 because `low` consumed the entire packet. This part of the patch isn't a workaround, it's the fix. This fixes the client so it can correctly parse the server's response. https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -3433,8 +3433,40 @@ bool GDBRemoteCommunicationClient::CalculateMD5( return false; if (response.Peek() && *response.Peek() == 'x') return false; -low = response.GetHexMaxU64(false, UINT64_MAX); -high = response.GetHexMaxU64(false, UINT64_MAX); + +// GDBRemoteCommunicationServerCommon::Handle_vFile_MD5 concatenates low and +// high hex strings. We can't use response.GetHexMaxU64 because that can't +// handle the concatenated hex string. What would happen is parsing the low +// would consume the whole response packet - which is a bug. Instead, we get +// the byte string for each low and high hex separately, and parse them. +// +// An alternate way to handle this is to change the server to put a +// delimiter between the low/high parts, and change the client to parse the +// delimiter. However, we choose not to do this so existing lldb-servers +// don't have to be patched + +// Get low part +auto part = response.GetStringRef().substr(response.GetFilePos(), + sizeof(uint64_t) * 2); bulbazord wrote: Can you add some meaning to `sizeof(uint64_t) * 2`? It's used a lot, so giving it a name would help with the readability. https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -3433,8 +3433,40 @@ bool GDBRemoteCommunicationClient::CalculateMD5( return false; if (response.Peek() && *response.Peek() == 'x') return false; -low = response.GetHexMaxU64(false, UINT64_MAX); -high = response.GetHexMaxU64(false, UINT64_MAX); + +// GDBRemoteCommunicationServerCommon::Handle_vFile_MD5 concatenates low and +// high hex strings. We can't use response.GetHexMaxU64 because that can't +// handle the concatenated hex string. What would happen is parsing the low +// would consume the whole response packet - which is a bug. Instead, we get +// the byte string for each low and high hex separately, and parse them. +// +// An alternate way to handle this is to change the server to put a +// delimiter between the low/high parts, and change the client to parse the +// delimiter. However, we choose not to do this so existing lldb-servers +// don't have to be patched + +// Get low part +auto part = response.GetStringRef().substr(response.GetFilePos(), + sizeof(uint64_t) * 2); +if (part.size() != sizeof(uint64_t) * 2) + return false; +response.SetFilePos(response.GetFilePos() + part.size()); + +bool conversionErrored = part.getAsInteger(16, low); +if (conversionErrored) bulbazord wrote: suggestion: `if (part.getAsInteger(/*radix=*/16, low))` https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -684,6 +684,15 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
+bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec _spec,
+ uint64_t , uint64_t ) {
+ if (IsConnected()) {
+// Note that high/low are switched in the gdb remote communication client
bulbazord wrote:
It's good that you have a comment for this because it "feels" wrong. But why
are `high/low` swapped in the gdb remote communication client in the first
place? Is that intentional or is this working around a bug?
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1197,6 +1197,34 @@ Status Platform::PutFile(const FileSpec , const
FileSpec ,
if (!source_file)
return Status(source_file.takeError());
Status error;
+
+ bool requires_upload = true;
+ {
+uint64_t dest_md5_low, dest_md5_high;
+bool success = CalculateMD5(destination, dest_md5_low, dest_md5_high);
+if (!success) {
+ LLDB_LOGF(log, "[PutFile] couldn't get md5 sum of destination");
+} else {
+ auto local_md5 = llvm::sys::fs::md5_contents(source.GetPath());
+ if (!local_md5) {
+LLDB_LOGF(log, "[PutFile] couldn't get md5 sum of source");
+ } else {
+uint64_t local_md5_high, local_md5_low;
+std::tie(local_md5_high, local_md5_low) = local_md5->words();
bulbazord wrote:
LLDB uses c++17 so you can use structured binding for this.
```
const auto [local_md5_high, local_md5_low] = local_md5->words();
```
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1184,7 +1184,7 @@ bool Platform::IsCompatibleArchitecture(const ArchSpec
,
Status Platform::PutFile(const FileSpec , const FileSpec ,
uint32_t uid, uint32_t gid) {
Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(log, "[PutFile] Using block by block transfer\n");
+ LLDB_LOGF(log, "[PutFile] Using block by block transfer");
bulbazord wrote:
Why did this get changed?
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -1197,6 +1197,34 @@ Status Platform::PutFile(const FileSpec , const
FileSpec ,
if (!source_file)
return Status(source_file.takeError());
Status error;
+
+ bool requires_upload = true;
+ {
bulbazord wrote:
Why is this all in its own block?
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -3433,8 +3433,40 @@ bool GDBRemoteCommunicationClient::CalculateMD5( return false; if (response.Peek() && *response.Peek() == 'x') return false; -low = response.GetHexMaxU64(false, UINT64_MAX); -high = response.GetHexMaxU64(false, UINT64_MAX); + +// GDBRemoteCommunicationServerCommon::Handle_vFile_MD5 concatenates low and +// high hex strings. We can't use response.GetHexMaxU64 because that can't +// handle the concatenated hex string. What would happen is parsing the low +// would consume the whole response packet - which is a bug. Instead, we get bulbazord wrote: Why not fix the bug instead of working around it here? What causes the bug? https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
@@ -684,6 +684,15 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
+bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec _spec,
+ uint64_t , uint64_t ) {
+ if (IsConnected()) {
bulbazord wrote:
Suggestion: Flip the condition and do an early return with false.
https://github.com/llvm/llvm-project/pull/88812
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
llvmbot wrote:
@llvm/pr-subscribers-lldb
Author: Anthony Ha (Awfa)
Changes
This PR adds a check within `PutFile` to exit early when both local and
destination files have matching MD5 hashes. If they differ, or there is trouble
getting the hashes, the regular code path to put the file is run.
As I needed this to talk to an `lldb-server` which runs the gdb-remote
protocol, I enabled `CalculateMD5` within `Platform/gdb-server` and also found
and fixed a parsing bug within it as well. Before this PR, the client is
incorrectly parsing the response packet containing the checksum; after this PR,
hopefully this is fixed. There is a test for the parsing behavior included in
this PR.
---
Full diff: https://github.com/llvm/llvm-project/pull/88812.diff
5 Files Affected:
- (modified)
lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp (+9)
- (modified) lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
(+3)
- (modified)
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp (+34-2)
- (modified) lldb/source/Target/Platform.cpp (+29-1)
- (modified)
lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp (+23)
``diff
diff --git
a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
index 88f1ad15b6b485..d9f3e40019cf29 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
@@ -684,6 +684,15 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
+bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec _spec,
+ uint64_t , uint64_t ) {
+ if (IsConnected()) {
+// Note that high/low are switched in the gdb remote communication client
+return m_gdb_client_up->CalculateMD5(file_spec, high, low);
+ }
+ return false;
+}
+
void PlatformRemoteGDBServer::CalculateTrapHandlerSymbolNames() {
m_trap_handlers.push_back(ConstString("_sigtramp"));
}
diff --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
index 638f7db5ef800c..d83fc386f59427 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
@@ -146,6 +146,9 @@ class PlatformRemoteGDBServer : public Platform, private
UserIDResolver {
void CalculateTrapHandlerSymbolNames() override;
+ bool CalculateMD5(const FileSpec _spec, uint64_t ,
+uint64_t ) override;
+
const lldb::UnixSignalsSP () override;
size_t ConnectToWaitingProcesses(lldb_private::Debugger ,
diff --git
a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 1f6116967a4f64..8c79d5fecf12fe 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -3433,8 +3433,40 @@ bool GDBRemoteCommunicationClient::CalculateMD5(
return false;
if (response.Peek() && *response.Peek() == 'x')
return false;
-low = response.GetHexMaxU64(false, UINT64_MAX);
-high = response.GetHexMaxU64(false, UINT64_MAX);
+
+// GDBRemoteCommunicationServerCommon::Handle_vFile_MD5 concatenates low
and
+// high hex strings. We can't use response.GetHexMaxU64 because that can't
+// handle the concatenated hex string. What would happen is parsing the low
+// would consume the whole response packet - which is a bug. Instead, we
get
+// the byte string for each low and high hex separately, and parse them.
+//
+// An alternate way to handle this is to change the server to put a
+// delimiter between the low/high parts, and change the client to parse the
+// delimiter. However, we choose not to do this so existing lldb-servers
+// don't have to be patched
+
+// Get low part
+auto part = response.GetStringRef().substr(response.GetFilePos(),
+ sizeof(uint64_t) * 2);
+if (part.size() != sizeof(uint64_t) * 2)
+ return false;
+response.SetFilePos(response.GetFilePos() + part.size());
+
+bool conversionErrored = part.getAsInteger(16, low);
+if (conversionErrored)
+ return false;
+
+// Get high part
+part = response.GetStringRef().substr(response.GetFilePos(),
+ sizeof(uint64_t) * 2);
+if (part.size() != sizeof(uint64_t) * 2)
+ return false;
+response.SetFilePos(response.GetFilePos() + part.size());
+
+conversionErrored = part.getAsInteger(16, high);
+if (conversionErrored)
+ return false;
+
return true;
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
github-actions[bot] wrote: Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using `@` followed by their GitHub username. If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the [LLVM GitHub User Guide](https://llvm.org/docs/GitHub.html). You can also ask questions in a comment on this PR, on the [LLVM Discord](https://discord.com/invite/xS7Z362) or on the [forums](https://discourse.llvm.org/). https://github.com/llvm/llvm-project/pull/88812 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Skip remote PutFile when MD5 hashes equal (PR #88812)
https://github.com/Awfa created https://github.com/llvm/llvm-project/pull/88812
This PR adds a check within `PutFile` to exit early when both local and
destination files have matching MD5 hashes. If they differ, or there is trouble
getting the hashes, the regular code path to put the file is run.
As I needed this to talk to an `lldb-server` which runs the gdb-remote
protocol, I enabled `CalculateMD5` within `Platform/gdb-server` and also found
and fixed a parsing bug within it as well. Before this PR, the client is
incorrectly parsing the response packet containing the checksum; after this PR,
hopefully this is fixed. There is a test for the parsing behavior included in
this PR.
>From 095696411172034f80233f1722e293c1f458d67f Mon Sep 17 00:00:00 2001
From: Anthony Ha
Date: Mon, 15 Apr 2024 19:34:19 +
Subject: [PATCH] [lldb] Skip remote PutFile when MD5 hashes equal
---
.../gdb-server/PlatformRemoteGDBServer.cpp| 9 +
.../gdb-server/PlatformRemoteGDBServer.h | 3 ++
.../GDBRemoteCommunicationClient.cpp | 36 +--
lldb/source/Target/Platform.cpp | 30 +++-
.../GDBRemoteCommunicationClientTest.cpp | 23
5 files changed, 98 insertions(+), 3 deletions(-)
diff --git
a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
index 88f1ad15b6b485..d9f3e40019cf29 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
@@ -684,6 +684,15 @@ Status PlatformRemoteGDBServer::RunShellCommand(
signo_ptr, command_output, timeout);
}
+bool PlatformRemoteGDBServer::CalculateMD5(const FileSpec _spec,
+ uint64_t , uint64_t ) {
+ if (IsConnected()) {
+// Note that high/low are switched in the gdb remote communication client
+return m_gdb_client_up->CalculateMD5(file_spec, high, low);
+ }
+ return false;
+}
+
void PlatformRemoteGDBServer::CalculateTrapHandlerSymbolNames() {
m_trap_handlers.push_back(ConstString("_sigtramp"));
}
diff --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
index 638f7db5ef800c..d83fc386f59427 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
@@ -146,6 +146,9 @@ class PlatformRemoteGDBServer : public Platform, private
UserIDResolver {
void CalculateTrapHandlerSymbolNames() override;
+ bool CalculateMD5(const FileSpec _spec, uint64_t ,
+uint64_t ) override;
+
const lldb::UnixSignalsSP () override;
size_t ConnectToWaitingProcesses(lldb_private::Debugger ,
diff --git
a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 1f6116967a4f64..8c79d5fecf12fe 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -3433,8 +3433,40 @@ bool GDBRemoteCommunicationClient::CalculateMD5(
return false;
if (response.Peek() && *response.Peek() == 'x')
return false;
-low = response.GetHexMaxU64(false, UINT64_MAX);
-high = response.GetHexMaxU64(false, UINT64_MAX);
+
+// GDBRemoteCommunicationServerCommon::Handle_vFile_MD5 concatenates low
and
+// high hex strings. We can't use response.GetHexMaxU64 because that can't
+// handle the concatenated hex string. What would happen is parsing the low
+// would consume the whole response packet - which is a bug. Instead, we
get
+// the byte string for each low and high hex separately, and parse them.
+//
+// An alternate way to handle this is to change the server to put a
+// delimiter between the low/high parts, and change the client to parse the
+// delimiter. However, we choose not to do this so existing lldb-servers
+// don't have to be patched
+
+// Get low part
+auto part = response.GetStringRef().substr(response.GetFilePos(),
+ sizeof(uint64_t) * 2);
+if (part.size() != sizeof(uint64_t) * 2)
+ return false;
+response.SetFilePos(response.GetFilePos() + part.size());
+
+bool conversionErrored = part.getAsInteger(16, low);
+if (conversionErrored)
+ return false;
+
+// Get high part
+part = response.GetStringRef().substr(response.GetFilePos(),
+ sizeof(uint64_t) * 2);
+if (part.size() != sizeof(uint64_t) * 2)
+ return false;
+response.SetFilePos(response.GetFilePos() + part.size());
+
+conversionErrored = part.getAsInteger(16, high);
+if
