Skip to content

patternChangeTagTest reports a binary tag and an invalid request with the same *ccsid value #665

Description

@JoeNemo

patternChangeTagTest in c/zosfile.c writes -1 into *ccsid on its error
path, and writes CCSID_BINARY on the success path for type=BINARY:

if (!strcmp(strupcase(type), "BINARY")) {
  *pure = FALSE;
  if (lccsid != 0) {
    *ccsid = -1;                       /* invalid: binary plus a codeset */
    snprintf(message, message_length, "%s", "binary specified with codeset");
    return -1;
  }
  else {
    *ccsid = CCSID_BINARY;             /* valid: tag this file binary */
    return 0;
  }
}

CCSID_BINARY is (short)0xFFFF (h/charsets.h:51), so assigning it to an
int sign-extends to -1. The two branches leave the caller holding the same
value for "tag this file binary" and "this request is malformed".

This is latent, not a live failure

Both callers happen to be insulated from it, by two different accidents:

  • c/httpfileservice.c:680 (directoryChangeTagAndRespond) uses the function
    purely as a validator. It checks the return code, discards ccsid, and hands
    type/codepage to directoryChangeTagRecursive, which recomputes.
  • c/zosfile.c:1713 (patternChangeTagCheck) does use the value, passing it to
    fileChangeTagPure, which stores it in attributes.fileTagCCSID -- an
    unsigned int ...:16 bitfield (h/unixfile.h:272). The -1 truncates back
    to 0xFFFF, which is the correct binary tag, so the right thing reaches
    BPXCHR.

So nothing is broken today. What is broken is the contract: an int out
parameter that cannot distinguish a legitimate value from a failure, held
harmless only by a 16-bit truncation two calls away and by one caller ignoring
the output entirely. Any future caller that inspects *ccsid -- reasonably, to
log it, to branch on binary-versus-text, or to pass it somewhere wider than 16
bits -- inherits the collision.

Suggested fix

Cast at the assignment, as parseEncodingValue in c/charsets.c:175 already
does:

*ccsid = (unsigned short)CCSID_BINARY;   /* 65535, not -1 */

CCSID_BINARY appears in only five places tree-wide and is never the right-hand
side of a comparison, so widening the stored value has no other reader. The
16-bit field still receives 0xFFFF, so the z/OS tag written is unchanged.

The identical collision in zss/c/unixFileService.c was fixed this way in
zowe/zss#825, where it had stopped being latent: once parseEncodingValue
accepted binary as an ordinary encoding name, source=binary&target=UTF-8
reached the caller carrying 65535 while source=binary&target=binary carried
-1 -- the same sentinel in two representations, one of which the caller reads
as a parse failure. Fixing it here keeps the two files consistent before
something similar surfaces.

Also worth a look while in this function

The fallthrough at the end accepts an unrecognised type outright:

*pure = FALSE;
*ccsid = lccsid;
return 0;

type=BANANA returns success with whatever findCcsidId(codepage) produced,
including -1 for an unknown codepage. Only the four spelled-out types
(BINARY, TEXT, DELETE/UNTAGGED, MIXED) look validated; anything else
is waved through. That is a separate change and may be deliberate -- flagging
it rather than folding it in.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions