Now thinking again I see no much point in my diff, I prefer yours as
it's easier to update to a newer file(1), no point in changing only
this.
ok from me.
On Wed, Sep 28, 2011 at 12:33:13AM -0300, Christiano F. Haesbaert wrote:
> On Thu, Sep 22, 2011 at 03:56:11PM +0100, Edd Barrett wrote:
> > Hi,
> >
> > An upstream of one of the ports I work on (radare2) has imported our file(1)
> > implementation and claims to have found bugs. This is the first:
> >
> > 'ms->file' will only be assigned to 'fn' after a call to apprentice_load()
> > and
> > ultimately load_1(), so the file_magwarn() at line 274 would report the
> > default
> > filename "unknown".
> >
> > We can trigger this behaviour by executing `file -c`:
> > unknown, 0: Warning: using regular magic file `/etc/magic'
> >
> > Is it a bug?
> >
> > Index: apprentice.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/file/apprentice.c,v
> > retrieving revision 1.29
> > diff -u -r1.29 apprentice.c
> > --- apprentice.c 11 Nov 2009 16:21:51 -0000 1.29
> > +++ apprentice.c 22 Sep 2011 14:27:17 -0000
> > @@ -258,6 +258,7 @@
> > return -1;
> > }
> >
> > + ms->file = fn;
> > if (action == FILE_COMPILE) {
> > rv = apprentice_load(ms, &magic, &nmagic, fn, action);
> > if (rv != 0)
>
> Seems correct to me, but this "fn" seems kinda redundant down the
> stack, except for load_1() which really needs a file and not a
> directory.
>
> So I thought we could kill this fn by assigning ms->file right in the
> beginning at file_apprentice(). Also tweak load_1() so it won't do the
> whole processing in an else.
>
> Index: apprentice.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/file/apprentice.c,v
> retrieving revision 1.29
> diff -d -u -p -w -r1.29 apprentice.c
> --- apprentice.c 11 Nov 2009 16:21:51 -0000 1.29
> +++ apprentice.c 28 Sep 2011 03:18:29 -0000
> @@ -96,11 +96,11 @@ private int parse(struct magic_set *, st
> private int parse_mime(struct magic_set *, struct magic_entry **, uint32_t *,
> const char *);
> private void eatsize(const char **);
> -private int apprentice_1(struct magic_set *, const char *, int, struct mlist
> *);
> +private int apprentice_1(struct magic_set *, int, struct mlist *);
> private size_t apprentice_magic_strength(const struct magic *);
> private int apprentice_sort(const void *, const void *);
> private int apprentice_load(struct magic_set *, struct magic **, uint32_t *,
> - const char *, int);
> + int);
> private void byteswap(struct magic *, uint32_t);
> private void bs1(struct magic *);
> private uint16_t swap2(uint16_t);
> @@ -242,7 +242,7 @@ init_file_tables(void)
> * Handle one file or directory.
> */
> private int
> -apprentice_1(struct magic_set *ms, const char *fn, int action,
> +apprentice_1(struct magic_set *ms, int action,
> struct mlist *mlist)
> {
> struct magic *magic = NULL;
> @@ -259,19 +259,19 @@ apprentice_1(struct magic_set *ms, const
> }
>
> if (action == FILE_COMPILE) {
> - rv = apprentice_load(ms, &magic, &nmagic, fn, action);
> + rv = apprentice_load(ms, &magic, &nmagic, action);
> if (rv != 0)
> return -1;
> - rv = apprentice_compile(ms, &magic, &nmagic, fn);
> + rv = apprentice_compile(ms, &magic, &nmagic, ms->file);
> free(magic);
> return rv;
> }
>
> #ifndef COMPILE_ONLY
> - if ((rv = apprentice_map(ms, &magic, &nmagic, fn)) == -1) {
> + if ((rv = apprentice_map(ms, &magic, &nmagic, ms->file)) == -1) {
> if (ms->flags & MAGIC_CHECK)
> - file_magwarn(ms, "using regular magic file `%s'", fn);
> - rv = apprentice_load(ms, &magic, &nmagic, fn, action);
> + file_magwarn(ms, "using regular magic file `%s'",
> ms->file);
> + rv = apprentice_load(ms, &magic, &nmagic, action);
> if (rv != 0)
> return -1;
> }
> @@ -359,7 +359,8 @@ file_apprentice(struct magic_set *ms, co
> *p++ = '\0';
> if (*fn == '\0')
> break;
> - file_err = apprentice_1(ms, fn, action, mlist);
> + ms->file = fn;
> + file_err = apprentice_1(ms, action, mlist);
> errs = MAX(errs, file_err);
> fn = p;
> }
> @@ -571,13 +572,15 @@ load_1(struct magic_set *ms, int action,
> {
> char line[BUFSIZ];
> size_t lineno = 0;
> - FILE *f = fopen(ms->file = fn, "r");
> - if (f == NULL) {
> + FILE *f;
> +
> + if ((f = fopen(fn, "r")) == NULL) {
> if (errno != ENOENT)
> file_error(ms, errno, "cannot read magic file `%s'",
> fn);
> (*errs)++;
> - } else {
> + return;
> + }
> /* read and parse this file */
> for (ms->line = 1; fgets(line, sizeof(line), f) != NULL;
> ms->line++) {
> size_t len;
> @@ -606,7 +609,6 @@ load_1(struct magic_set *ms, int action,
>
> (void)fclose(f);
> }
> -}
>
> /*
> * parse a file or directory of files
> @@ -614,7 +616,7 @@ load_1(struct magic_set *ms, int action,
> */
> private int
> apprentice_load(struct magic_set *ms, struct magic **magicp, uint32_t
> *nmagicp,
> - const char *fn, int action)
> + int action)
> {
> int errs = 0;
> struct magic_entry *marray;
> @@ -625,7 +627,6 @@ apprentice_load(struct magic_set *ms, st
> struct dirent *d;
>
> ms->flags |= MAGIC_CHECK; /* Enable checks for parsed files */
> -
> maxmagic = MAXMAGIS;
> if ((marray = calloc(maxmagic, sizeof(*marray))) == NULL) {
> file_oomem(ms, maxmagic * sizeof(*marray));
> @@ -638,12 +639,12 @@ apprentice_load(struct magic_set *ms, st
> (void)fprintf(stderr, "%s\n", usg_hdr);
>
> /* load directory or file */
> - if (stat(fn, &st) == 0 && S_ISDIR(st.st_mode)) {
> - dir = opendir(fn);
> + if (stat(ms->file, &st) == 0 && S_ISDIR(st.st_mode)) {
> + dir = opendir(ms->file);
> if (dir) {
> while (d = readdir(dir)) {
> snprintf(subfn, sizeof(subfn), "%s/%s",
> - fn, d->d_name);
> + ms->file, d->d_name);
> if (stat(subfn, &st) == 0 &&
> S_ISREG(st.st_mode)) {
> load_1(ms, action, subfn, &errs,
> &marray, &marraycount);
> @@ -653,7 +654,7 @@ apprentice_load(struct magic_set *ms, st
> } else
> errs++;
> } else
> - load_1(ms, action, fn, &errs, &marray, &marraycount);
> + load_1(ms, action, ms->file, &errs, &marray, &marraycount);
> if (errs)
> goto out;
