Problem mit Hex2bix. (cypress Tool)

[Thomas (Gast)]

Ich benutze das Tool schon sehr lange und kenne die Aufrufparameter. 
Unter XP funktioniert es immer noch auch in einer VM. Auf einem Win10 
System geht es leider nicht.
Das Tool würde ursprünglich mit MSC 6 übersetzt. Auch ein Neuübersetzung 
wie von cypress in einem Thread empfohlen hat nicht weitergeholfen.
Da ich mir das Fehlverhalten (es kommt ein Fehler bezüglich der 
Speicherzugriffe) nicht so recht erklären konnte habe ich den Debugger 
angeworfen und das näher angeschaut.

Letztendlich könnte ich das Problem auf malloc bei den Filenamen 
eingrenzen.
Im Code wird per malloc Speicher für die Filenamen angefordert

Outfilename=(char*)malloc(strlen(argv[i]+EXT_LENGTH));
strcopy(Outfilename,argv[i];

soweit so gut oder auch nicht ich kann da erst Mal nichts falsches 
erkennen und wie gesagt unter XP geht ja auch alles wie erwartet.
Wenn ich nun den Speicher nicht per malloc anfordern sondern die 
Filenamen als globale Variablen mit char[256] definiere funktioniert das 
Tool auch unter W10.

Nun zur eigentlichen Frage:
was passiert unter W10?  Wieso arbeitet malloc jetzt auf einmal anders?
Ist das vielleicht ein Emulation Problem von WOW64?
Allgemein gefragt :
Wie kann ich sowas für mich verhindern?

Noch ein Hinweis. Das Tool hat noch ein Problem mit Intel Hex 
Checksummen die werden nicht korrekt behandelt. Das ist aber eine andere 
Baustelle und sehr einfach zu fixen. In alten Versionen von Hex2bix 
wurde die checksum Prüfung einfach auskommentiert

falls von Interesse hier meine Aufrufparameter
..\..\tools\hex2bix -IR -O .\obj\outf.eep .\obj\infile.hex

Thomas

[mh (Gast)]

Thomas schrieb:

Outfilename=(char*)malloc(strlen(argv[i]+EXT_LENGTH));

Sicher dass es nicht

Outfilename=(char*)malloc(strlen(argv[i])+EXT_LENGTH);

sein sollte?

[Thomas (Gast)]

Ja ich bin mir sicher und das ist wohl auch das Problem .... Manchmal 
sieht man halt den Wald vor lauter Bäumen nicht. Der Code ist ja im 
Anhang.
Die Frage bleibt trotzdem warum geht sowas unter XP?
 Danke für den Hinweis.
Thomas

[Kaj (Gast)]

Ach du... das sollte man aber patchen...

[...]

case 'O':
  i++;
  OutFilename = (char *)malloc(strlen(argv[i]+EXT_LENGTH+1));
  strcpy(OutFilename, argv[i]);
  cont = FALSE;
  break;

[...]

if (OutFilename == NULL)
{
  root_length = strcspn(InFilename,".");
  OutFilename = (char *)malloc(root_length+EXT_LENGTH);
  memcpy(OutFilename,InFilename,root_length);
  OutFilename[root_length] = 0;
  strcat(OutFilename,Extension[OutFileType]);
}

[...]

Image = (BYTE *)malloc(MEMORY_BUFFER_SIZE);
ImageMap = (BYTE *)malloc(MEMORY_BUFFER_SIZE+2);
memset(Image,MEM_FILL,MEMORY_BUFFER_SIZE);
memset(ImageMap,FALSE,MEMORY_BUFFER_SIZE);

[...]

Soweit ich das beim ueberfliegen sehe, wird nie geprueft, ob man 
ueberhaupt speicher bekommen hat. Da gehoert zu jedem malloc noch ein

if (ptr == NULL) {
  // FEHLER! Mach was sinnvolles, z.B. Fehlerbehandlung...
}

[Dr. Sommer (Gast)]

Thomas schrieb:
> Die Frage bleibt trotzdem warum geht sowas unter XP?

Vielleicht gibt der Kernel unter XP größere Adressbereiche zurück sodass 
der dann folgende Buffer Overflow Fehler unentdeckt bleibt. So kann man 
sich natürlich beliebige (Sicherheits-)Probleme einfangen. Es "geht" 
also nur zufällig. Also froh sein dass Win10 hier strenger ist und 
solche Fehler auffallen...

[Kaj (Gast)]

Und sowas

memset(ImageMap,FALSE,MEMORY_BUFFER_SIZE);
                ^^^^^

ist auch ganz dolle pfui! Wenn man den Speicher mit 0 fuellen will, dann 
sollte man auch hinschreiben, dass man 0 meint, und nicht 'false'...

Der ganze Code sollte mal ueberarbeitet werden. Ist ja gruselig...

[Dr. Sommer (Gast)]

Kaj schrieb:
> Der ganze Code sollte mal ueberarbeitet werden. Ist ja gruselig...

So ist C doch immer - gehört zum guten Ton! Gute alte 
Programmier-Wertarbeit :-)

[Thomas (Gast)]

Das der Code nicht so dolle ist war mir schon klar. Um ehrlich zu sein 
habe ich mir das nie angeschaut da ja alles bisher funktioniert hat. Ich 
habe halt immer die exe von Cypress verwendet.
Das ganze ist mir ja erst aufgefallen als ich die Entwicklermaschine 
getauscht hatte.
Ich finde das ganze schon ziemlich interessant. Hätte cypress nicht den 
Quellcode offengelegt, hätte ich das Tool selber nachprogrammieren 
müssen.

Meine Frage wie ich das für mich verhindern kann hat sich aber erledigt.

Thomas

[Kaj (Gast)]

Auch das hier:

    if (compressIIC)
        while((ImageMap[addr] || ImageMap[addr+1] || ImageMap[addr+2]) && (addr < MemSize) && (i++ < 0x3ff))
            ++addr;
    else
        while(ImageMap[addr] && (addr < MemSize) && (i++ < 0x3ff))
            ++addr;

Was CppCheck dazu sagt:

Summary: Array index 'addr' is used before limits check.

Message: Defensive programming:
The variable 'addr' is used as an array index before it is checked that
is within limits. This can mean that the array might be accessed out of
bounds. Reorder conditions such as '(a[i] && i < 10)' to '(i < 10 && a[i])'.
That way the array will not be accessed if the index is out of limits.

Das dieses Programm jemals (ansatzweise) korrekt funktioniert hat grenzt 
an Zauberei...

[Thomas (Gast)]

Naja an dem Tool wurde sehr oft rumgeflickt. Ich habe aleine 4 Versionen 
davon und bin mir nicht sicher ab ich die aktuellste Version habe. Da 
ich nur eine Teilmenge der Optionen bisher genutzt habe waren mir 
Optionen wie das compressed iic bisher egal.
Bei Cypress ist da alles auf obsolet gestellt. Ich will gar nicht wissen 
wie viele Programme noch im Umlauf sind die ähnliche Probleme haben.
Thomas

[Rufus Τ. F. (rufus) (Moderator)]

Man kann Dateinamen in char-Arrays fester Länge unterbringen; dafür kann 
man das Macro MAX_PATH verwenden, das mit #include <windows.h> zur 
Verfügung steht.

Weder ist es für ein paar Dateinamen nötig, mit malloc zu arbeiten, noch 
ist es nötig, hier ein paar Bytes zu sparen und weniger als MAX_PATH 
anzufordern.

Um windows.h einbinden zu können, sollte man allerdings das enum "BOOL" 
entfernen, sonst dürfte es mit den enum-Werten TRUE und FALSE zu 
Konflikten kommen.

[Dr. Sommer (Gast)]

Wenn man natürlich ein Programm unbedingt unportabel machen will... 
Außerdem können Pfade teilweise doch länger sein, siehe:

https://docs.microsoft.com/en-us/windows/desktop/fileio/naming-a-file

[Christian R. (supachris)]

Seltsam, das scheint dann aber nicht immer aufzutreten. Ich nutze das 
Tool auch um die EEPROM Files für den FX2 zu erzeugen, und das klappt 
auch unter Win 10 x64 einwandfrei.
Version müsste ich mal nachschauen...

[Dr. Sommer (Gast)]

Das ist das schöne an "Undefined behaviour" (hier: das Schreiben auf 
nicht allokierten Speicher dank zu kleiner Allokation) - mal 
funktioniert's, mal stürzt es ab, mal produziert es klammheimlich 
falsche Ergebnisse, mal formatiert es deine Festplatte. Deswegen ist es 
wichtig, korrekte Programme zu schreiben, und nicht nur solche, die 
"bei mir ja funktionieren".

[Thomas (Gast)]

Christian R. schrieb:
> Seltsam, das scheint dann aber nicht immer aufzutreten

Das Problem existiert sobald man mit -O das Ausgabefile erstellt. Bei 
der automatischen Erzeugung ist alles okay

Thomas

[Steffen R. (steffen_rose)]

Kaj schrieb:
> Und sowas
>

> memset(ImageMap,FALSE,MEMORY_BUFFER_SIZE);
>                 ^^^^^
>

> ist auch ganz dolle pfui! Wenn man den Speicher mit 0 fuellen will, dann
> sollte man auch hinschreiben, dass man 0 meint, und nicht 'false'...
>
> Der ganze Code sollte mal ueberarbeitet werden. Ist ja gruselig...

Da das Array ImageMap Booleans enthält, erscheint es mir richtig.
Es soll mit False initialisiert werden, nicht mit 0.

ImageMap[rec_addr+i] = TRUE;

und an anderer Stelle wird

addr < MemSize

als Schleifenabbruch genutzt und nicht zur Prüfung, dass der 
Eingabeparameter addr zu groß ist. Der wird durch den Kontext maximal 
addr == MemSize.
Nicht toll, aber dass es nur zufällig funktioniert... Na ja.

Kaj schrieb:
> Soweit ich das beim ueberfliegen sehe, wird nie geprueft, ob man
> ueberhaupt speicher bekommen hat.

Auch nicht toll. Aber wenn man die paar Byte nicht bekommt, dann hat man 
ein größeres Problem.

Natürlich ist der Source nicht das gelbe vom Ei. Aber der TO will es 
auch nur wieder zum Laufen bekommen. Nicht das Ziel aus den Augen 
verlieren.

[Thomas (Gast)]

Um das ganze abzuschließen im Anhang eine geänderte Version im Quelltext 
und compiliert
Sowie eine kleine Beschreibung dazu.
Es sind zwei Bugs behoben:
zum einen die Sache mit den Intel Hex checksum zum andern der Bug mit 
malloc bei -@O Filename.
Danke an alle die geholfen haben.

Thomas