udev: readd format length parameter

[tblume]

legacy udev had an udev rule parameter "format length" that could determine the
lenght of a string taken from sysfs attributes.
This seems to have become lost during the transition to systemd-udev.

Adding it back helps for backward compatibility and more userfriendly symlink names.

[evverx]

I think it would also be great to update udev-test.pl.

[evverx]

Is it safe to call strjoina inside this loop?

[poettering]

stack allocation is never permissible in loops. strjoina() does stack allocation, hence no, it's not safe.

[evverx]

What happens to dest if num+1 is greater than size?

[tblume]

num+1 > size should be handeled by the backend function.
The function strpcpy (src/basic/strxcpyx.c) that is called from subst_format_var will only copy up to the length of the source string:

size_t strpcpy(char **dest, size_t size, const char *src) {
[...]
len = strlen(src);
if (len >= size) {
if (size > 1)
*dest = mempcpy(*dest, src, size-1);
size = 0;
} else {
if (len > 0) {
*dest = mempcpy(*dest, src, len);
size -= len;
}
}

[evverx]

It seems that the length of src might be greater than the length of dst in this case.

[evverx]

In other words, l shows how many bytes can be written to dst. If l is increased and strlen(src) is greater than the previous value of l, then dst might be overflowed.

[evverx]

I've changed test-udev.pl a bit:

@@ -289,7 +289,7 @@ EOF
                 not_exp_name    => "not" ,
                 rules           => <<EOF
 SUBSYSTEMS=="scsi", PROGRAM=="/bin/echo -n special-device", RESULT=="-special-*", SYMLINK+="not"
-SUBSYSTEMS=="scsi", PROGRAM=="/bin/echo -n special-device", RESULT=="special-*", SYMLINK+="%c-%n"
+SUBSYSTEMS=="scsi", PROGRAM=="/bin/echo -n special-device", RESULT=="special-*", SYMLINK+="%14c-%n"
 EOF

. I got the following:

==10735== Invalid write of size 1
==10735==    at 0x4C34F44: __GI_mempcpy (vg_replace_strmem.c:1518)
==10735==    by 0x4F18AA9: strpcpy (strxcpyx.c:53)
==10735==    by 0x117E08: subst_format_var (udev-event.c:182)
==10735==    by 0x118BA2: udev_event_apply_format (udev-event.c:418)
==10735==    by 0x125652: udev_rules_apply_to_event (udev-rules.c:2330)
==10735==    by 0x11AA7E: udev_event_execute_rules (udev-event.c:903)
==10735==    by 0x1176CC: main (test-udev.c:137)
==10735==  Address 0xf is not stack'd, malloc'd or (recently) free'd
==10735==
==10735==
==10735== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==10735==  Access not within mapped region at address 0xF
==10735==    at 0x4C34F44: __GI_mempcpy (vg_replace_strmem.c:1518)
==10735==    by 0x4F18AA9: strpcpy (strxcpyx.c:53)
==10735==    by 0x117E08: subst_format_var (udev-event.c:182)
==10735==    by 0x118BA2: udev_event_apply_format (udev-event.c:418)
==10735==    by 0x125652: udev_rules_apply_to_event (udev-rules.c:2330)
==10735==    by 0x11AA7E: udev_event_execute_rules (udev-event.c:903)
==10735==    by 0x1176CC: main (test-udev.c:137)

.

By the way, judging by a0ee5a0, it seems that %number was removed on purpose. Does anybody know why?

[tblume]

Thanks for the investigation, I wasn't aware that the format length feature has been explicitely removed. Waiting for feedback about the reason before continuing.

Concerning the dst overflow, shouldn't this rather be fixed in the strpcpy function, instead of taking care of this via the calling function?
IMHO, if strpcpy uses strlen(src) instead of l, it should also take care that the size of dst gets reset to strlen(src), no?

[evverx]

I'm not sure what strpcpy can do when it receives the wrong length of the destination string:

s1 = new(char, 5)
strpcpy(&s1, 7, "012345678")

Only the caller knows that 5 should be passed instead of 7.

[tblume]

I see, thanks for elaboration.

[evverx]

You're welcome. Actually, this PR has helped me to find #6664 and I'm a bit worried that bugs of that kind seem to be quite common in udev.

[tblume]

@kaysievers can you give some background about the reason for a0ee5a0 ?

[kaysievers]

I don't remember any specific reasons about this one, just that we removed a whole bunch of magic string mangling; we did not want udev provide shell-like features. People asked for more of them, and we thought they should use a shell, if they need to do more complicated things.

[poettering]

I think we should have less string mangling support in udev rules, not more. hence, let's not add this, I see little point in restoring something that was removed 8 years ago and nobody noticed or missed so far.

Closing. I hope this makes sense.