[pacman-dev] [PATCH] Allow querying directory ownership
Andrew Gregory
andrew.gregory.8 at gmail.com
Fri Sep 7 10:34:47 EDT 2012
On Fri, 07 Sep 2012 21:56:48 +1000
Allan McRae <allan at archlinux.org> wrote:
> On 07/09/12 21:06, Menachem Moystoviz wrote:
> > I'm not a dev, nor the original author of the patch, but...
> >
> > On Fri, Sep 7, 2012 at 10:29 AM, Allan McRae <allan at archlinux.org> wrote:
> >> On 06/09/12 13:34, Andrew Gregory wrote:
<snip>
> >>> +
> >>> + /* make sure pkgfile and target are of the same type */
> >>> + if(!is_dir != !(pkgfile[pkgfile_len - 1] == '/')) {
> >>
> >> !!! - literally.... How about:
> >>
> >> if (is_dir && (pkgfile[pkgfile_len - 1] != '/'))
> > This isn't the same check - Andrew's check verifies that is_dir XOR
> > (pkgfile's last character is not '/' (i.e. pkgfile names a file
> > path)),
> > is true, while your test is for AND. This way of defining XOR is quite
> > confusing, though - isn't there a clearer way? (i.e. macros, inline
> > functions?)
>
> Ah - that is confusing... obviously!
>
> So a clearer version of the test would be:
>
> if ((!is_dir && pkgfile[pkgfile_len - 1] == '/') ||
> (is_dir && pkgfile[pkgfile_len - 1] != '/'))
>
Yeah, the !!! xor can be a bit confusing if you're not familiar with
it, but I dislike long multi-line ifs. I can just force is_dir to 1
or 0 and compare them directly:
is_dir = S_ISDIR(buf.st_mode) ? 1 : 0;
...
if(is_dir != (pkgfile[pkgfile_len - 1] == '/'))...
> >>
> >>> + continue;
> >>> + }
> >>> +
> >>> + /* make sure pkgfile is long enough */
> >>> + if(pkgfile_len < pbname_len) {
> >>> + continue;
> >>> + }
> >>
> >> If I understand this correctly, we are comparing the length of the final
> >> component of the passed in parameter to the entire filename length from
> >> the package. I think that it would be quite uncommon to hit the
> >> continue here, so that can be removed.
> >>
> > I think you mean to say that we replace this if(){continue;} with
> > if(!){...} wrapping the rest of the loop?
> > That would make sense if this branch is costly.
This is a safety to make sure that we don't try to dereference a
negative index in the next if.
>
> No. I was saying that I think this test is going to result in a very
> small number of cases being skipped and so it would be more efficient to
> just not perform it.
>
> >>>
> >>> - /* avoid the costly realpath usage if the basenames don't match */
> >>> - if(strcmp(mbasename(pkgfile), bname) != 0) {
> >>> + /* make sure pbname_len takes us to the start of a path component */
> >>> + if(pbname_len != pkgfile_len && pkgfile[pkgfile_len - pbname_len - 1] != '/') {
> >>
> >> With the above removal:
> >> if (pbname_len > pkgfile_len)
> >>
This check must be != because it is specifically looking for pbname_len
either taking us to the beginning of the string or the beginning of a
path component, ie:
pkgfile == "basename"
or
pkgfile == "foo/bar/basename"
but not
pkgfile == "foo/barbasename"
It could be combined (not replaced) with the check in the previous if
statement, but that gets a little ugly, so I left them separate.
> >>> + continue;
> >>> + }
> >>> +
> >>> + /* compare basename with bname */
> >>> + if(strncmp(pkgfile + pkgfile_len - pbname_len, bname, bname_len) != 0) {
> >>
> >> Why an strncmp here? strcmp will do the same comparison...
> >>
> > In order to make sure that we don't run into buffer overflow issues -
> > http://stackoverflow.com/questions/448563/am-i-correct-that-strcmp-is-equivalent-and-safe-for-literals
>
> Both strings are null terminated.
>
strncmp is needed because if it's a directory pkgfile will end in '/'
but bname will not.
> >>> continue;
> >>> }
> >>>
> >>> /* concatenate our file and the root path */
> >>> - if(rootlen + 1 + strlen(pkgfile) > PATH_MAX) {
> >>> + if(rootlen + 1 + pkgfile_len > PATH_MAX) {
> >>> path[rootlen] = '\0'; /* reset path for error message */
> >>> pm_printf(ALPM_LOG_ERROR, _("path too long: %s%s\n"), path, pkgfile);
> >>> continue;
> >>> }
> >>> strcpy(path + rootlen, pkgfile);
> >>>
> >>> - pdname = mdirname(path);
> >>> - ppath = realpath(pdname, NULL);
> >>> + /* check that the file exists on disk and is the correct type */
> >>> + if(lstat(path, &buf) == -1 || !is_dir != !S_ISDIR(buf.st_mode)) {
> >>
> >> !!! again. How about:
> >>
> >> if(is_dir && lstat(path, &buf) == 0 && !S_ISDIR(buf.st_mode))
> >>
> >> which is what I think is being tested here.
> > Again, your fix doesn't match the original test - Andrew's test
> > verifies that the file either doesn't exist
> > or that (the file is a directory) XOR is_dir is true.
> >>
> >> But I wonder if we really need this... I believe this is to avoid
> >> "strange" results when people replace directories in a package with
> >> symlinks to directories in another package. The more I think about
> >> this, the more I want to just remove our special case handling of this
> >> in general. A directory in a package and a symlink on the filesystem
> >> should just be a conflict. So unless I am missing another reason for
> >> this, get rid of it.
> >>
> > No comment, as I do not know the codebase well enough to respond.
I didn't really like this in the first place; deleted.
> >>> + continue;
> >>> + }
> >>> +
> >>> + if(is_dir) {
> >>> + pdname = realpath(path, NULL);
> >>> + ppath = mdirname(pdname);
> >>> + } else {
> >>> + pdname = mdirname(path);
> >>> + ppath = realpath(pdname, NULL);
> >>> + }
> >>> free(pdname);
> >>
> >> Umm... why calculate pdname just to free it immediately?
> > Since we need to use it to calculate ppath?
>
> Oops... I blame doing quick review before rushing to the train!
>
I copied this particular bit from above where I do the same thing to
the target, but it's unnecessary work here. A simple strncpy will
actually suffice:
strncpy(path + rootlen, pkgfile, pkgfile_len - pbname_len);
path[rootlen + pkgfile_len - pbname_len] = '\0';
ppath = realpath(path, NULL);
> >>
> >>> if(!ppath) {
> >>>
> >>
> >>
> >
> > Overall, I can't really judge the quality of Andrew's patch, but I definitely
> > do not agree with the comments given by Allan.
> >
> > HTH in my own way,
> >
> > Gesh
> >
> >
> >
>
>
More information about the pacman-dev
mailing list