Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1384363002:
[NOT TO COMMIT YET] Using proc/pid/status file contents to get process metrics in linux. (Closed)
|
Created:
5 years, 2 months ago by ssid Modified:
4 years, 9 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, rickyz (no longer on Chrome), picksi1 Base URL:
https://chromium.googlesource.com/chromium/src.git@smaps_fscan Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUsing proc/pid/status file contents to get process metrics in linux.
The sandboxed processes cannot read /proc directory to get the process
metrics (working set size and peak) in linux. So, to work around this
problem the browser sends the stats file data to renderer and the
renderer needs to parse it. So, this CL adds api to process metrics to
parse and get the metrics.
BUG=461788
Patch Set 1 #Patch Set 2 : Renames and fixes. #
Total comments: 12
Patch Set 3 : Change the api. #
Total comments: 1
Patch Set 4 : Remove the trace_event changes. #
Total comments: 11
Patch Set 5 : Fixes. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 26 (9 generated)
ssid@chromium.org changed reviewers: + thestig@chromium.org
+thestig I need to use the linux specific parsing code that exists in ProcessMetrics. But the GetWorkingSetSize reads file and parses. I have to add if defines since this is specific for linux only. I could introduce a process_metrics_linux.h maybe, but only for the 2 functions? or I could just write the code again to parse the file, where ever I need it. WDYT is the best way to do this? Please see the file process_metrics.h. Thanks
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #2 (id:60001) has been deleted
I'll take a look at this soon. +Linux sandboxing folks FYI
On 2015/10/06 20:29:30, Lei Zhang wrote: > I'll take a look at this soon. > > +Linux sandboxing folks FYI This CL does not actually get the file from browser. It adds support to parse if I get the descriptor. Passing the descriptor will be done in later CL, and will get that reviewed by the sandboxing and security folks. Note: the ProcessMemoryTotalsDumpProvider::proc_status_fd is never set here. Just added as placeholder to show how the new api will be used, and will be set in future.
On 2015/10/06 20:50:13, ssid wrote: > On 2015/10/06 20:29:30, Lei Zhang wrote: > > I'll take a look at this soon. > > > > +Linux sandboxing folks FYI > > This CL does not actually get the file from browser. It adds support to parse if > I get the descriptor. > Passing the descriptor will be done in later CL, and will get that reviewed by > the sandboxing and security folks. > Note: the ProcessMemoryTotalsDumpProvider::proc_status_fd is never set here. > Just added as placeholder to show how the new api will be used, and will be set > in future. How do you feel about merging this with https://codereview.chromium.org/1329273002 ? The CLs are small enough that combining them won't be overwhelming.
On 2015/10/06 20:57:07, Lei Zhang wrote: > How do you feel about merging this with > https://codereview.chromium.org/1329273002 ? The CLs are small enough that > combining them won't be overwhelming. I'll review this in either case.
On 2015/10/06 20:57:07, Lei Zhang wrote: > On 2015/10/06 20:50:13, ssid wrote: > > On 2015/10/06 20:29:30, Lei Zhang wrote: > > > I'll take a look at this soon. > > > > > > +Linux sandboxing folks FYI > > > > This CL does not actually get the file from browser. It adds support to parse > if > > I get the descriptor. > > Passing the descriptor will be done in later CL, and will get that reviewed by > > the sandboxing and security folks. > > Note: the ProcessMemoryTotalsDumpProvider::proc_status_fd is never set here. > > Just added as placeholder to show how the new api will be used, and will be > set > > in future. > > How do you feel about merging this with > https://codereview.chromium.org/1329273002 ? The CLs are small enough that > combining them won't be overwhelming. I was only worried about the review time. It will be difficult for you to review not very related changes. I split this since I was not sure if you will be fine with a linux specific code in ProcessMetrics. If you are fine with such addition I can merge it. If you feel it is not, then I have to copy the parsing code where required or do something else. The point was that security folks will be filled with a base api change review comments.
On 2015/10/06 21:06:52, ssid wrote: > On 2015/10/06 20:57:07, Lei Zhang wrote: > > On 2015/10/06 20:50:13, ssid wrote: > > > On 2015/10/06 20:29:30, Lei Zhang wrote: > > > > I'll take a look at this soon. > > > > > > > > +Linux sandboxing folks FYI > > > > > > This CL does not actually get the file from browser. It adds support to > parse > > if > > > I get the descriptor. > > > Passing the descriptor will be done in later CL, and will get that reviewed > by > > > the sandboxing and security folks. > > > Note: the ProcessMemoryTotalsDumpProvider::proc_status_fd is never set here. > > > Just added as placeholder to show how the new api will be used, and will be > > set > > > in future. > > > > How do you feel about merging this with > > https://codereview.chromium.org/1329273002 ? The CLs are small enough that > > combining them won't be overwhelming. > > I was only worried about the review time. It will be difficult for you to review > not very related changes. I split this since I was not sure if you will be fine > with a linux specific code in ProcessMetrics. If you are fine with such addition > I can merge it. If you feel it is not, then I have to copy the parsing code > where required or do something else. > The point was that security folks will be filled with a base api change review > comments. I will wait till the other CL gets approved to land this CL since this change will be pointless without that.
https://codereview.chromium.org/1384363002/diff/140001/base/process/process_m... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/1384363002/diff/140001/base/process/process_m... base/process/process_metrics_linux.cc:53: size_t ParseProcStatsAndGetFieldAsSizeT(const char* status_contents, Just use StringPiece instead of const char* ? Then you can pass in a std::string or const char*, without needing to do .c_str(). https://codereview.chromium.org/1384363002/diff/140001/base/process/process_m... base/process/process_metrics_linux.cc:97: size_t ReadProcStatusFdAndGetFieldAsSizeT(PlatformFile proc_status_fd, /proc/<pid>/status isn't that big, right? Why bother passing the fd and not just the contents? I can see using a fd for smaps, which IIRC, is quite large. https://codereview.chromium.org/1384363002/diff/140001/base/process/process_m... base/process/process_metrics_linux.cc:99: const size_t kMaxStatFileSize = 1 << 12; Easier to just write 4096? https://codereview.chromium.org/1384363002/diff/140001/base/process/process_m... base/process/process_metrics_linux.cc:102: lseek(proc_status_fd, 0, SEEK_SET); Check return value? https://codereview.chromium.org/1384363002/diff/140001/base/process/process_m... base/process/process_metrics_linux.cc:103: ReadFromFD(proc_status_fd, status_data, kMaxStatFileSize); Ditto, check this return value in place of the next line. https://codereview.chromium.org/1384363002/diff/140001/base/trace_event/proce... File base/trace_event/process_memory_totals_dump_provider.cc (right): https://codereview.chromium.org/1384363002/diff/140001/base/trace_event/proce... base/trace_event/process_memory_totals_dump_provider.cc:63: #if defined(OS_LINUX) I haven't thought about this much, but if there's a way to separate the #ifdefs from being intertwined with the if-else statements, that would be a lot cleaner.
I have changed and moved the api. now calling it ParseProcStatus... Moved it along with linux specific parse apis. Moved the file read to tracing file. About the ifdef in the trace_event file, I will discuss it on the next CL and do what the trace_event owners think is the best. PTAL thanks https://codereview.chromium.org/1384363002/diff/140001/base/process/process_m... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/1384363002/diff/140001/base/process/process_m... base/process/process_metrics_linux.cc:53: size_t ParseProcStatsAndGetFieldAsSizeT(const char* status_contents, On 2015/10/06 21:09:00, Lei Zhang wrote: > Just use StringPiece instead of const char* ? Then you can pass in a std::string > or const char*, without needing to do .c_str(). changed to string https://codereview.chromium.org/1384363002/diff/140001/base/process/process_m... base/process/process_metrics_linux.cc:97: size_t ReadProcStatusFdAndGetFieldAsSizeT(PlatformFile proc_status_fd, On 2015/10/06 21:09:00, Lei Zhang wrote: > /proc/<pid>/status isn't that big, right? Why bother passing the fd and not just > the contents? I can see using a fd for smaps, which IIRC, is quite large. yes, I moved the reading part to other file and not the api takes string. https://codereview.chromium.org/1384363002/diff/140001/base/process/process_m... base/process/process_metrics_linux.cc:99: const size_t kMaxStatFileSize = 1 << 12; On 2015/10/06 21:09:00, Lei Zhang wrote: > Easier to just write 4096? Done. https://codereview.chromium.org/1384363002/diff/140001/base/process/process_m... base/process/process_metrics_linux.cc:102: lseek(proc_status_fd, 0, SEEK_SET); On 2015/10/06 21:09:00, Lei Zhang wrote: > Check return value? Done. https://codereview.chromium.org/1384363002/diff/140001/base/process/process_m... base/process/process_metrics_linux.cc:103: ReadFromFD(proc_status_fd, status_data, kMaxStatFileSize); On 2015/10/06 21:09:00, Lei Zhang wrote: > Ditto, check this return value in place of the next line. Done. https://codereview.chromium.org/1384363002/diff/140001/base/trace_event/proce... File base/trace_event/process_memory_totals_dump_provider.cc (right): https://codereview.chromium.org/1384363002/diff/140001/base/trace_event/proce... base/trace_event/process_memory_totals_dump_provider.cc:63: #if defined(OS_LINUX) On 2015/10/06 21:09:00, Lei Zhang wrote: > I haven't thought about this much, but if there's a way to separate the #ifdefs > from being intertwined with the if-else statements, that would be a lot cleaner. I cannot remove the ifdef totally since proc_status_file makes sense only in case of linux. I removed the ifdef around the member, but the variable will only be set in linux / android. This is so that I can move the ifdef here inside the else instead of having ifdef around else statement. The inner ifdef cannot be remove since ParseStatus... is defined only in linux/android.
primiano@chromium.org changed reviewers: + primiano@chromium.org
Some general comments: - I'd not merge this with https://codereview.chromium.org/1329273002 as that one requires a security review and I'd keep it as smaller as we can to ease security folk's job. - Feels a bit odd to me (but probably I'm missing some prior discussion) that process_memory_totals_dump_provider.cc has to read fully the proc file on its own (before the read was doing in base), and then ask base/ to extract all the fields re-parsing the string over and over every time. Maybe we can figure out a better API here. - Can I be CC-ed to CLs touching memory-infra code plz? Just happened to discover this by randomly looking at tracing-reviews. https://codereview.chromium.org/1384363002/diff/160001/base/trace_event/proce... File base/trace_event/process_memory_totals_dump_provider.cc (right): https://codereview.chromium.org/1384363002/diff/160001/base/trace_event/proce... base/trace_event/process_memory_totals_dump_provider.cc:43: int ret = fread(buffer, 1, sizeof(buffer), status_file); Use base::ReadFromFD, which is protected from EINTR
On 2015/10/07 10:25:34, Primiano Tucci wrote: > Some general comments: > - I'd not merge this with https://codereview.chromium.org/1329273002 as that > one requires a security review and I'd keep it as smaller as we can to ease > security folk's job. Yes, that is why I made this CL. > - Feels a bit odd to me (but probably I'm missing some prior discussion) that > process_memory_totals_dump_provider.cc has to read fully the proc file on its > own (before the read was doing in base), and then ask base/ to extract all the > fields re-parsing the string over and over every time. Maybe we can figure out a > better API here. Summary of the discussion: thestig@ did not like the idea of passing a file descriptor to the api in process metrics. He felt passing the string is better api. In order to make the api to get strings, i have to do the file read in trace_event. > - Can I be CC-ed to CLs touching memory-infra code plz? Just happened to > discover this by randomly looking at tracing-reviews. > I did not include you since at the beginning the CL just had changes in base/process. I had only added a place holder variable in the trace_event files. So, I sent review only to thestig@. But since he wanted to change the api to get string, I had to move the reading code to trace_event/. I was hoping the other CL (https://codereview.chromium.org/1329273002 ) I sent to you will get reviewed and I willupdate this file and clean up as you expect. I was going to land this CL only when the other gets approved, since the api is not useful without that one. > https://codereview.chromium.org/1384363002/diff/160001/base/trace_event/proce... > File base/trace_event/process_memory_totals_dump_provider.cc (right): > > https://codereview.chromium.org/1384363002/diff/160001/base/trace_event/proce... > base/trace_event/process_memory_totals_dump_provider.cc:43: int ret = > fread(buffer, 1, sizeof(buffer), status_file); > Use base::ReadFromFD, which is protected from EINTR ReadFromFD takes a file pointer, and useful only in linux. Now I am storing a FILE* and not fd. So, I would have to get fileno and then use ReadFromFD. Hm, the issue why I can't store just the int of file descriptor is that I have to ifdef the member itself and this means that I have to ifdef the "else" condition which checks for the member. This looks weird as thestig@ pointed out. That is the reason I removed the ifdef guard around the proc_status_file member and made it FILE* instead of FD. Again This was supposed to be a discussion in the https://codereview.chromium.org/1329273002, and I will make changes to this one with what is decided over there. Now it feels like I have to merge the CLs or remove the changes in trace_event/ and just have the api in this CL.
I have removed the trace_event changes and just adding the api in this CL. Lets move the discussion about how to store the files in https://codereview.chromium.org/1384363002/. Sorry for the confusion.
https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... File base/process/process_metrics.h (right): https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... base/process/process_metrics.h:313: BASE_EXPORT bool ParseProcStatusAndGetField( Why not #if defined(OS_LINUX) // and possible defined(OS_ANDROID) if needed there? https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... base/process/process_metrics.h:314: const std::string& proc_status_contents, Why not StringPiece? https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... base/process/process_metrics_linux.cc:63: CHECK(ParseProcStatusAndGetField(status, field, &value)); CHECK() is a bit strong. Any project that uses this code will crash if a /proc/<pid>/status file is ever malformed. https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... File base/process/process_metrics_unittest.cc (right): https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... base/process/process_metrics_unittest.cc:273: std::string valid_status_contents = const char kValindStatusContents[]; // and you can probably shorten it a bit without too much loss. https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... base/process/process_metrics_unittest.cc:320: EXPECT_EQ(vm_size, 7896 * 1024u); nit: EXPECT_EQ(expected, actual); // Otherwise on failure the gtest message is flipped and confusing.
Patchset #5 (id:200001) has been deleted
Thanks, PTAL https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... File base/process/process_metrics.h (right): https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... base/process/process_metrics.h:313: BASE_EXPORT bool ParseProcStatusAndGetField( On 2015/10/07 16:10:54, Lei Zhang wrote: > Why not #if defined(OS_LINUX) // and possible defined(OS_ANDROID) if needed > there? It is expected that it will be needed in future since the seccomp-bpf will be enabled on android and this work around will be needed. https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... base/process/process_metrics.h:314: const std::string& proc_status_contents, On 2015/10/07 16:10:54, Lei Zhang wrote: > Why not StringPiece? Hm yes, changed. https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... base/process/process_metrics_linux.cc:63: CHECK(ParseProcStatusAndGetField(status, field, &value)); On 2015/10/07 16:10:54, Lei Zhang wrote: > CHECK() is a bit strong. Any project that uses this code will crash if a > /proc/<pid>/status file is ever malformed. Sorry I beleived NOTREACHED was CHECK. Just realized it is DCHECK. fixed it. https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... File base/process/process_metrics_unittest.cc (right): https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... base/process/process_metrics_unittest.cc:273: std::string valid_status_contents = On 2015/10/07 16:10:54, Lei Zhang wrote: > const char kValindStatusContents[]; // and you can probably shorten it a bit > without too much loss. I just followed the pattern in the file to stay consistent. It used std::string. Fixed it. https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... base/process/process_metrics_unittest.cc:320: EXPECT_EQ(vm_size, 7896 * 1024u); On 2015/10/07 16:10:54, Lei Zhang wrote: > nit: EXPECT_EQ(expected, actual); // Otherwise on failure the gtest message is > flipped and confusing. sorry, fixed.
The CL description needs to be updated. Otherwise LGTM. https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... File base/process/process_metrics.h (right): https://codereview.chromium.org/1384363002/diff/180001/base/process/process_m... base/process/process_metrics.h:313: BASE_EXPORT bool ParseProcStatusAndGetField( On 2015/10/07 17:11:34, ssid wrote: > On 2015/10/07 16:10:54, Lei Zhang wrote: > > Why not #if defined(OS_LINUX) // and possible defined(OS_ANDROID) if needed > > there? > > It is expected that it will be needed in future since the seccomp-bpf will be > enabled on android and this work around will be needed. It turned out I didn't sleep enough last night and I had trouble matching #ifs to #endifs all day.
When you add the actual IPC, please make sure that you restrict which field of "status" you allow to access. A lot of these fields are technically info leaks (in an ideal world, renderers wouldn't even know their own "external pid" and we're slowly moving towards that). Also of course, there is no telling how this file will evolve over time and what info it'll contain: so let's have a (hopefully small) whitelist of fields.
On 2015/10/19 19:18:43, jln (slow on Chromium) wrote: > When you add the actual IPC, please make sure that you restrict which field of > "status" you allow to access. A lot of these fields are technically info leaks > (in an ideal world, renderers wouldn't even know their own "external pid" and > we're slowly moving towards that). > > Also of course, there is no telling how this file will evolve over time and what > info it'll contain: so let's have a (hopefully small) whitelist of fields. We had an internal discussion and I think we are going to make simpler and at the same time stronger from the security viewpoint. The plan is that this data will never reach back the renderer (no IPC at all). The way it's going to work is that the browser process is going to create the memory dumps and add to the (browser process) trace annotating this with a "this is really for the renderer X), so all the data will stay in the browser. I think this CL still applies though, for the following reason: On Linux desktop, the browser can open directly /proc/PID_OF_RENDERER/status and friends with no problem. On Android it cannot, as renderers have a separate uid namespace (due to the isolated_process label). Right now each renderer can open its own /proc/self/status and everything is fine. But once Android seccomp-bpf will be on, this should be changes as follows: renderers are going to hand to the browser the fd to their own /proc/self/status, and the browser will open and parse it. But at that point there should be no problem for the browser to access arbitrary fields in /proc/.../status for the renderers, as they will be consumed only in the browser. Does it make sense? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
