Skip to content

Commit

Permalink
DFSClient: BinaryRecordWriter NPE with null UTF8 strings (#629)
Browse files Browse the repository at this point in the history 
- Fixed BinaryRecordWriter pathways to check for null elements
- Added a new null element test

Signed-off-by: James McMullan [email protected]

Signed-off-by: James McMullan [email protected]
  • Loading branch information
@jpmcmu
jpmcmu authored Aug 10, 2023
1 parent 760dc98 commit 69444f3
Show file tree
Hide file tree
Showing 3 changed files with 117 additions and 31 deletions.
61 changes: 39 additions & 22 deletions dfsclient/src/main/java/org/hpccsystems/dfs/client/BinaryRecordWriter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,18 @@ private long calculateFieldSize(FieldDef fd, IRecordAccessor recordAccessor, Obj
case SET:
case DATASET:
{
long dataLen = BinaryRecordWriter.DataLenFieldSize;
boolean isSet = fd.getDef(0).getFieldType() != FieldType.RECORD;
if (isSet)
{
dataLen++;
}

if (fieldValue == null)
{
return dataLen;
}

List<Object> listValue = null;
if (fieldValue instanceof List)
{
Expand All @@ -662,13 +674,6 @@ private long calculateFieldSize(FieldDef fd, IRecordAccessor recordAccessor, Obj
throw new Exception("Error writing list. Expected List, got: " + fieldValue.getClass().getName());
}

long dataLen = BinaryRecordWriter.DataLenFieldSize;
boolean isSet = fd.getDef(0).getFieldType() != FieldType.RECORD;
if (isSet)
{
dataLen++;
}

for (Object o : listValue)
{
dataLen += calculateFieldSize(fd.getDef(0), recordAccessor, o);
Expand All @@ -681,6 +686,10 @@ private long calculateFieldSize(FieldDef fd, IRecordAccessor recordAccessor, Obj
{
return fd.getDataLen();
}
else if (fieldValue == null)
{
return BinaryRecordWriter.DataLenFieldSize;
}
else
{
byte[] data = (byte[]) fieldValue;
Expand Down Expand Up @@ -741,6 +750,27 @@ else if (fd.getSourceType() == HpccSrcType.QSTRING)
return dataLen;
}

long dataLen = 0;
if (fd.getFieldType() == FieldType.STRING)
{
dataLen = BinaryRecordWriter.DataLenFieldSize;
}
else
{
int eosLen = 1;
if (fd.getSourceType().isUTF16())
{
eosLen++;
}

dataLen = eosLen;
}

if (fieldValue == null)
{
return dataLen;
}

String value = (String) fieldValue;
byte[] data = null;

Expand Down Expand Up @@ -775,21 +805,8 @@ else if (fd.getSourceType() == HpccSrcType.QSTRING)
"Unsupported string encoding type: " + fd.getSourceType() + " encountered while writing field: " + fd.getFieldName());
}

if (fd.getFieldType() == FieldType.STRING)
{
return data.length + BinaryRecordWriter.DataLenFieldSize;
}
else
{
int eosLen = 1;
if (fd.getSourceType().isUTF16())
{
eosLen++;
}

return data.length + eosLen;
}

dataLen += data.length;
return dataLen;
}
case RECORD:
{
Expand Down
18 changes: 11 additions & 7 deletions dfsclient/src/main/java/org/hpccsystems/dfs/client/HPCCRecordAccessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import org.hpccsystems.commons.ecl.FieldDef;
import org.hpccsystems.commons.ecl.FieldType;

/**
/**
* Allows consumers of IRecordAccessor to access data within an @see org.hpccsystems.dfs.client.HPCCRecord.
*/
public class HPCCRecordAccessor implements IRecordAccessor
Expand Down Expand Up @@ -61,12 +61,11 @@ public HPCCRecordAccessor(FieldDef fieldDef)
childRecordAccessors[i] = null;
}
}

}

/*
* (non-Javadoc)
*
*
* @see org.hpccsystems.dfs.client.IRecordAccessor#setRecord(java.lang.Object)
*/
public IRecordAccessor setRecord(Object rd)
Expand All @@ -77,7 +76,7 @@ public IRecordAccessor setRecord(Object rd)

/*
* (non-Javadoc)
*
*
* @see org.hpccsystems.dfs.client.IRecordAccessor#getNumFields()
*/
public int getNumFields()
Expand All @@ -87,17 +86,22 @@ public int getNumFields()

/*
* (non-Javadoc)
*
*
* @see org.hpccsystems.dfs.client.IRecordAccessor#getFieldValue(int)
*/
public Object getFieldValue(int index)
{
if (this.record == null)
{
return null;
}

return this.record.getField(index);
}

/*
* (non-Javadoc)
*
*
* @see org.hpccsystems.dfs.client.IRecordAccessor#getFieldDefinition(int)
*/
public FieldDef getFieldDefinition(int index)
Expand All @@ -107,7 +111,7 @@ public FieldDef getFieldDefinition(int index)

/*
* (non-Javadoc)
*
*
* @see org.hpccsystems.dfs.client.IRecordAccessor#getChildRecordAccessor(int)
*/
public IRecordAccessor getChildRecordAccessor(int index)
Expand Down
69 changes: 67 additions & 2 deletions dfsclient/src/test/java/org/hpccsystems/dfs/client/DFSReadWriteTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,71 @@ else if (field instanceof List)
}
}

@Test
public void nullElementTests()
{
FieldDef[] stringSetElemFD = new FieldDef[1];
stringSetElemFD[0] = new FieldDef("strValue", FieldType.STRING, "UTF8", 4, false, false, HpccSrcType.UTF8, new FieldDef[0]);

FieldDef[] childDatasetElemFD = new FieldDef[1];
{
FieldDef[] fieldDefs = new FieldDef[1];
fieldDefs[0] = new FieldDef("strValue", FieldType.STRING, "UTF8", 4, false, false, HpccSrcType.UTF8, new FieldDef[0]);
childDatasetElemFD[0] = new FieldDef("RootRecord", FieldType.RECORD, "rec", 4, false, false, HpccSrcType.LITTLE_ENDIAN, fieldDefs);
}

FieldDef recordDef = null;
{
FieldDef[] fieldDefs = new FieldDef[3];
fieldDefs[0] = new FieldDef("stringSet", FieldType.SET, "SET", 4, false, false, HpccSrcType.LITTLE_ENDIAN, stringSetElemFD);
fieldDefs[1] = new FieldDef("childDataset", FieldType.DATASET, "DATASET", 4, false, false, HpccSrcType.LITTLE_ENDIAN, childDatasetElemFD);
fieldDefs[2] = new FieldDef("binaryField", FieldType.BINARY, "BINARY", 128, true, false, HpccSrcType.LITTLE_ENDIAN, new FieldDef[0]);
recordDef = new FieldDef("RootRecord", FieldType.RECORD, "rec", 4, false, false, HpccSrcType.LITTLE_ENDIAN, fieldDefs);
}

List<String> stringSet = new ArrayList<String>();
List<HPCCRecord> childDataSet = new ArrayList<HPCCRecord>();
for (int i = 0; i < 10; i++)
{
if ((i % 2) == 1)
{
stringSet.add("" + i);

Object[] fields = new Object[1];
fields[0] = "" + i;
HPCCRecord childRecord = new HPCCRecord(fields, childDatasetElemFD[0]);
childDataSet.add(childRecord);
}
else
{
stringSet.add(null);
childDataSet.add(null);
}
}

List<HPCCRecord> records = new ArrayList<HPCCRecord>();
for (int i = 0; i < 10; i++)
{

Object[] fields = new Object[3];
fields[0] = stringSet;
fields[1] = childDataSet;

if ((i % 2) == 1)
{
fields[2] = generateRandomString(128).getBytes();
}
else
{
fields[2] = null;
}

HPCCRecord record = new HPCCRecord(fields, recordDef);
records.add(record);
}
writeFile(records, "null::element::test", recordDef, connTO);
}

@Test
public void getMetadataTest() throws Exception
{
Expand Down Expand Up @@ -915,7 +980,7 @@ public void protocolVersionTest()
HPCCWsDFUClient dfuClient = wsclient.getWsDFUClient();

HpccRemoteFileReader<HPCCRecord> fileReader = null;
try
try
{
HPCCFile file = new HPCCFile("benchmark::integer::20kb", connString , hpccUser, hpccPass);
DataPartition[] fileParts = file.getFileParts();
Expand All @@ -928,7 +993,7 @@ public void protocolVersionTest()
{
Assert.fail("Exception while setting up protocol test: " + e.getMessage());
}

Version remoteVersion = dfuClient.getTargetHPCCBuildVersion();

if (remoteVersion.isEqualOrNewerThan(newProtocolVersion))
Expand Down

0 comments on commit 69444f3

Please sign in to comment.