Show year as start time for processes older than a year

[arza-zara]

Old format is month+day which can be ambiguous.

Not sure what to do with this compilation warning:

Process.c: In function ‘Process_fillStarttimeBuffer’:                                                                                                    
Process.c:288:4: warning: format not a string literal, format string not checked [-Wformat-nonliteral]
  288 |    strftime(this->starttime_show, sizeof(this->starttime_show) - 1, fmt, &date);

[BenBE]

That warning says, that when calling strftime you must provide a string literal as part of the call itself instead of providing it through a variable. This is to protect against a class of format string vulnerabilities where the format string might be under control of an attacker. In this case the warning is a false positive, but nonetheless it's sound advise to have this argument always be strictly a literal or an expression yielding a literal (as is done in the old version.

On the actual change: I see the problem you're addressing but I'm not sure the current suggested patch is a sensible way to go forward with it.

[Explorer09]

Just a note: I tried this workaround in Compiler Explorer but it results in a slightly larger code (three assembly call instructions instead of merged to one).
I think we are better ignore that warning now, or report to the compiler developers that this looks like a bug (false positive).

   if (this->starttime_ctime > now - 86400) {
      strftime(this->starttime_show, sizeof(this->starttime_show) - 1, "%R ", &date);
   } else if (this->starttime_ctime > now - 364*86400) {
      strftime(this->starttime_show, sizeof(this->starttime_show) - 1, "%b%d ", &date);
   } else {
      strftime(this->starttime_show, sizeof(this->starttime_show) - 1, " %Y ", &date);
   }

[cgzones]

Suggestion to please the compiler:

void Process_fillStarttimeBuffer(Process* this) {
   struct tm date;
   time_t now = time(NULL);
   (void) localtime_r(&this->starttime_ctime, &date);

   strftime(this->starttime_show,
            sizeof(this->starttime_show) - 1,
            (this->starttime_ctime > now - 86400) ? "%R " : (this->starttime_ctime > now - 364*86400) ? "%b%d " : " %Y ",
            &date);
}

[BenBE]

@natoscott Can you take a look at this patch re PCP time archive support? Not sure if time(NULL) is acceptable for when replaying old PCP archives.

[BenBE]

Even the the code might be a bit larger (we are not constrained on this), I'd prefer the fully static format string variant here. And be it to avoid accidentally providing the wrong number of arguments and thus causing information leakage bugs …

[BenBE]

I think to avoid confusion regarding the year display the switch-over should be similar to what is used for things like ls, where files older than like 6 months are marked by the year, instead of a precise date/time.

Otherwise LGTM.

[natoscott]

@natoscott Can you take a look at this patch re PCP time archive support? Not sure if time(NULL) is acceptable for when replaying old PCP archives.

Sorry, I missed this question earlier. We don't have archive support yet in pcp-htop but yes, this sort of thing would be problematic from that POV. In general calling time() for every process we find is not ideal anyway - it would be more efficient to use the time_t from the timestamp we take at each time step. Nowadays this lives in the 'realtime.tv_sec' field of struct Machine and using this has the added benefit that it will do the right thing with PCP archives when we get to that.

[natoscott]

As per inline comment, it'd be slightly more efficient to use the timestamp we already took for this sample. Should be a relatively straight forward change too.
Otherwise, LGTM.

[BenBE]

Why are you moving the call to Process_fillStarttimeBuffer relative to the ProcessList_add call everywhere?

[arza-zara]

Why are you moving the call to Process_fillStarttimeBuffer relative to the ProcessList_add call everywhere?

Process_fillStarttimeBuffer needed processList. It's not needed anymore after rebasing on main so I'll remove that.

[natoscott]

LGTM.