View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0000188fileGeneralpublic2020-08-26 08:372020-09-05 14:28
Reporterneal Assigned Tochristos  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Fixed in Version5.40 
Summary0000188: nested use of name is unexpected
Descriptionfile doesn't handle non-top level "name" directives well.
Steps To ReproduceConsider the following magic file (also attached):

0 byte x 1
>0 name foo
>>0 byte x in foo
>0 byte x 2
>0 use foo

If this successfully parses, then I'd expect the 'use foo' to succeed. Instead, it executes from top to bottom *including the body of foo*, and then fails at the 'use foo' line. See the trace below.

I can understand not wanting the complexity that non-top level named continuations would introduce. But, in that case, I think the parse should reject files that have named continuations that are not at the top level.
Additional Information$ file -d -m /tmp/nested-name.magic /tmp/byte.bin
unknown, 0: Warning: using regular magic file `/tmp/nest-name.magic'
1: binary
[try zmagic 0]
[try tar 0]
[try json 0]
[try cdf 0]
[try elf 0]
bb=[0x7f227cca8010,256], 0 [b=0x7f227cca8010,256], [o=0, c=0]
mget(type=1, flag=0x20, offset=0, o=0, nbytes=256, il=0, nc=0)
mget/96 @0: \000\001\002\003\004\005\006\a\b\t\n\v\f\r\016\017\020\021\022\023\024\025\026\027\030\031\032\033\034\035\036\037 !"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_

1: > 0 byte&,x,"1"]
0 == *any* = 1
bb=[0x7f227cca8010,256], 0 [b=0x7f227cca8010,256], [o=0, c=1]
mget(type=45, flag=0, offset=0, o=0, nbytes=256, il=0, nc=0)
mget/96 @0: \000\001\002\003\004\005\006\a\b\t\n\v\f\r\016\017\020\021\022\023\024\025\026\027\030\031\032\033\034\035\036\037 !"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_

2: >> 0 name,='foo',""]
bb=[0x7f227cca8010,256], 0 [b=0x7f227cca8010,256], [o=0, c=2]
mget(type=1, flag=0, offset=0, o=0, nbytes=256, il=0, nc=0)
mget/96 @0: \000\001\002\003\004\005\006\a\b\t\n\v\f\r\016\017\020\021\022\023\024\025\026\027\030\031\032\033\034\035\036\037 !"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_

3: >>> 0 byte&,x,"in foo"]
0 == *any* = 1
bb=[0x7f227cca8010,256], 0 [b=0x7f227cca8010,256], [o=0, c=1]
mget(type=1, flag=0, offset=0, o=0, nbytes=256, il=0, nc=0)
mget/96 @0: \000\001\002\003\004\005\006\a\b\t\n\v\f\r\016\017\020\021\022\023\024\025\026\027\030\031\032\033\034\035\036\037 !"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_

4: >> 0 byte&,x,"2"]
0 == *any* = 1
bb=[0x7f227cca8010,256], 0 [b=0x7f227cca8010,256], [o=0, c=1]
mget(type=46, flag=0, offset=0, o=0, nbytes=256, il=0, nc=0)
mget/96 @0: \000\001\002\003\004\005\006\a\b\t\n\v\f\r\016\017\020\021\022\023\024\025\026\027\030\031\032\033\034\035\036\037 !"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_

4: >> 0 use,='foo',""]
[try softmagic -1]
/tmp/byte.bin: ERROR: 1 in foo 2 cannot find entry `foo'
TagsNo tags attached.

Activities

neal

2020-08-26 08:37

reporter  

nested-name.magic (63 bytes)

neal

2020-08-26 12:12

reporter   ~0003464

This fixes the issue for me. I've tried to make the changes as non-invasive as possible.
0001-Fix-use.patch (10,365 bytes)    
From a92c756ee150bf2e47871fedecfb8486f355920f Mon Sep 17 00:00:00 2001
From: "Neal H. Walfield" <neal@gnu.org>
Date: Wed, 26 Aug 2020 14:02:09 +0200
Subject: [PATCH] Fix 'use'.

  - When 'use' is used, it calls 'match' with the same magic set (ms).

  - 'match' starts counting the continuation levels at 0.

  - When the 'use' call returns, 'ms->c' has been clobbered.

  - Change 'ms->c' to a stack, and push a fresh 'struct cont' before
    calling 'match' or 'parse'.  Pop those when the functions return.

  - Example:

      10 byte x
      >&0 byte x 1:%d
      >&0 use foo
      >&0 byte x 2:%d

      20 name foo
      >&30 byte x 3:%d

      That should print:

        $ src/file -m /tmp/cont_level.magic /tmp/byte.bin
        /tmp/byte.bin: 1:11 3:41 2:11

      Without this patch, it incorrectly prints:

        $ file -m /tmp/cont_level.magic /tmp/byte.bin
        /tmp/byte.bin: 1:11 3:41 2:0

      byte.bin was generated as follows:

        $ for i in $(seq 0 255); do echo -en '\x'$(printf "%x" $i); done > /tmp/byte.bin

  - See issue #188.  https://bugs.astron.com/view.php?id=188
---
 src/apprentice.c | 24 ++++++++++++++----------
 src/der.c        |  2 +-
 src/file.h       |  6 ++++--
 src/funcs.c      | 33 ++++++++++++++++++++++++---------
 src/softmagic.c  | 37 +++++++++++++++++++++++--------------
 5 files changed, 66 insertions(+), 36 deletions(-)

diff --git a/src/apprentice.c b/src/apprentice.c
index 1437bcc1..64df467d 100644
--- a/src/apprentice.c
+++ b/src/apprentice.c
@@ -504,7 +504,9 @@ file_ms_free(struct magic_set *ms)
 		mlist_free(ms->mlist[i]);
 	free(ms->o.pbuf);
 	free(ms->o.buf);
-	free(ms->c.li);
+	while (ms->c) {
+		file_continuation_level_pop(ms);
+	}
 	free(ms);
 }
 
@@ -512,7 +514,7 @@ protected struct magic_set *
 file_ms_alloc(int flags)
 {
 	struct magic_set *ms;
-	size_t i, len;
+	size_t i;
 
 	if ((ms = CAST(struct magic_set *, calloc(CAST(size_t, 1u),
 	    sizeof(struct magic_set)))) == NULL)
@@ -525,10 +527,7 @@ file_ms_alloc(int flags)
 
 	ms->o.buf = ms->o.pbuf = NULL;
 	ms->o.blen = 0;
-	len = (ms->c.len = 10) * sizeof(*ms->c.li);
-
-	if ((ms->c.li = CAST(struct level_info *, malloc(len))) == NULL)
-		goto free;
+	ms->c = NULL;
 
 	ms->event_flags = 0;
 	ms->error = -1;
@@ -1183,6 +1182,7 @@ load_1(struct magic_set *ms, int action, const char *fn, int *errs,
 	char *line = NULL;
 	ssize_t len;
 	struct magic_entry me;
+	int rt;
 
 	FILE *f = fopen(ms->file = fn, "r");
 	if (f == NULL) {
@@ -1241,7 +1241,11 @@ load_1(struct magic_set *ms, int action, const char *fn, int *errs,
 			/*FALLTHROUGH*/
 		default:
 		again:
-			switch (parse(ms, &me, line, lineno, action)) {
+			if (file_check_mem(ms, 0, 1) == -1)
+				return;
+			rt = parse(ms, &me, line, lineno, action);
+			file_continuation_level_pop(ms);
+			switch (rt) {
 			case 0:
 				continue;
 			case 1:
@@ -1676,7 +1680,7 @@ private int
 check_cond(struct magic_set *ms, int cond, uint32_t cont_level)
 {
 	int last_cond;
-	last_cond = ms->c.li[cont_level].last_cond;
+	last_cond = ms->c->li[cont_level].last_cond;
 
 	switch (cond) {
 	case COND_IF:
@@ -1711,7 +1715,7 @@ check_cond(struct magic_set *ms, int cond, uint32_t cont_level)
 		break;
 	}
 
-	ms->c.li[cont_level].last_cond = last_cond;
+	ms->c->li[cont_level].last_cond = last_cond;
 	return 0;
 }
 #endif /* ENABLE_CONDITIONALS */
@@ -1884,7 +1888,7 @@ parse(struct magic_set *ms, struct magic_entry *me, const char *line,
 	}
 #ifdef ENABLE_CONDITIONALS
 	if (cont_level == 0 || cont_level > last_cont_level)
-		if (file_check_mem(ms, cont_level) == -1)
+		if (file_check_mem(ms, cont_level, 0) == -1)
 			return -1;
 	last_cont_level = cont_level;
 #endif
diff --git a/src/der.c b/src/der.c
index 4bee9f16..84df577b 100644
--- a/src/der.c
+++ b/src/der.c
@@ -290,7 +290,7 @@ der_offs(struct magic_set *ms, struct magic *m, size_t nbytes)
 	if (m->cont_level != 0) {
 		if (offs + tlen > nbytes)
 			return -1;
-		ms->c.li[m->cont_level - 1].off = CAST(int, offs + tlen);
+		ms->c->li[m->cont_level - 1].off = CAST(int, offs + tlen);
 		DPRINTF(("cont_level[%u] = %u\n", m->cont_level - 1,
 		    ms->c.li[m->cont_level - 1].off));
 	}
diff --git a/src/file.h b/src/file.h
index 8fb9e060..7298b424 100644
--- a/src/file.h
+++ b/src/file.h
@@ -417,7 +417,8 @@ struct magic_set {
 	struct cont {
 		size_t len;
 		struct level_info *li;
-	} c;
+		struct cont *next;
+	} *c;
 	struct out {
 		char *buf;		/* Accumulation buffer */
 		size_t blen;		/* Length of buffer */
@@ -526,7 +527,8 @@ protected void file_showstr(FILE *, const char *, size_t);
 protected size_t file_mbswidth(const char *);
 protected const char *file_getbuffer(struct magic_set *);
 protected ssize_t sread(int, void *, size_t, int);
-protected int file_check_mem(struct magic_set *, unsigned int);
+protected void file_continuation_level_pop(struct magic_set *);
+protected int file_check_mem(struct magic_set *, unsigned int, int);
 protected int file_looks_utf8(const unsigned char *, size_t, file_unichar_t *,
     size_t *);
 protected size_t file_pstring_length_size(struct magic_set *,
diff --git a/src/funcs.c b/src/funcs.c
index ecbfa28c..55e96435 100644
--- a/src/funcs.c
+++ b/src/funcs.c
@@ -576,28 +576,43 @@ file_getbuffer(struct magic_set *ms)
 }
 
 protected int
-file_check_mem(struct magic_set *ms, unsigned int level)
+file_check_mem(struct magic_set *ms, unsigned int level,
+	       int push_continuation_stack)
 {
 	size_t len;
 
-	if (level >= ms->c.len) {
-		len = (ms->c.len = 20 + level) * sizeof(*ms->c.li);
-		ms->c.li = CAST(struct level_info *, (ms->c.li == NULL) ?
+	if (push_continuation_stack) {
+		struct cont *c = calloc(1, sizeof *c);
+		if (c == NULL) {
+			file_oomem(ms, sizeof *c);
+		}
+		c->next = ms->c;
+		ms->c = c;
+	}
+	if (level >= ms->c->len) {
+		len = (ms->c->len = 20 + level) * sizeof(*ms->c->li);
+		ms->c->li = CAST(struct level_info *, (ms->c->li == NULL) ?
 		    malloc(len) :
-		    realloc(ms->c.li, len));
-		if (ms->c.li == NULL) {
+		    realloc(ms->c->li, len));
+		if (ms->c->li == NULL) {
 			file_oomem(ms, len);
 			return -1;
 		}
 	}
-	ms->c.li[level].got_match = 0;
+	ms->c->li[level].got_match = 0;
 #ifdef ENABLE_CONDITIONALS
-	ms->c.li[level].last_match = 0;
-	ms->c.li[level].last_cond = COND_NONE;
+	ms->c->li[level].last_match = 0;
+	ms->c->li[level].last_cond = COND_NONE;
 #endif /* ENABLE_CONDITIONALS */
 	return 0;
 }
 
+protected void
+file_continuation_level_pop(struct magic_set *ms)
+{
+	ms->c = ms->c->next;
+}
+
 protected size_t
 file_printedlen(const struct magic_set *ms)
 {
diff --git a/src/softmagic.c b/src/softmagic.c
index 95061e56..c27209ad 100644
--- a/src/softmagic.c
+++ b/src/softmagic.c
@@ -130,12 +130,18 @@ file_softmagic(struct magic_set *ms, const struct buffer *b,
 		indir_count = &ic;
 	}
 
+	if (file_check_mem(ms, 0, 1) == -1)
+		return -1;
+
 	for (ml = ms->mlist[0]->next; ml != ms->mlist[0]; ml = ml->next)
 		if ((rv = match(ms, ml->magic, ml->nmagic, b, 0, mode,
 		    text, 0, indir_count, name_count,
-		    &printed_something, &need_separator, NULL, NULL)) != 0)
+		    &printed_something, &need_separator, NULL, NULL)) != 0) {
+			file_continuation_level_pop(ms);
 			return rv;
+		}
 
+	file_continuation_level_pop(ms);
 	return 0;
 }
 
@@ -214,7 +220,7 @@ match(struct magic_set *ms, struct magic *magic, uint32_t nmagic,
 	if (found_match == NULL)
 		found_match = &found_matchv;
 
-	if (file_check_mem(ms, cont_level) == -1)
+	if (file_check_mem(ms, cont_level, 0) == -1)
 		return -1;
 
 	for (magindex = 0; magindex < nmagic; magindex++) {
@@ -302,7 +308,7 @@ flush:
 				return -1;
 		}
 
-		switch (moffset(ms, m, &bb, &ms->c.li[cont_level].off)) {
+		switch (moffset(ms, m, &bb, &ms->c->li[cont_level].off)) {
 		case -1:
 		case 0:
 			goto flush;
@@ -311,7 +317,7 @@ flush:
 		}
 
 		/* and any continuations that match */
-		if (file_check_mem(ms, ++cont_level) == -1)
+		if (file_check_mem(ms, ++cont_level, 0) == -1)
 			return -1;
 
 		while (magindex + 1 < nmagic &&
@@ -339,13 +345,13 @@ flush:
 					return 0;
 				}
 				ms->offset +=
-				    ms->c.li[cont_level - 1].off;
+				    ms->c->li[cont_level - 1].off;
 			}
 
 #ifdef ENABLE_CONDITIONALS
 			if (m->cond == COND_ELSE ||
 			    m->cond == COND_ELIF) {
-				if (ms->c.li[cont_level].last_match == 1)
+				if (ms->c->li[cont_level].last_match == 1)
 					continue;
 			}
 #endif
@@ -375,20 +381,20 @@ flush:
 				return -1;
 			case 0:
 #ifdef ENABLE_CONDITIONALS
-				ms->c.li[cont_level].last_match = 0;
+				ms->c->li[cont_level].last_match = 0;
 #endif
 				break;
 			default:
 #ifdef ENABLE_CONDITIONALS
-				ms->c.li[cont_level].last_match = 1;
+				ms->c->li[cont_level].last_match = 1;
 #endif
 				if (m->type == FILE_CLEAR)
-					ms->c.li[cont_level].got_match = 0;
-				else if (ms->c.li[cont_level].got_match) {
+					ms->c->li[cont_level].got_match = 0;
+				else if (ms->c->li[cont_level].got_match) {
 					if (m->type == FILE_DEFAULT)
 						break;
 				} else
-					ms->c.li[cont_level].got_match = 1;
+					ms->c->li[cont_level].got_match = 1;
 
 				if (*m->desc)
 					*found_match = 1;
@@ -432,7 +438,7 @@ flush:
 				}
 
 				switch (moffset(ms, m, &bb,
-				    &ms->c.li[cont_level].off)) {
+				    &ms->c->li[cont_level].off)) {
 				case -1:
 				case 0:
 					flush = 1;
@@ -447,7 +453,7 @@ flush:
 				 * at a higher level,
 				 * process them.
 				 */
-				if (file_check_mem(ms, ++cont_level) == -1)
+				if (file_check_mem(ms, ++cont_level, 0) == -1)
 					return -1;
 				break;
 			}
@@ -1743,7 +1749,7 @@ mget(struct magic_set *ms, struct magic *m, const struct buffer *b,
 					    "indirect *zero* cont_level\n");
 				return 0;
 			}
-			offset += ms->c.li[cont_level - 1].off;
+			offset += ms->c->li[cont_level - 1].off;
 			if (offset == 0) {
 				if ((ms->flags & MAGIC_DEBUG) != 0)
 					fprintf(stderr,
@@ -1880,9 +1886,12 @@ mget(struct magic_set *ms, struct magic *m, const struct buffer *b,
 		oneed_separator = *need_separator;
 		if (m->flag & NOSPACE)
 			*need_separator = 0;
+		if (file_check_mem(ms, 0, 1) == -1)
+			return -1;
 		rv = match(ms, ml.magic, ml.nmagic, b, offset + o,
 		    mode, text, flip, indir_count, name_count,
 		    printed_something, need_separator, returnval, found_match);
+		file_continuation_level_pop(ms);
 		(*name_count)--;
 		if (rv != 1)
 		    *need_separator = oneed_separator;
-- 
2.20.1
0001-Fix-use.patch (10,365 bytes)   

christos

2020-09-05 14:28

manager   ~0003474

Fixed, thanks!
mag2, 2: Warning: `name foo' entries can only be declared at top level

Issue History

Date Modified Username Field Change
2020-08-26 08:37 neal New Issue
2020-08-26 08:37 neal File Added: nested-name.magic
2020-08-26 12:12 neal File Added: 0001-Fix-use.patch
2020-08-26 12:12 neal Note Added: 0003464
2020-09-05 14:27 christos Assigned To => christos
2020-09-05 14:27 christos Status new => assigned
2020-09-05 14:28 christos Status assigned => resolved
2020-09-05 14:28 christos Resolution open => fixed
2020-09-05 14:28 christos Fixed in Version => 5.40
2020-09-05 14:28 christos Note Added: 0003474