labath added a comment.
Thank you for taking the time to write the test. Just a couple of more comments
on things I noticed when going through this again:
================
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3172
+ if (custom_params)
+ json_packet.AddItem("jparams", custom_params);
+
----------------
I don't think we need the "j" in the "jparams" now that the whole packet is
json.
================
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3288
+
+ json_dict->GetValueForKeyAsInteger<uint64_t>("type", type);
+ options.setType(static_cast<lldb::TraceType>(type));
----------------
I'm wondering whether we should be sending the trace type as an opaque number
-- it's much easier to allocate IDs and manage compatibility if this is a
string field. Greg, what do you think?
================
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1142
+ StructuredData::ObjectSP custom_params_sp =
json_dict->GetValueForKey("jparams");
+ options.setTraceParams(static_pointer_cast<StructuredData::Dictionary>
(custom_params_sp));
+
----------------
What if jparams **isn't** a dictionary?
================
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1298
+ std::vector<uint8_t> buffer(byte_count, '\0');
+ if (buffer.empty())
+ return SendErrorResponse(0x78);
----------------
this is dead code. empty will never return true here. The process will just get
killed if it fails to allocate.
However, you are right that we should check the allocation as we are allocating
a buffer of user-provided size. I recommend explicitly allocating a uint8_t[]
using nothrow new. It doesn't stop us from getting killed if the kernel
overcommits, but it's better than nothing.
Alternatively you could also change the API to leave the memory allocation to
the callee, which will be in a better position do determine whether the size is
reasonable.
https://reviews.llvm.org/D32585
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
