[webkit-reviews] review denied: [Bug 210346] [GTK][WPE] Replace evil strtok() calls with fscanf() in MemoryPressureMonitor.cpp : [Attachment 396217] patch

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has denied Pablo Saavedra
<psaavedra at igalia.com>'s request for review:
Bug 210346: [GTK][WPE] Replace evil strtok() calls with fscanf() in
MemoryPressureMonitor.cpp
https://bugs.webkit.org/show_bug.cgi?id=210346

Attachment 396217: patch

https://bugs.webkit.org/attachment.cgi?id=396217&action=review




--- Comment #3 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 396217
  --> https://bugs.webkit.org/attachment.cgi?id=396217
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396217&action=review

I think this is going in the right direction (thanks!) but I have
a few questions and comment, please check them below ��️

>> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:66
>>  static size_t lowWatermarkPages(FILE* zoneInfoFile)
> 
> See notes in comment #1 from 210345
https://bugs.webkit.org/show_bug.cgi?id=210345#c1. Probably we want just to
remove this code because memAvailable is already in /proc/meminfo since Linux
3.14 LTS and this is already EOL since 2016 so there is no reason to maintain
this code.

We would like to decide whether we want to keep supporting kernels
older than 3.14 — but I think it's better to play safe and keep the
code for now.

If (or when) we decide to remove it, we can use a new bug for that.

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:77
> +	   r = fscanf(zoneInfoFile, " Node %*u, zone %128[^\n]\n", buffer);

Well, 128 bytes sounds a bit too much for the zone names, but at the same time
it's not that much… so let's just leave it as is ��️

Note that you should use “%127[^\n]” because fscanf() will read at most 127
characters and *then* add the '\0' terminator, which is character number 128.
When using fscanf() I find it clearer to use something like:

   char buffer[128 + 1];  // +1 makes it clearer that we need space for \0
   fscanf(f, "%128s", buffer);

(Actually, you did just this below!)

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:80
> +	   r = fscanf(zoneInfoFile, "%s", buffer);

This is unsafe, please indicate a field width corresponding to the
size of the buffer as part of the format string:

  r = fscanf(zoneInfoFile, "%127s", buffer);
			     ^^^

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:81
> +	   if (r == 1 && inNormalZone && !strcmp(buffer, "low")) {

How does this work? The “low” keyword is a few tokens *after* the zone name.
Wouldn't you need a nested loop here reading tokens until “low” is seen?

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:168
> +	   // int scanResult = fscanf(cgroupControllerFile,
"%*u:%40[^:]:%4096[^\n]", name, path);

Please remove this commented statement.