Peter Tribble wrote:
> On Thu, 2006-04-20 at 22:43, Dave Miner wrote:
...
>> Peter Tribble wrote:
...
>>> 2. 6412140
>>>
>>> Rewritten the ELF check for the file referenced by the -i flag. I
>>> presume that this feature is to guard against supplying a binary file
>>> as input, but it also catches genuine cases. So I explicitly check just
>>> the first 4 characters of the file for the ELF signature.
>>>
>> Fair enough. I'm not sure how useful this check really is, as there are
>> a lot of other types of files which would be somewhat toxic as input,
>> but I guess it doesn't hurt.
>
> I'm not sure either. Presumably there would be some record of why it
> was added. Without the history, I don't know enough about what it's
> meant to do to feel safe in taking it out.
>
> Besides, I can see 'pkgchk -l -i /bin/ls' as a plausible typo.
>
No, I don't see any point to taking it out now, I just think it's a
curious choice.
>>> 3. 6412749
>>>
>>> Enhanced to allow -i input file to be specified on stdin.
>>>
>>> Check and generate an error on empty input file.
>>>
>>> This last check adds an extra error message.
>>>
>> Fine by me.
>>
>>> 4. 6412765
>>>
>>> I guard against overly long input strings (doesn't do anything
>>> smart, but doesn't crash).
>>>
>>> I do this by adding a length specifier to fscanf. (Ideally this would
>>> use PATH_MAX rather than being hardcoded to 1024, but I'm not sure how
>>> to get a numeric define in a format specifier past the preprocessor.)
>>>
>> OK with this part. I don't have a better idea.
>
> It's a tricky one. I did see some two-layer substitution trick,
> but it looked pretty ugly.
>
Probably offensively obscure.
>>> 5. 6322837
>>>
>>> I've added a simple check that the input file specified is indeed a
>>> regular file.
>>>
>>> Note that this actually constitutes a change of behaviour, and will
>>> cause anything that explicitly uses /dev/stdin (rather than the more
>>> conventional notation - see point 3 above) to fail. I have identified 2
>>> places where this occurs
>>>
>>> /sfw/usr/src/pkgdefs/common_files/checkinstall.initd
>>> /on/usr/src/pkgdefs/common_files/checkinstall.initd
>>>
>> Ouch; synchronizing fixing those two would be painful, we might also
>> need an ARC fasttrack to allow this change. How about adding an S_ISCHR
>> as well, which would continue to allow /dev/stdin to work?
>
> That idea falls over. You actually need S_ISFIFO for stdin, and that
> doesn't work very well either. My preferred solution (after trying
> a number of variations that didn't work) is simply to check for the
> "/dev/stdin" string explicitly. Diff from my original:
>
I'm still hoping for something less, um, special. Can you say more
about the variation you tried that didn't work? I'm wondering if
!S_ISDIR() (maybe && !S_ISBLK()) is the better choice, since it would
catch pretty much all the obviously stupid errors.
>
> *** main.c.firstpass Tue Apr 18 19:11:32 2006
> --- main.c Sun Apr 23 20:26:14 2006
> ***************
> *** 530,535 ****
> --- 530,537 ----
>
> if (strcmp(file, "-") == 0) {
> fplist = stdin;
> + } else if (strcmp(file, "/dev/stdin") == 0) {
> + fplist = stdin;
> } else {
> if ((fd = open(file, O_RDONLY)) == -1) {
> progerr(gettext(ERR_IOPEN), file);
>
> This captures the exact usage, and doesn't allow other peculiar
> file to get through the trap.
>
>>> 6. 6413788
>>>
>>> Fixed various typos and removed unused defines.
>>>
>> Thanks for cleaning up the trash!
>>
>> I've built and tested the submitted code and it does work as billed.
>
> That's good to hear. I mean, I hoped that would be the case,
> but it's nice to have it confiremd that I haven't goofed.
>
Not that I expected otherwise, this was mostly to record that I had in
fact tried it ;-)
Dave
