I'm guessing this is IF_EVAL's fault

[devSin]

I'm thinking this is IF_EVAL making the assumption that I don't know what I'm doing:

Code: [Select]

COPY_EXISTING ~AATAQAH.CRE~ ~/Users/akay/Desktop~
  READ_LONG 0x6a effOff           // some offset to somewhere
  INSERT_BYTES effOff 0x3e8       // stick 1,000 bytes there
  effOff += 0x3e8                 // give us plenty of room
  READ_BYTE effOff - 0x330 myByte // in the really-real real world this would work
  PATCH_PRINT ~Whatever cause you're never gonna see it.~

Code: [Select]

$ weidu ~/Desktop/Setup.tp2 --yes --debug-value --debug-assign
[weidu] WeiDU version 183
[./Chitin.key] 182 BIFFs, 41794 resources
[./dialog.tlk] 74274 string entries
[WeiDU.log] parsed
[/Users/akay/Desktop/Setup.tp2] parsed
FIXPACK.TP2  0  0 Installed

Installing [Test-o]
Copying and patching 1 file ...
GET ~/Users/akay/Desktop~ = ~/Users/akay/Desktop~
GET ~AATAQAH.CRE~ = ~AATAQAH.CRE~
SET %SOURCE_DIRECTORY% = ~.~
SET %SOURCE_FILESPEC% = ~AATAQAH.CRE~
SET %SOURCE_FILE% = ~AATAQAH.CRE~
SET %SOURCE_RES% = ~AATAQAH~
SET %DEST_DIRECTORY% = ~/Users/akay~ // this isn't right -- Desktop is the DEST_DIRECTORY
SET %DEST_FILESPEC% = ~/Users/akay/Desktop~ // sigh
SET %DEST_FILE% = ~Desktop~ // I guess; should be AATAQAH.CRE (without me having to add SRCFILE.EXT to every dest list)
SET %DEST_RES% = ~Desktop~ // I guess; should be AATAQAH (without me having to add SRCFILE.EXT to every dest list)
SET %SOURCE_SIZE% = 1272
Value [0x6a] = 106
SET %effOff% = 0
Value [(effOff) - (0x330)] = -816
ERROR: illegal 1-byte read from offset -816 of 1272-byte file [AATAQAH.CRE]
Stopping installation because of error.

ERROR Installing [Test-o], rolling back to previous state
WARNING: [tempsave/0/UNINSTALL.0] is a 0 byte file
Will uninstall    0 files for [/Users/akay/Desktop/Setup.tp2] component 0.
Uninstalled       0 files for [/Users/akay/Desktop/Setup.tp2] component 0.
FIXPACK.TP2  0  0 Installed
ERROR: Failure("AATAQAH.CRE: read out of bounds")
Automatically Skipping [Test-o] because of error.
Saving This Log:
FIXPACK.TP2  0  0 Installed
[FIXPACK.TP2] parsed
[FIXPACK/ENGLISH/FIXPACK.TRB] has 2 translation stringsI only noticed this now that I removed the ELSE values from the READ_*s here. If the ELSE value is supplied, the initial read-all-the-READs flies through with a bunch of worthless, invalid values, and then the actual patch works as expected.

Before bounds-checking, I guess WeiDU was just walking out in the weeds (I wonder how many places IF_EVAL ever actually did anything worthwhile). Compatibility be damned: IF_EVAL must be stomped and burned to ash. It's time to excise this tumor.

[devSin]

Code: [Select]

let open_for_writing_internal backup filename binary =
  (if (backup) then backup_if_extant filename) ;
  if file_exists filename then (* if it already exists *)
    begin (* handle read-only files! *)
      try
        Unix.chmod filename 511 ; (* 511 = octal 0777 = a+rwx *)
      with e ->
        log_or_print "WARNING: chmod %s : %s\n" filename
          (Printexc.to_string e)
    end ;
  let out_chn = (if binary then open_out_bin else open_out) filename in
  out_chnCan we kill this message here? I've never understood why this one is try/catch:
 - The permissions of a file aren't checked before the chmod, so WeiDU is almost always just changing valid permissions (we won't be complaining about that today)
 - The chmod operation has no effect on anything: if chmod fails at this point it says nothing about writability nor the ability to continue installation (if you're going to fail, you'll fail on write, making the feedback redundant)
 - I hate sifting through these pointless messages in the log (shocking to come from the #1 complainer about logging and feedback, no?) for every existing file (especially since there has never been a case here where the chmod was necessary)

Maybe there's some Windows magic or something that makes this more useful, or some perfectly logical function or case that I've completely neglected. I've always commented the entire statement out locally, but I've been running out of the steam required to keep trolling through the WeiDU source; I thought I'd see if I can get an "amen" that this code (nice illusion that it is) isn't actually beneficial in all but the weirdest edge-cases (that person who doesn't have permission to write to a file, but can change access permissions willy-nilly).

Barring that, I'll submit this for consideration:

Code: [Select]

log_or_print "WARNING: chmod [%s] : %s\nWorry not, however. Miss Cleo sees success in your future.\n" filename (Printexc.to_string e)
Whatever. Sorry for being overly-critical today. All these earthquakes must be shaking my brains.

[devSin]

Code: [Select]

(* if (buff <> "") then *) begin
            (* List.iter (fun p -> process_patch1 src game buff p) plist ; *) // kill the read and kill IF_EVAL
            let ok_to_copy = List.fold_left (fun acc elt -> acc &&I'm currently doing this, but I want to make sure (without having to walk through the code here) that I'm not screwing anything else up. If this is just going to kill IF_EVAL, then I'm fine now (let everyone else suffer).

Code: [Select]

  let bounds_check_write idx size (str : string) =
    let len = String.length buff in
    let out_of_bounds = (idx < 0 || (idx + size) > len) in
    if out_of_bounds then begin
      log_and_print "ERROR: illegal %d-byte write (%s) offset %d of %d-byte file [%s]\n" size str idx len patch_filename ;
      failwith (patch_filename ^ ": write out of bounds")
    end ;
    ()As far as I can tell, there's never any value for str (the message will always read "size-byte write () offset idx of len-byte file [patch_filename]"). You always pass what here, but we never get to see it.

[weimer]

Grr. My reply to this was eaten by the ether.

It's time to excise this tumor.

You must gather signatures from all and sundry before I break backwards compatibility. It's an albatross, I know. bigg's refinements mod uses IF_EVAL, for one.

Your IF_EVAL lobotomy looks good, though. It's possible that someone could have written this mod, which will fail under your edit:

Code: [Select]

COPY foo bar
  WRITE_BYTE 0x10 (myvar+1) // looks like use-before-def but is not
  READ_BYTE 0x10 myvar


I thought I'd see if I can get an "amen" that this code (nice illusion that it is) isn't actually beneficial in all but the weirdest edge-cases (that person who doesn't have permission to write to a file, but can change access permissions willy-nilly).

Turns out that default distributions of the official ToB patch and Baldurdash left read-only files in override on windows machines. I know WeiDU is a bunch of cruft and it must like I add this stuff just for fun, but in fact 95% of it was added in response to user error reports or feature requests. On windows the chmod error message never triggers and the chmod does useful work (the filesystem allows the situation where you can make a file read-write but not write to it). I'll comment out the message for you.

As far as I can tell, there's never any value for str (the message will always read "size-byte write () offset idx of len-byte file [patch_filename]"). You always pass what here, but we never get to see it.

I disagree. On my machine this code:

Code: [Select]

COPY_EXISTING ~SW1H01.ITM~ ~override~
  SAY 0x9999 ~Imoen~

Yields this message:

Copying and patching 1 file ...
ERROR: illegal 4-byte write (Imoen) offset 39321 of 314-byte file SW1H01.ITM
ERROR: [SW1H01.ITM] -> [override] Patching Failed (COPY) (Failure("SW1H01.ITM: write out of bounds"))

You're probably not seeing it most of the time because for things like WRITE_LONG we interpret the 32-bit value as a four character string (which will print like 8-bit garbage).

[the bigg]

bigg's refinements mod uses IF_EVAL, for one.

My current build (not yet released) doesn't have IF_EVAL. And, blame Littiz on them, not me 
Finally, I believe there are mods which have an higher ratio of non-updateness risk than Refinements 

[devSin]

Grr. My reply to this was eaten by the ether.

The vagaries of the internet. Such fun. :-)

It's possible that someone could have written this mod

I'm not picking up on how that's not "use-before-def." Either myvar has been previously defined earlier, or it was intended that this value gets picked up by the initial read and works (unless you use myvar elsewhere, in which case the value is likely different than what you want). If you were pointing out the second behavior (as far as I'm aware, the first will still work fine, yes?), then it deserves to fail (they both do, IMO).

I know WeiDU is a bunch of cruft and it must like I add this stuff just for fun

I read the patch notes to see exactly when you added it before posting the message. I don't think it's cruft (or just a case where you thought this or that should do some pointless work), but I did want to acknowledge that I don't see this ever achieving anything on Mac OS X. Pointing out the fact that access positions aren't even checked before they're changed gave me a fall-back position (I know what I'm talking about, honest! :-)). Anyway, thanks for killing the warning message.

which will print like 8-bit garbage

I don't know. Terminal is pretty good about printing high-ASCII and other non-printing characters. I see now that PatchStrRef is passing a dialog.tlk string, so I'm not surprised that works (to save face, I'll point out that you're not actually writing "Imoen" there). Whatever; nothing to pay attention to here.

EDIT: yep, you were right. I must have been running afoul of stupid nul bytes. Sorry.