Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.debian.kernel > #77215 > unrolled thread

Bug#1024811: linux: /proc/[pid]/stat unparsable

Back to article view | Back to linux.debian.kernel

Contents

  Bug#1024811: linux: /proc/[pid]/stat unparsable Thorsten Glaser <tg@mirbsd.de> - 2022-11-25 18:20 +0100
    Bug#1024811: linux: /proc/[pid]/stat unparsable Thorsten Glaser <tg@debian.org> - 2022-11-25 18:40 +0100
    Bug#1024811: linux: /proc/[pid]/stat unparsable Thorsten Glaser <tg@mirbsd.de> - 2022-12-22 02:10 +0100
      Bug#1024811: Re: Bug#1024811: linux: /proc/[pid]/stat unparsable Donald Buczek <buczek@molgen.mpg.de> - 2022-12-22 15:00 +0100
        Bug#1024811: Re: Bug#1024811: linux: /proc/[pid]/stat unparsable Thorsten Glaser <tg@mirbsd.de> - 2022-12-22 21:40 +0100
          Bug#1024811: linux: /proc/[pid]/stat unparsable Donald Buczek <buczek@molgen.mpg.de> - 2022-12-23 09:50 +0100
            Bug#1024811: linux: /proc/[pid]/stat unparsable Thorsten Glaser <tg@mirbsd.de> - 2022-12-23 10:00 +0100
              Bug#1024811: linux: /proc/[pid]/stat unparsable Donald Buczek <buczek@molgen.mpg.de> - 2022-12-23 10:20 +0100

#77215 — Bug#1024811: linux: /proc/[pid]/stat unparsable

Package: src:linux
Version: 5.10.149-2
Severity: normal
Tags: upstream
X-Debbugs-Cc: tg@mirbsd.de, adobriyan@gmail.com

tglase@x61w:~ $ cp /bin/mksh mk\ sh
tglase@x61w:~ $ ./mk\ sh -c 'echo $$; sleep 10' &
[1] 12862
tglase@x61w:~ $ 12862
cat /proc/12862/stat
12862 (mk sh) S 12649 12862 12649 34838 12864 4194304 124 0 0 0 0 0 0 0 20 0 1 0 58970609 2977792 211 18446744073709551615 93898845827072 93898846076365 140724844598496 0 0 0 0 0 134307847 0 0 0 17 0 0 0 0 0 0 93898846102320 93898846110384 93898853036032 140724844603277 140724844603306 140724844603306 140724844605424 0
tglase@x61w:~ $ cat /proc/12862/stat | hd
00000000  31 32 38 36 32 20 28 6d  6b 20 73 68 29 20 53 20  |12862 (mk sh) S |
[…]

This directly violates the documentation of proc(5) which says…

              The  fields,  in order, with their proper scanf(3) format speci‐
              fiers, are listed below.  Whether or not certain of these fields

… while scanf(3) indicates:

       s      Matches  a  sequence  of  non-white-space  characters;  the next

The effect is that /proc/[pid]/stat cannot be parsed the way it is
documented, as it does not escape embedded whitespace characters;
this, as a result, makes this file realistically useless and anything
relying on it broken.

-- Package-specific info:
** Version:
Linux version 5.10.0-19-amd64 (debian-kernel@lists.debian.org) (gcc-10 (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2) #1 SMP Debian 5.10.149-2 (2022-10-21)

** Command line:
BOOT_IMAGE=/SDcardBoot/vmlinuz-5.10.0-19-amd64 root=/dev/mapper/vg--cSD-lv--root ro net.ifnames=0 vga=792

** Tainted: W (512)
 * kernel issued warning

** Kernel log:
Unable to read kernel log; any relevant messages should be attached

** Model information
sys_vendor: LENOVO
product_name: 7673AG4
product_version: ThinkPad X61
chassis_vendor: LENOVO
chassis_version: Not Available
bios_vendor: LENOVO
bios_version: 7NET30WW (1.11 )
board_vendor: LENOVO
board_name: 7673AG4
board_version: Not Available

** Loaded modules:
cdc_acm
apple_mfi_fastcharge
fuse
cpufreq_ondemand
tun
ctr
ccm
cpufreq_powersave
snd_hda_codec_analog
snd_hda_codec_generic
snd_hda_intel
i915
snd_intel_dspcfg
soundwire_intel
soundwire_generic_allocation
snd_soc_core
iwl4965
iwlegacy
snd_compress
soundwire_cadence
mac80211
snd_hda_codec
coretemp
snd_hda_core
kvm_intel
snd_hwdep
drm_kms_helper
cfg80211
kvm
soundwire_bus
thinkpad_acpi
cec
snd_pcm
pcmcia
drm
iTCO_wdt
nvram
snd_timer
ppdev
ledtrig_audio
intel_pmc_bxt
libarc4
snd
evdev
yenta_socket
i2c_algo_bit
soundcore
rfkill
iTCO_vendor_support
irqbypass
pcmcia_rsrc
watchdog
parport_pc
pcmcia_core
serio_raw
pcspkr
sg
parport
ac
acpi_cpufreq
button
ecb
aes_generic
libaes
crypto_simd
cryptd
glue_helper
xts
dm_crypt
dm_mod
ext4
crc16
mbcache
jbd2
crc32c_generic
sd_mod
t10_pi
crc_t10dif
crct10dif_generic
mmc_block
crct10dif_common
ata_generic
ahci
ata_piix
sdhci_pci
libahci
cqhci
ehci_pci
uhci_hcd
libata
e1000e
scsi_mod
ehci_hcd
sdhci
i2c_i801
i2c_smbus
psmouse
lpc_ich
mmc_core
usbcore
ptp
usb_common
pps_core
battery
video

** PCI devices:
not available

** USB devices:
Bus 004 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 006 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
Bus 005 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
Bus 002 Device 002: ID 17ef:1000 Lenovo ThinkPad X6 UltraBase
Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 003 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
Bus 001 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub


-- System Information:
Debian Release: 11.5
  APT prefers stable-updates
  APT policy: (500, 'stable-updates'), (500, 'stable-security'), (500, 'stable')
Architecture: amd64 (x86_64)

Kernel: Linux 5.10.0-19-amd64 (SMP w/2 CPU threads)
Kernel taint flags: TAINT_WARN
Locale: LANG=C, LC_CTYPE=C (charmap=UTF-8) (ignored: LC_ALL set to C.UTF-8), LANGUAGE not set
Shell: /bin/sh linked to /bin/lksh
Init: sysvinit (via /sbin/init)

Versions of packages linux-image-5.10.0-19-amd64 depends on:
ii  initramfs-tools [linux-initramfs-tool]  0.140
ii  kmod                                    28-1
ii  linux-base                              4.6

Versions of packages linux-image-5.10.0-19-amd64 recommends:
pn  apparmor             <none>
ii  firmware-linux-free  20200122-1

Versions of packages linux-image-5.10.0-19-amd64 suggests:
pn  debian-kernel-handbook  <none>
ii  grub-pc                 2.06-3~deb11u4
pn  linux-doc-5.10          <none>

Versions of packages linux-image-5.10.0-19-amd64 is related to:
pn  firmware-amd-graphics     <none>
pn  firmware-atheros          <none>
pn  firmware-bnx2             <none>
pn  firmware-bnx2x            <none>
pn  firmware-brcm80211        <none>
pn  firmware-cavium           <none>
pn  firmware-intel-sound      <none>
pn  firmware-intelwimax       <none>
pn  firmware-ipw2x00          <none>
pn  firmware-ivtv             <none>
ii  firmware-iwlwifi          20210315-3
pn  firmware-libertas         <none>
pn  firmware-linux-nonfree    <none>
pn  firmware-misc-nonfree     <none>
pn  firmware-myricom          <none>
pn  firmware-netxen           <none>
pn  firmware-qlogic           <none>
pn  firmware-realtek          <none>
pn  firmware-samsung          <none>
pn  firmware-siano            <none>
pn  firmware-ti-connectivity  <none>
pn  xen-hypervisor            <none>

-- no debconf information

[toc] | [next] | [standalone]

#77216

Dixi quod…

>The effect is that /proc/[pid]/stat cannot be parsed the way it is
>documented, as it does not escape embedded whitespace characters;

… nor parenthesēs:

tglase@x61w:~ $ ./mk\)sh -c 'echo $$; sleep 10' &
[1] 13375
tglase@x61w:~ $ 13375
tglase@x61w:~ $ cat /proc/13375/stat
13375 (mk)sh) S 13330 13375 13330 34837 13377 4194304 124 0 0 0 0 0 0 0 20 0 1 0 59029474 2977792 180 18446744073709551615 94911056490496 94911056739789 140721459110048 0 0 0 0 0 134307847 0 0 0 17 1 0 0 0 0 0 94911056765744 94911056773808 94911059955712 140721459115917 140721459115946 140721459115946 140721459118064 0

This is… sad — extremely so. It does not escape anything.
I found proc_task_name(), which has an escape parameter,
which is set to false here, but it’s only for /status
which must escape newlines.

It’s used with false in /stat and /comm… the latter indeed
needing no escapes.

I’d argue that it needs a tristate argument, 0 for /comm
to not escape anything, 1 for /status to escape newlines,
and 2 for /stat to escape whitespace (and perhaps also a
closing parenthesis, using \x29, so splitting both using
scanf as indicated in the manpage and using parenthesēs
as people seem to do on the ’net is fixed).

bye,
//mirabilos
-- 
22:20⎜<asarch> The crazy that persists in his craziness becomes a master
22:21⎜<asarch> And the distance between the craziness and geniality is
only measured by the success 18:35⎜<asarch> "Psychotics are consistently
inconsistent. The essence of sanity is to be inconsistently inconsistent

[toc] | [prev] | [next] | [standalone]

#77675

On Sat, 26 Nov 2022, Alexey Dobriyan wrote:

>/proc never escaped "comm" field of /proc/*/stat.

Yes, that’s precisely the bug.

>To parse /proc/*/stat reliably, search for '(' from the beginning, then
>for ')' backwards. Everything in between parenthesis is "comm".

That’s not guaranteed to stay reliable: fields can be, and have
been in the past, added, and new %s fields will break this. Do
not rely on it either.

>Everything else are numbers separated by spaces.

Currently, yes.

But the field is *clearly* documented as intended to be
parsable by scanf(3), which splits on white space. So the
Linux kernel MUST encode embedded whitespace so the
documented(!) access method works.

bye,
//mirabilos
-- 
15:41⎜<Lo-lan-do:#fusionforge> Somebody write a testsuite for helloworld :-)

[toc] | [prev] | [next] | [standalone]

#77681 — Bug#1024811: Re: Bug#1024811: linux: /proc/[pid]/stat unparsable

On 12/22/22 1:53 AM, Thorsten Glaser wrote:
> On Sat, 26 Nov 2022, Alexey Dobriyan wrote:
> 
>> /proc never escaped "comm" field of /proc/*/stat.
> 
> Yes, that’s precisely the bug.
> 
>> To parse /proc/*/stat reliably, search for '(' from the beginning, then
>> for ')' backwards. Everything in between parenthesis is "comm".
> 
> That’s not guaranteed to stay reliable: fields can be, and have
> been in the past, added, and new %s fields will break this. Do
> not rely on it either.
> 
>> Everything else are numbers separated by spaces.
> 
> Currently, yes.
> 
> But the field is *clearly* documented as intended to be
> parsable by scanf(3), which splits on white space. So the
> Linux kernel MUST encode embedded whitespace so the
> documented(!) access method works.

No, Escaping would break existing programs which parse the line by searching for the ')' from the right. The format, surly, is ugly, but that is how it is.

If some documentation suggests, that you can just parse it with scanf, the documentation should be corrected/improved instead.

Are you referring to proc(5) "The fields, in order, with their proper scanf(3) format specifiers, are listed below" [1] or something else?

The referenced manual page is wrong in regard to the length, too. There is no 16 character limit to the field, because it can contain a workqueue task name, too:

    buczek@theinternet:/tmp$ cat /proc/27190/stat
    27190 (kworker/11:2-mm_percpu_wq) I 2 0 0 0 -1 69238880 0 0 0 0 0 170 0 0 20 0 1 0 109348986 0 0 18446744073709551615 0 0 0 0 0 0 0 2147483647 0 0 0 0 17 11 0 0 0 0 0 0 0 0 0 0 0 0 0

The current limit seems to be 64 characters [2] when escaping is off, as it is the case with /proc/pid/stat. But generally the length of the field and thereby of the whole line seems to be rather undefined. So to parse that, you either either need to do some try-and-restart-with-a-bigger-buffer dance or use a buffer size of which you just hope that it will be big enough for the forseable time. 

In fact, if you start escaping now you might also break programs which rely on the current 64 character limit.

Best

  Donald

[1]: https://man7.org/linux/man-pages/man5/proc.5.html
[2]: https://elixir.bootlin.com/linux/latest/source/fs/proc/array.c#L99

> 
> bye,
> //mirabilos
> 


-- 
Donald Buczek
buczek@molgen.mpg.de
Tel: +49 30 8413 1433

[toc] | [prev] | [next] | [standalone]

#77684 — Bug#1024811: Re: Bug#1024811: linux: /proc/[pid]/stat unparsable

Donald Buczek dixit:

>No, Escaping would break existing programs which parse the line by
>searching for the ')' from the right.

Huh? No!

The format is "(" + string + ") " after all, and only the string
part would get escaped.

The only visible change would be that programs containing a
whitespace character (and, ideally, a ‘(’) in their name would
be escaped, which are these that are currently broken anyway.
And perhaps backslashes, if you decide to encode unambiguous,
but given the field length limit, I don’t think that was ever
a goal (both because I suspect this file was intended to be
used to get a quick overview and therefore deliberately shortens
and because the full info is available elsewhere), so no need to
encode unambiguously.

>If some documentation suggests, that you can just parse it with scanf,
>the documentation should be corrected/improved instead.

No. Someone recently did a survey, and most code in existence splits
by whitespace. Fix the kernel bug instead.

>Are you referring to proc(5) "The fields, in order, with their proper
>scanf(3) format specifiers, are listed below" [1] or something else?

Yes.

>The referenced manual page is wrong in regard to the length, too. There
>is no 16 character limit to the field, because it can contain a
>workqueue task name, too:

Probably used to be cut off after 16. Go fix that in the manpage
then. But do fix the encoding kernel-side.

>In fact, if you start escaping now you might also break programs which
>rely on the current 64 character limit.

Just cut off at the end then, like I suspect was done at 16 bytes
initially.

Or strip whitespace and closing parenthesis if present instead
of encoding them, or replace them with a question mark.

bye,
//mirabilos
-- 
“It is inappropriate to require that a time represented as
 seconds since the Epoch precisely represent the number of
 seconds between the referenced time and the Epoch.”
	-- IEEE Std 1003.1b-1993 (POSIX) Section B.2.2.2

[toc] | [prev] | [next] | [standalone]

#77697

On 12/22/22 21:28, Thorsten Glaser wrote:
> Donald Buczek dixit:
> 
>> No, Escaping would break existing programs which parse the line by
>> searching for the ')' from the right.
> 
> Huh? No!
> 
> The format is "(" + string + ") " after all, and only the string
> part would get escaped.
> 
> The only visible change would be that programs containing a
> whitespace character (and, ideally, a ‘(’) in their name would

')'

> be escaped, which are these that are currently broken anyway.

You would still break programs which use the string for anything else than displaying it to the user.

We have a job control daemon, which stored PIDs of jobs it has started in a database. The daemon is able to restart. When it comes back up, it needs to find out, whether its jobs are still alive. The problem here is pid wrap: A job might be gone, but a unrelated new process might have gotten the recycled pid. To avoid confusion, the restarting job control daemon looks at the comm value of the process in question, which is known for its own jobs [1].

[1]: https://github.molgen.mpg.de/mariux64/mxq/blob/f3d9fb8c6143c3a884210b212ed4a8514a49a414/mxqd.c#L904

In this case, the fixed process name (set with prctl PR_SET_NAME) even contains a space: "mxqd reaper".

To be fair, this daemon doesn't use /proc/pid/stat for that, but /proc/pid/comm instead. So it wouldn't really be affected by your proposed change. But that is just a random design decision. As /proc/pid/stat is also used in many places, it could as well use that to avoid code duplication or reuse data already read from the other source.

> And perhaps backslashes, if you decide to encode unambiguous,
> but given the field length limit, I don’t think that was ever
> a goal (both because I suspect this file was intended to be
> used to get a quick overview and therefore deliberately shortens
> and because the full info is available elsewhere), so no need to
> encode unambiguously.
> 
>> If some documentation suggests, that you can just parse it with scanf,
>> the documentation should be corrected/improved instead.
> 
> No. Someone recently did a survey, and most code in existence splits
> by whitespace. Fix the kernel bug instead.

Yes, I've seen that on oss-security. No doubt, its easy to parse the file wrongly and no doubt, many programs do that.

I also acknowledge, that the man page and the implementation are in conflict.

However, afaik, 'correctness' in the kernel world is not defined by specifications, less by man pages, but by implementation. So this can't be declared a kernel bug just because it conflicts with a manpage.

Plus the manpage, which you use as a foundation that there is something to fix, doesn't talk about encoding, either. So even when some encoding was applied, the interface would still be in conflict with the manpage.

Generally, changes, which might break userspace, are not very welcome, even if the current implementation is ugly and difficult to work with. I just wanted to point out,  that there exists programs which interpret the comm value they got from procfs. If these programs happen to use /proc/pid/stat for reading it, they might fail, if the format was changed. And experience shows, that any (miss-)feature is used by somebody somewhere, so any "might break" is really a "will break".

I don't object to a change and I think its a valid position to risk a breakage of a very few programs for what you might gain here. But it is not self-evident by the declarative power of the manpage or otherwise. It's a judgement, which has to be taken.

Best
   Donald

>> Are you referring to proc(5) "The fields, in order, with their proper
>> scanf(3) format specifiers, are listed below" [1] or something else?
> 
> Yes.
> 
>> The referenced manual page is wrong in regard to the length, too. There
>> is no 16 character limit to the field, because it can contain a
>> workqueue task name, too:
> 
> Probably used to be cut off after 16. Go fix that in the manpage
> then. But do fix the encoding kernel-side.


>> In fact, if you start escaping now you might also break programs which
>> rely on the current 64 character limit.
> 
> Just cut off at the end then, like I suspect was done at 16 bytes
> initially.
> 
> Or strip whitespace and closing parenthesis if present instead
> of encoding them, or replace them with a question mark.
> 
> bye,
> //mirabilos

-- 
Donald Buczek
buczek@molgen.mpg.de
Tel: +49 30 8413 1433

[toc] | [prev] | [next] | [standalone]

#77698

Donald Buczek dixit:

> To be fair, this daemon doesn't use /proc/pid/stat for that, but /proc/pid/comm

Yes, and that’s proper. The field in /proc/pid/stat is size-limited
and so not necessarily distinct.

> As /proc/pid/stat is also used in many places, it could as well use
> that to avoid code duplication or reuse data already read from the
> other source.

No, because the data in /stat is incomplete *and* anything using
it that would be affected by escaping was already broken.

bye,
//mirabilos
-- 
22:20⎜<asarch> The crazy that persists in his craziness becomes a master
22:21⎜<asarch> And the distance between the craziness and geniality is
only measured by the success 18:35⎜<asarch> "Psychotics are consistently
inconsistent. The essence of sanity is to be inconsistently inconsistent

[toc] | [prev] | [next] | [standalone]

#77702

On 12/23/22 09:49, Thorsten Glaser wrote:
> Donald Buczek dixit:
> 
>> To be fair, this daemon doesn't use /proc/pid/stat for that, but /proc/pid/comm
> 
> Yes, and that’s proper. The field in /proc/pid/stat is size-limited
> and so not necessarily distinct.

No, it is the process name itself, which is size limited, so in this regard it doesn't make a difference if you read it from /proc/pid/stat or /proc/pid/comm.

>> As /proc/pid/stat is also used in many places, it could as well use
>> that to avoid code duplication or reuse data already read from the
>> other source.
> 
> No, because the data in /stat is incomplete *and* anything using
> it that would be affected by escaping was already broken.

"Incomplete" because if truncation?

The usage in my example is not already broken. Truncation doesn't happen, because the process name used is the fixed string "mxqd reaper".

A process name is limited to 15 characters. The limit is already in force when you use PR_SET_NAME, so there is no truncation when you read it back from procfs.

D.

> 
> bye,
> //mirabilos

-- 
Donald Buczek
buczek@molgen.mpg.de
Tel: +49 30 8413 1433

[toc] | [prev] | [standalone]

Back to top | Article view | linux.debian.kernel

csiph-web