Exposing internal representation - security?

[Joe Attardi]

I have some table model classes that contain String[] objects that
contain the column names. Now, since a String[] is mutable, even if the
String[] is declared final, someone calling getColumnNames() could
change the column names.

Would it be overkill to, in getColumnNames(), return a copy of the
array instead via:
return (String[]) columnNames.clone();

or would that be considered a good practice so that the column names
cannot be changed by ill-mannered code?

Thanks in advance..

 

[Mike Schilling]

I have some table model classes that contain String[] objects that
contain the column names. Now, since a String[] is mutable, even if the
String[] is declared final, someone calling getColumnNames() could
change the column names.

Would it be overkill to, in getColumnNames(), return a copy of the
array instead via:
return (String[]) columnNames.clone();

or would that be considered a good practice so that the column names
cannot be changed by ill-mannered code?

The latter; it's a good idea.

Alternatively, you could define getColumnNames to return a List, and do
something like

return new ArrayList(Arrays.asList(columnNames));

or

return Collections.unmodifiableList(Arrays.asList(columnNames));

 

[Oliver Wong]

I have some table model classes that contain String[] objects that
contain the column names. Now, since a String[] is mutable, even if the
String[] is declared final, someone calling getColumnNames() could
change the column names.

Would it be overkill to, in getColumnNames(), return a copy of the
array instead via:
return (String[]) columnNames.clone();

or would that be considered a good practice so that the column names
cannot be changed by ill-mannered code?

Thanks in advance..

Is this for security, e.g. you have rogue programmers that may screw
around with your program, or is it for code correctness, e.g. you might
accidentally change the variable, but you want to be able to detect these
changes as bugs and fix them?

- Oliver

 

[Joe Attardi]

Is this for security, e.g. you have rogue programmers that may screw
around with your program, or is it for code correctness, e.g. you might
accidentally change the variable, but you want to be able to detect these
changes as bugs and fix them?

Mostly for correctness. Actually, I ran FindBugs
[http://findbugs.sourceforge.net/] on the code and that was one of the
potential flaws it uncovered.

 

[Eric Sosman]

Joe Attardi wrote On 03/21/06 10:46,:

I have some table model classes that contain String[] objects that
contain the column names. Now, since a String[] is mutable, even if the
String[] is declared final, someone calling getColumnNames() could
change the column names.

Would it be overkill to, in getColumnNames(), return a copy of the
array instead via:
return (String[]) columnNames.clone();

or would that be considered a good practice so that the column names
cannot be changed by ill-mannered code?

I'd start by asking what harm will come if the
column names are changed, and to whom. If the name-
changing rogue can only hurt himself, how much effort
should you expend to shield him from his own folly?
If he can hurt others, though, ...

Also, consider intermediate-level protection schemes.
If the column headers are doing double duty as database
field names or something (and hence mustn't be changed
lest your communications with the database go awry), it
might be better to separate the roles: Keep one closely-
guarded array of database names, and another "decorative"
array of column headers. Let the rogue mess with the
headers if he likes (and if no other harm ensues), while
keeping the database field names hidden away out of
harm's reach.

 

[Oliver Wong]

Is this for security, e.g. you have rogue programmers that may screw
around with your program, or is it for code correctness, e.g. you might
accidentally change the variable, but you want to be able to detect these
changes as bugs and fix them?

Mostly for correctness. Actually, I ran FindBugs
[http://findbugs.sourceforge.net/] on the code and that was one of the
potential flaws it uncovered.

As another poster pointed out, you can use the Collections class' "give
me an immutable version of this list" methods for this purpose. I believe
that this method just wraps your collection, rather than duplicating all the
pointers within it, thus saving a bit of memory and time over cloning.
However, cloning may be more secure in that rogue programmers could use
reflection and casting and stuff like that to gain access to the underlying
(mutable) collection.

- Oliver

 

[Joe Attardi]

I'd start by asking what harm will come if the
column names are changed, and to whom.

Also, consider intermediate-level protection schemes.
If the column headers are doing double duty as database
field names or something (and hence mustn't be changed
lest your communications with the database go awry), it
might be better to separate the roles

That is a good point, Eric, and the answer to your first question is,
not much harm at all. The column names are just for display purposes.
The column values are tied to integer constants for each column, i.e.

private static final int COLUMN_NAME = 0;
private static final int COLUMN_AGE = 1;

....

switch (column)
{
case COLUMN_NAME:
value = rowObject.getName();
break;
case COLUMN_AGE:
value = rowObject.getAge();
break;
}

....

etc.

 

[Thomas Hawtin]

Would it be overkill to, in getColumnNames(), return a copy of the
array instead via:
return (String[]) columnNames.clone();

From 1.5 you can write:

return columnNames.clone();

or would that be considered a good practice so that the column names
cannot be changed by ill-mannered code?

You should always make a defensive copy when passing mutable values into
or out of an object.

Tom Hawtin

 

[Mike Schilling]

Joe Attardi wrote On 03/21/06 10:46,:

I have some table model classes that contain String[] objects that
contain the column names. Now, since a String[] is mutable, even if the
String[] is declared final, someone calling getColumnNames() could
change the column names.

Would it be overkill to, in getColumnNames(), return a copy of the
array instead via:
return (String[]) columnNames.clone();

or would that be considered a good practice so that the column names
cannot be changed by ill-mannered code?

I'd start by asking what harm will come if the
column names are changed, and to whom. If the name-
changing rogue can only hurt himself, how much effort
should you expend to shield him from his own folly?
If he can hurt others, though, ...

I don't understand the distinction between himself and others. If passing
the raw array allows bugs to be introduced either from malice or
misunderstanding, then don't do it. It's much cheaper to prevent the
problem than to debug and fix it

 

[Joe Attardi]

I'd start by asking what harm will come if the
column names are changed, and to whom.

Also, consider intermediate-level protection schemes.
If the column headers are doing double duty as database
field names or something (and hence mustn't be changed
lest your communications with the database go awry), it
might be better to separate the roles

That is a good point, Eric, and the answer to your first question is,
not much harm at all. The column names are just for display purposes.
The column values are tied to integer constants for each column, i.e.

private static final int COLUMN_NAME = 0;
private static final int COLUMN_AGE = 1;

....

switch (column)
{
case COLUMN_NAME:
value = rowObject.getName();
break;
case COLUMN_AGE:
value = rowObject.getAge();
break;
}

....

etc.

 

[Oliver Wong]

I don't understand the distinction between himself and others. If passing
the raw array allows bugs to be introduced either from malice or
misunderstanding, then don't do it. It's much cheaper to prevent the
problem than to debug and fix it

It's like the how on a typical file server, you can delete, muck around
with, or otherwise damage your own files, but you can't damage other
people's files.

If the array contains a billion elements, it may be very costly to
create a duplicate of the array. While you wouldn't nescessarily need to
duplicate every element, you'd have to at least duplicate all of the
pointers.

- Oliver

 

[Joe Attardi]

If the array contains a billion elements, it may be very costly to
create a duplicate of the array. While you wouldn't nescessarily need to
duplicate every element, you'd have to at least duplicate all of the
pointers.

The arrays are very small, and are of a fixed size. The biggest table
in the app has like 9 columns, so the biggest array is an int[9]. So I
suppose it's cheap enough to just do a shallow copy, so I might as well.

 

[Mike Schilling]

It's like the how on a typical file server, you can delete, muck around
with, or otherwise damage your own files, but you can't damage other
people's files.

But objects don't have owners, at least in Java. Corrupted data has a way
of causing bugs in the oddest places.

If the array contains a billion elements, it may be very costly to
create a duplicate of the array. While you wouldn't nescessarily need to
duplicate every element, you'd have to at least duplicate all of the
pointers.

If the objects themselves are immutable (as they are here, since they're
Strings) , all you need is

Collections.unmodifiableList(Arrays.asList(array));

Two additional objects, both small and independent of the size of the array.
(Arrays$ArrayList seems to have only two fields, a pointer to the array and
a modification count for the fast-fail iterators.
Collections$UnmodifiableList also has two, both pointers to the list.)

 

[Oliver Wong]

[...]

If the objects themselves are immutable (as they are here, since they're
Strings) , all you need is

Collections.unmodifiableList(Arrays.asList(array));

See other posts I've been making on this thread about the distinction
between security (e.g. hurting others) and bug prevention (e.g. hurting
oneself).

- Oliver