Re: [PATCH] fix detection of non-LUKS CRYPT

[Corey Hickey <bugfood-ml@fatooh.org>]

On 2016-11-06 02:01, Andrei Borzenkov wrote:
> 06.11.2016 10:09, Corey Hickey пишет:
>> On 2016-11-05 05:31, Andrei Borzenkov wrote:
>>>> diff --git a/grub-core/osdep/devmapper/getroot.c
>>>> b/grub-core/osdep/devmapper/getroot.c
>>>> index 72e5582..a13a39c 100644
>>>> --- a/grub-core/osdep/devmapper/getroot.c
>>>> +++ b/grub-core/osdep/devmapper/getroot.c
>>>> @@ -143,7 +143,7 @@ grub_util_get_dm_abstraction (const char *os_dev)
>>>>        grub_free (uuid);
>>>>        return GRUB_DEV_ABSTRACTION_LVM;
>>>>      }
>>>> -  if (strncmp (uuid, "CRYPT-LUKS1-", 4) == 0)
>>>> +  if (strncmp (uuid, "CRYPT-LUKS1-", 12) == 0
>>>
>>> Committed, thanks! We really need some wrapper around (strncmp (foo,
>>> "bar", sizeof ("bar") - 1), but for now it is OK as bug fix.
>>
>> Excellent, you're welcome. That seemed like the most simple fix.
>>
>> I took a stab at adding such a wrapper, but there are a ton of files
>> that could use it which I won't have a chance at being able to test. I
>> can send in an untested patch if you want...
>>
>
> Large scale replacement will have to wait until release, but we sure can
> discuss (and add) macro itself. I'm leaning towards simple
>
> #define GRUB_IS_PREFIX(string,prefix) (strncmp((string), #prefix, sizeof
> (#prefix) - 1) == 0)
>
> Any other idea how to make it constant-safe?

I have to admit at this point that I never really got good at C, and 
most of that was years ago. If I understand correctly, though, the 
stringification in your example macro makes "foo" into "\"foo\"", before 
sizeof() which would not be suitable.

My initial idea matched your earlier example:

#define STARTS_WITH(s1, s2) (strncmp((s1), (s2), sizeof(s2) - 1) == 0)

Am I failing to see a use case where that breaks?

The one bad thing I can see is that it can fail poorly if somebody 
reverses the arguments by mistake. In that case, sizeof(s2) returns the 
pointer size rather than the length of the string literal. I don't know 
how to enforce that a macro argument be a string literal. An alternate 
idea is:

#define STARTS_WITH(s1, s2) (strncmp((s1), (s2), strlen(s2)) == 0)

That should work with arguments in either order (though of course s2 is 
expected to be smaller). I don't know of a drawback other than strlen 
presumably being a bit slower, but I didn't get the impression any of 
the code in question was performance-sensitive.

-Corey