Open geraldo-netto opened 1 year ago
lukefromdc
commented
1 year ago 32 is a power of two, so I am wondering if we are storing something in a fixed length variable ( 5 bytes maybe?) related to this and overflowing it with more than 32 cores. Keep in mind, when this code was first written, a big server board with two or maybe four sockets with single-core CPU's (maybe hyperthreaded) would have been the maximum anyone ever saw.
geraldo-netto
commented
1 year ago Hi @lukefromdc ,
Indeed you're right Also, I was trying to play around with it and noticed the same fix applied in [1] works:
...
<key name="cpu-color32" type="s">
<default>'#069999'</default>
<summary>Default graph CPU color</summary>
</key>
...
<key name="cpu-color47" type="s">
<default>'#369999'</default>
<summary>Default graph CPU color</summary>
</key>
...[1] https://github.com/mate-desktop/mate-system-monitor/pull/147/files
lukefromdc
commented
1 year ago This shows we were simply running out of individually specified gsettings keys. I could see trying to store a gsettings value that does not exist being enough to attempt a write to RAM that has not been allocated, which would be a segfault.
This layout of the program was fine for when only a few cores ever existed but now means we will have to keep growing the list of gsettings keys. For now we should do that as it's the simplest fix with the least potential for regressions. If we ever have to increase 48 to 64 or 96 keys though it becomes a lot of work (once each time) and rather a lot of RAM to hold separate gsettings keys. Not sure a better way exists, we'd need a struct holding one color value per core number, and allocated at startup to the number of cores present on the machine to optimize this sort of thing. Might be easier to just keep growing org.mate.system-monitor.gschema.xml.in as we know that works and won't create new bugs
geraldo-netto
commented
1 year ago @lukefromdc ,
shall I preper a patch for org.mate.system-monitor.gschema.xml.in containing 64 entries? My understanding is that colours are on rgba format, right? (never touched this code nor gtk) Do we have any guideline on colours? Also, I wonder if it would make any sense recreate all colours for all 64 entries because we don't want to have duplicates Maybe for the future, we could consider grouping CPU cores and/or socket (if there are too many) using geometric mean?
lukefromdc
commented
1 year ago Proceed with it, simply match the format of the preceding entries. These are RGB not RGBA colors, you can see them in
https://github.com/mate-desktop/mate-system-monitor/pull/147/files
where the default for higher numbers of core (but only up to 32) was set to
#339999
I don't have anything w more than 12 threads so cannot test for function, not sure if anyone on the team does. Thus I can check that it does not create a problem on a smaller machine but will have to rely on you to check the higher number cores
geraldo-netto
commented
1 year ago Uhm... I have update the xml and still sigtrapping...
(mate-system-monitor:1092987): GLib-GIO-ERROR **: 13:36:35.041: Settings schema 'org.mate.system-monitor' does not contain a key named 'cpu-color42'
Trace/breakpoint trap (core dumped)The XML ():
<key name="cpu-color0" type="s">
<default>'#FF6E00'</default>
<summary>Default graph CPU color</summary>
</key>
...
<key name="cpu-color31" type="s">
<default>'#339999'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color32" type="s">
<default>'#180808'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color33" type="s">
<default>'#301818'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color34" type="s">
<default>'#781818'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color35" type="s">
<default>'#901818'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color36" type="s">
<default>'#A81818'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color37" type="s">
<default>'#C01818'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color38" type="s">
<default>'#D81818'</default>
<summary>Default graph cpu color</summary>
</key>
<key name="cpu-color39" type="s">
<default>'#F01818'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color40" type="s">
<default>'#FFCC00'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color41" type="s">
<default>'#009900'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color42" type="s">
<default>'#CC6600'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color43" type="s">
<default>'#330099'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color44" type="s">
<default>'#CC0066'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color45" type="s">
<default>'#00FFCC'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color46" type="s">
<default>'#FFCC99'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color47" type="s">
<default>'#339999'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color48" type="s">
<default>'#A83018'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color49" type="s">
<default>'#A84818'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color50" type="s">
<default>'#A86018'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color51" type="s">
<default>'#A87818'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color52" type="s">
<default>'#A8A818'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color53" type="s">
<default>'#A8C018'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color54" type="s">
<default>'#A8E018'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color55" type="s">
<default>'#A8F018'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color56" type="s">
<default>'#A8A820'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color57" type="s">
<default>'#A8A840'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color58" type="s">
<default>'#A8A860'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color59" type="s">
<default>'#A8A880'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color60" type="s">
<default>'#A8A8A0'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color61" type="s">
<default>'#A8A8C0'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color62" type="s">
<default>'#A8A8E0'</default>
<summary>Default graph CPU color</summary>
</key>
<key name="cpu-color63" type="s">
<default>'#A8A8FE'</default>
<summary>Default graph CPU color</summary>
</key>Another thing is that, it seems better to use 24px for a lot of cores
// file interface.cpp
static void
create_sys_view (ProcData *procdata, GtkBuilder * builder)
{
...
gtk_box_pack_start (GTK_BOX (temp_hbox), color_picker, FALSE, TRUE, 0);
if (procdata->config.num_cpus > 15) {
gtk_widget_set_size_request(GTK_WIDGET(color_picker), 32, -1);
} else {
// reduces color box in order to fit more CPU cores
gtk_widget_set_size_request(GTK_WIDGET(color_picker), 24, -1);
}
// removed the check for single CPU, just let it enumerate for many cores
label_text = g_strdup_printf (_("CPU%d"), i+1);
...
}Before changing pixel size:
After changing pixel size:
lukefromdc
commented
1 year ago That suggests we are indeed overflowing a fixed length variable somewhere, and that in turn could mean a buffer overflow attack becomes possible on a multiuser system with enough cores with MATE installed: invoke mate-system-monitor from a specially crafted script, then bring up the CPU use display and watch it crash back to the terminal-and trigger your overflow attack. Never done this so I don't know what you'd have to do in terminal but segfaults on too much data are a known vulnerability.
Expected behaviour
System monitor is expected to use a different colour for each CPU core but many CPU cores get the same colours Also, the colour picker works properly up to 32 cores, after 32, if you select a different colour from CPU core 33 onwards and apply it, it will crash system monitor
Actual behaviour
Steps to reproduce the behaviour
1) open system monitor in a machine with more than 32 CPU cores 2) visualise the CPU graph with repeated colours for CPU cores
MATE general version
mate 1.26.0
Package version
mate system monitor 1.26.0-1
Linux Distribution
linux mint 21.1 (Vera)
Link to bugreport of your Distribution (requirement)
N/A
Crash dump