BRS parsing failed

[RaphC]

Hi,

Thanks for your library which is a great AFP parser. I'm facing an issue about BRS parsing. I got an infinite loop certainly due to reserved offset (8-9) not handled. If I look at BDT which have a similar SF structure, I notice that reserved bites are ignored during the computation of triplets.

I thins the BRS_BeginResource should look like :

public class BRS_BeginResource extends StructuredFieldBaseName {
    byte[] reserved8_9 = {0x00,0x00};
    protected List triplets;

```
@Override
public void decodeAFP(byte[] sfData, int offset, int length, AFPParserConfiguration config) throws AFPParserException{
    super.decodeAFP(sfData, offset, length, config);

    int actualLength = getActualLength(sfData, offset, length);
    if(actualLength>8){
        reserved8_9 = new byte[]{sfData[offset +8], sfData[offset +9]};
    }else{
        reserved8_9 = null;
    }
    if(actualLength>10){
        triplets = TripletParser.parseTriplets(sfData, offset +10, actualLength -10, config);
    }else{
        triplets = null;
    }
}

@Override
public void writeAFP(OutputStream os, AFPParserConfiguration config) throws IOException{
    ByteArrayOutputStream baos = new ByteArrayOutputStream();
    baos.write(UtilCharacterEncoding.stringToByteArray(name, config.getAfpCharSet(), 8, Constants.EBCDIC_ID_FILLER));
    if(reserved8_9!=null) baos.write(reserved8_9);
    if(triplets!=null){
        for(Triplet triplet : triplets){
            triplet.writeAFP(baos, config);
        }
    }
    writeFullStructuredField(os, baos.toByteArray());
}   

public final List getTriplets() {
    return triplets;
}

public final void setTriplets(List triplets) {
    this.triplets = triplets;
}

public final void addTriplet(Triplet triplet){
    if(triplet==null) return;
    if(triplets==null) triplets = new ArrayList();
    triplets.add(triplet);
}

public final void removeTriplet(Triplet triplet){
    if(triplet==null || triplets==null) return;
    triplets.remove(triplet);
}
```

}

Regards

[michaelknigge]

Grab my fixes (see the pull request).... They fix the bug. Or checkout from my fork: https://github.com/michaelknigge/alpheusafpparser

Ah... but I've noticed my fix does not handle writeAFP()... I've just forgot about that... I'll fix this soon, too.

[RaphC]

Thanks @michaelknigge

[afpdev]

Hi RaphC & michaelknigge, thank you for your contributions. I merged the bugfixes from michael. For BRS I decided to go with RaphC's proposal and re-implemented BRS accordingly so the class reflects BRS's implementation correctly.

[michaelknigge]

Hi,

sorry, but I have to disagree. IMHO the proposal of RalphC is bad (sorry).... Let me explain why:

• You've created a new class (BRS_BeginResource) that is nearly 100% identically to the existing class BDT_BeginDocument (the only difference is the class name - duplicating code is always a bad idea).
• Furtermore the methods getTriplets(), setTriplets(), addTriplet() and removeTriplet() are now implemented three times (in BRS_BeginResource, BDT_BeginDocument and StructuredFieldBaseNameAndTriplets - as said, duplicating code is really really bad).
• Technically a BRS or BDT structured fields is a StructuredFieldBaseNameAndTriplets. But with your implementation they are not. Now a developer can not rely on getting ALL structured fields with a name and Triplets with something like "if (sf instanceof StructuredFieldBaseNameAndTriplets)". But this a common requirement. Just last week I had to implement a feature that removes all comment and time stamp Triplets from those structured fields. I personally would classify this as a bug. BRS_BeginResource and BDT_BeginDocument should both extend StructuredFieldBaseNameAndTriplets.
• after looking at your code I realized that the handling of the Triplets is IMHO completely broken (sorry). Again, let me explain why:
• getTriplets() can return null. From the users view this is bad because it is not documented and the user has to care about that (the user can not simple iterate with a construct like "for(Triplet t : sf.getTriplets())"
• Adding Triplets to a BRS or BDT structured field that had initially no structured field is broken (to be more precise, to a structured field that initially did not contain the two "reserved" bytes, which was valid several years ago and still occurrs in AFP files in wild today). Imagine you've read such a BDT or BRS structured field (that just ends after the name) and then set Triplets with setTriplets(). When writing out such a structured field the structured field is broken because you don not write the two "filler bytes".
• You do not respect the Coded Graphic Character Set Global Identifier Triplet if it was added by the user. Imagine you've read a structured field without such a Triplet, than add such a Triplet with a "ASCII-CPGID" and write it with writeAFP(). You still encode the name with the charset from the parser configuration.

Maybe tomorrow I'll find some time to throw an eye on it and fix (some of) the issues.

Bye, Michael