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;
