On Tue, Sep 18, 2018 at 04:46:38PM -0700, Rob Herring wrote:
> Since commit 7975f6422260 ("Fix widespread incorrect use of strneq(),
> replace with new strprefixeq()") simple-bus checks have been silently
> skipped. The problem was 'end - str' is one more than the string length
> and the strnlen in strprefixeq fails. This can't be fixed simply by
> subtracting one as it is possible to have multiple '\0' at the end of
> the property. So first check the property is null terminated and then
> use streq() for comparisons.
>
> Add some tests so the problem doesn't happen again.
>
> Fixes: 7975f6422260 ("Fix widespread incorrect use of strneq(), replace with new strprefixeq()")
> Reported-by: Kumar Gala <kumar.gala@xxxxxxxxxx>
> Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
So, the existing version was definitely wrong. The second parameter
to strprefixeq needs to be the exact length of the prefix to compare,
and it's clearly not here. But your proposed version isn't quite
right either.
> ---
> checks.c | 8 ++++++--
> tests/run_tests.sh | 4 ++++
> tests/unit-addr-simple-bus-compatible.dts | 18 ++++++++++++++++++
> tests/unit-addr-simple-bus-reg-mismatch.dts | 18 ++++++++++++++++++
> 4 files changed, 46 insertions(+), 2 deletions(-)
> create mode 100644 tests/unit-addr-simple-bus-compatible.dts
> create mode 100644 tests/unit-addr-simple-bus-reg-mismatch.dts
>
> diff --git a/checks.c b/checks.c
> index 9c9b0c328af6..1ccbfa60c839 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -908,9 +908,13 @@ static bool node_is_compatible(struct node *node, const char *compat)
> if (!prop)
> return false;
>
> - for (str = prop->val.val, end = str + prop->val.len; str < end;
> + str = prop->val.val;
> + if (strnlen(str, prop->val.len) == prop->val.len)
> + return false;
This will catch the case where the property has no \0s at all, but not
a case like say...
compatible = "foo", /bits/ 8 <'s' 'i' 'm'>;
> +
> + for (end = str + prop->val.len; str < end;
> str += strnlen(str, end - str) + 1) {
> - if (strprefixeq(str, end - str, compat))
> + if (streq(str, compat))
...which will lead to this streq() running over the end of the property.
I think you instead will need something like this:
len = strlen(compat);
/* ... */
if ((end - str) > len && (memcmp(str, compat, len) == 0))
/* ... */
> return true;
> }
> return false;
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index adc4daebd945..6756f3d9f7be 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -668,6 +668,10 @@ dtc_tests () {
> check_tests pci-bridge-bad1.dts pci_bridge
> check_tests pci-bridge-bad2.dts pci_bridge
>
> + check_tests unit-addr-simple-bus-reg-mismatch.dts simple_bus_reg
> + check_tests unit-addr-simple-bus-compatible.dts simple_bus_reg
> +
> +
> # Check warning options
> run_sh_test dtc-checkfails.sh address_cells_is_cell interrupt_cells_is_cell -n size_cells_is_cell -- -Wno_size_cells_is_cell -I dts -O dtb bad-ncells.dts
> run_sh_test dtc-fails.sh -n test-warn-output.test.dtb -I dts -O dtb bad-ncells.dts
> diff --git a/tests/unit-addr-simple-bus-compatible.dts b/tests/unit-addr-simple-bus-compatible.dts
> new file mode 100644
> index 000000000000..c8f9341c1217
> --- /dev/null
> +++ b/tests/unit-addr-simple-bus-compatible.dts
> @@ -0,0 +1,18 @@
> +/dts-v1/;
> +
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + bus@10000000 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "foo-bus", "simple-bus";
> + ranges = <0x0 0x10000000 0x10000>;
> +
> + node@100 {
> + reg = <0x1000 1>;
> + };
> + };
> +
> +};
> diff --git a/tests/unit-addr-simple-bus-reg-mismatch.dts b/tests/unit-addr-simple-bus-reg-mismatch.dts
> new file mode 100644
> index 000000000000..28233777097b
> --- /dev/null
> +++ b/tests/unit-addr-simple-bus-reg-mismatch.dts
> @@ -0,0 +1,18 @@
> +/dts-v1/;
> +
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + bus@10000000 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "simple-bus";
> + ranges = <0x0 0x10000000 0x10000>;
> +
> + node@100 {
> + reg = <0x1000 1>;
> + };
> + };
> +
> +};
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
