Skip to content

Commit

Permalink
kernel: validate {CHANGED_,SET_,}METHODS_OPERATION args
Browse files Browse the repository at this point in the history
This ensures we don't return garbage resp. don't trigger an
assertion (in debug builds) when e.g. passing an argument list
with 7 or more entries to ApplicableMethod.
  • Loading branch information
fingolfin committed Jan 6, 2019
1 parent 8f8ea81 commit 4b70218
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 22 deletions.
26 changes: 16 additions & 10 deletions src/opers.c
Original file line number Diff line number Diff line change
Expand Up @@ -3146,13 +3146,13 @@ void SaveOperationExtras (
SaveSubObj(header->setter);
SaveSubObj(header->tester);
SaveSubObj(header->extra);
for (UInt i = 0; i <= 7; i++)
for (UInt i = 0; i <= MAX_OPER_ARGS; i++)
SaveSubObj(header->methods[i]);
#ifdef HPCGAP
// FIXME: We probably don't want to save/restore the cache?
// (and that would include "normal" GAP, too...)
#else
for (UInt i = 0; i <= 7; i++)
for (UInt i = 0; i <= MAX_OPER_ARGS; i++)
SaveSubObj(header->cache[i]);
#endif
}
Expand All @@ -3177,13 +3177,13 @@ void LoadOperationExtras (
header->setter = LoadSubObj();
header->tester = LoadSubObj();
header->extra = LoadSubObj();
for (UInt i = 0; i <= 7; i++)
for (UInt i = 0; i <= MAX_OPER_ARGS; i++)
header->methods[i] = LoadSubObj();
#ifdef HPCGAP
// FIXME: We probably don't want to save/restore the cache?
// (and that would include "normal" GAP, too...)
#else
for (UInt i = 0; i <= 7; i++)
for (UInt i = 0; i <= MAX_OPER_ARGS; i++)
header->cache[i] = LoadSubObj();
#endif
}
Expand Down Expand Up @@ -3432,8 +3432,10 @@ Obj FuncMETHODS_OPERATION (
Obj meth;

RequireOperation(oper);
RequireNonnegativeSmallInt("METHODS_OPERATION", narg);
n = INT_INTOBJ( narg );
n = IS_INTOBJ(narg) ? INT_INTOBJ(narg) : -1;
if (n < 0 || n > MAX_OPER_ARGS)
RequireArgument("METHODS_OPERATION", narg, "narg",
"must be an integer between 0 and 6");
meth = MethsOper( oper, (UInt)n );
#ifdef HPCGAP
MEMBAR_READ();
Expand All @@ -3457,13 +3459,15 @@ Obj FuncCHANGED_METHODS_OPERATION (
Int i;

RequireOperation(oper);
RequireNonnegativeSmallInt("CHANGED_METHODS_OPERATION", narg);
n = IS_INTOBJ(narg) ? INT_INTOBJ(narg) : -1;
if (n < 0 || n > MAX_OPER_ARGS)
RequireArgument("CHANGED_METHODS_OPERATION", narg, "narg",
"must be an integer between 0 and 6");
#ifdef HPCGAP
if (!PreThreadCreation) {
ErrorQuit("Methods may only be changed before thread creation",0L,0L);
}
#endif
n = INT_INTOBJ( narg );
cacheBag = CacheOper( oper, (UInt) n );
cache = ADDR_OBJ( cacheBag );
for ( i = 1; i < SIZE_OBJ(cacheBag) / sizeof(Obj); i++ ) {
Expand All @@ -3486,8 +3490,10 @@ Obj FuncSET_METHODS_OPERATION (
Int n;

RequireOperation(oper);
RequireNonnegativeSmallInt("SET_METHODS_OPERATION", narg);
n = INT_INTOBJ( narg );
n = IS_INTOBJ(narg) ? INT_INTOBJ(narg) : -1;
if (n < 0 || n > MAX_OPER_ARGS)
RequireArgument("SET_METHODS_OPERATION", narg, "narg",
"must be an integer between 0 and 6");
#ifdef HPCGAP
MEMBAR_WRITE();
#endif
Expand Down
17 changes: 11 additions & 6 deletions src/opers.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@
#include "calls.h"
#include "system.h"


enum {
MAX_OPER_ARGS = 6
};

/****************************************************************************
**
**
Expand All @@ -42,10 +47,10 @@ typedef struct {
Obj tester;

// method list of an operation
Obj methods[8];
Obj methods[MAX_OPER_ARGS+1];

// cache of an operation
Obj cache[8];
Obj cache[MAX_OPER_ARGS+1];

// small integer encoding a set of bit flags with information about the
// operation, see OperExtras below
Expand Down Expand Up @@ -177,13 +182,13 @@ static inline void SET_TESTR_FILT(Obj oper, Obj x)
*/
static inline Obj METHS_OPER(Obj oper, Int i)
{
GAP_ASSERT(0 <= i && i < 8);
GAP_ASSERT(0 <= i && i <= MAX_OPER_ARGS);
return CONST_OPER(oper)->methods[i];
}

static inline void SET_METHS_OPER(Obj oper, Int i, Obj x)
{
GAP_ASSERT(0 <= i && i < 8);
GAP_ASSERT(0 <= i && i <= MAX_OPER_ARGS);
OPER(oper)->methods[i] = x;
}

Expand All @@ -194,13 +199,13 @@ static inline void SET_METHS_OPER(Obj oper, Int i, Obj x)
*/
static inline Obj CACHE_OPER(Obj oper, Int i)
{
GAP_ASSERT(0 <= i && i < 8);
GAP_ASSERT(0 <= i && i <= MAX_OPER_ARGS);
return CONST_OPER(oper)->cache[i];
}

static inline void SET_CACHE_OPER(Obj oper, Int i, Obj x)
{
GAP_ASSERT(0 <= i && i < 8);
GAP_ASSERT(0 <= i && i <= MAX_OPER_ARGS);
OPER(oper)->cache[i] = x;
}

Expand Down
25 changes: 19 additions & 6 deletions tst/testinstall/kernel/opers.tst
Original file line number Diff line number Diff line change
Expand Up @@ -214,28 +214,41 @@ Error, operation already installed
gap> METHODS_OPERATION(fail,1);
Error, METHODS_OPERATION: <oper> must be an operation (not the value 'fail')
gap> METHODS_OPERATION(Size,-1);
Error, METHODS_OPERATION: <narg> must be a non-negative small integer (not the\
integer -1)
Error, METHODS_OPERATION: <narg> must be an integer between 0 and 6 (not the i\
nteger -1)
gap> METHODS_OPERATION(Size,0);
[ ]
gap> METHODS_OPERATION(Size,6);
[ ]
gap> METHODS_OPERATION(Size,7);
Error, METHODS_OPERATION: <narg> must be an integer between 0 and 6 (not the i\
nteger 7)

# note: CHANGED_METHODS_OPERATION is not usable on HPC-GAP
gap> CHANGED_METHODS_OPERATION(fail,1);
Error, CHANGED_METHODS_OPERATION: <oper> must be an operation (not the value '\
fail')
gap> CHANGED_METHODS_OPERATION(Size,-1);
Error, CHANGED_METHODS_OPERATION: <narg> must be a non-negative small integer \
(not the integer -1)
Error, CHANGED_METHODS_OPERATION: <narg> must be an integer between 0 and 6 (n\
ot the integer -1)
gap> if not IsHPCGAP then CHANGED_METHODS_OPERATION(Size,0); fi;
gap> if not IsHPCGAP then CHANGED_METHODS_OPERATION(Size,6); fi;
gap> CHANGED_METHODS_OPERATION(Size,7);
Error, CHANGED_METHODS_OPERATION: <narg> must be an integer between 0 and 6 (n\
ot the integer 7)

#
gap> SET_METHODS_OPERATION (fail,1,[]);
Error, SET_METHODS_OPERATION: <oper> must be an operation (not the value 'fail\
')
gap> SET_METHODS_OPERATION (Size,-1,[]);
Error, SET_METHODS_OPERATION: <narg> must be a non-negative small integer (not\
the integer -1)
Error, SET_METHODS_OPERATION: <narg> must be an integer between 0 and 6 (not t\
he integer -1)
gap> SET_METHODS_OPERATION (Size,0,[]);
gap> SET_METHODS_OPERATION (Size,6,[]);
gap> SET_METHODS_OPERATION (Size,7,[]);
Error, SET_METHODS_OPERATION: <narg> must be an integer between 0 and 6 (not t\
he integer 7)

#
gap> f:=SETTER_FUNCTION("foobar", IsPGroup);;
Expand Down

0 comments on commit 4b70218

Please sign in to comment.