233763 - Delete Dialog 64 char bug

Status: RESOLVED FIXED

(SeaMonkey :: Installer, defect)

Description

[Alexander Opitz]

If in config.ini on section [LegacyCheck0] you have a message which have a space
on a 64 char, the installer chows the same content on all following lines.

Maybe another bug is, that on lines with 64 chars the next chars aren't printed
to the Dialog.

An example is the from me mentioned new string in #69153

Comment 1

[Alexander Opitz]

Attachment: patch diff -u8p (trunk)

Seems easiest patch to a very strang if condition

Comment 2

[Andrew Schultz]

Comment on attachment 141115 [details] [diff] [review]
patch diff -u8p (trunk)

>                   msgChunk[msgChunkPtr - msgChunk] = 0;

wouldn't this be happier as

  *msgChunkPtr = '\0';

>-                  msgPtr = msgPtr + (msgChunkPtr - msgChunk + 1);
>+                  msgPtr += (msgChunkPtr - msgChunk + 1);
>+              } else
>+              {
>+                msgPtr += 64;

fix indenting (tabs?)
also the formatting style used in the linux installer is:

      code;
   }
   else
   {
      code;

Comment 3

[Alexander Opitz]

Attachment: patch v2

> fix indenting (tabs?)
OK, done

>also the formatting style ...
OK, done ... I forgett it.

>wouldn't this be happier as ...
Yes, looks nicer ... the line wasn't in question yesterday.

Thanks Andrew

Comment 4

[chris hofmann]

deveditz, can you super review?

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 141144 [details] [diff] [review]
patch v2

I'd prefer cleaning up the routine as long as we're touching it
(e.g.  replace magic numbers with consts or sizeof, set numlines
to min(numlines,20) rather than double testing each time through
the loop, move the constant strlen() out of the loop) but it's
not strictly necessary and might get in the way of the attempt
to slip this fix into 1.7a

In addition to the mentioned symptoms, if a line didn't have any spaces
the old code would write nulls to address 00000000

>               strncpy(msgChunk, msgPtr, 64);
>
>               // pad by a line but don't allow overflow
>               if (msgPtr > msg + strlen(msg))
>                   break;
> 

msgPtr might point to data past the allocated buffer. We break out of
the loop before we use junk, but if that part of the heap happens
to be allocated to another process we could crash.

Move the if...break to the end of the loop after msgPtr is incremented
(or add it to the for() condition) rather than checking after it's used.

Otherwise looks good.

Comment 6

[Alexander Opitz]

Attachment: patch v3

Daniel I hope I've understand you correct :-)

I've removed the numLines, cause it works very stupid if you have many words
with much chars and it makes no sence.

The strlen(msg) is moved out and the for loop have the msgPtr condition.

And I hope I've understand the 'replace magic numbers' correct.

Comment 7

[Alexander Opitz]

Attachment: patch v3.1

small fix, thx StefanB

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 141230 [details] [diff] [review]
patch v3.1

>-    char *msg = NULL, *msgPtr = NULL, *msgChunkPtr = NULL;
>-    char msgChunk[65];
>+    char msgChunk[MAXCHARS+1];
>+    char msg[1024];

Thanks for the magic number cleanup.

msg still has to be a char* because you're getting a pointer
to a string in a resource structure.

>-          // wrap message at 64 columns, and truncate at 20 rows

This comment is valuable in explaining what the loop is trying to do.
You could replace the raw numbers with your constant names, but we
still want a comment here.

Ah, I see a later comment about stopping at MAXLINES rows. I still
think you want to mention that we're trying to wrap lines, otherwise
your new comment raises the question "stop doing WHAT at maxlines?"

>+          numChars = strlen(msg);
>+          // stop at MAXLINES rows or stop after last char is reached
>+          for (i = 0; i < MAXLINES && msgPtr <= msg + numChars; i++)

Since you don't use numChars anywhere else it'd be slightly better
with a char* msgEnd = msgPtr + strlen(msg), then compare msgPtr
and msgEnd in the loop. Ok, so it doesn't *really* help performance
here over a max of 20 loops, but if you get in good habits then
you'll do the right thing when it does matter.

Either way I think you want '<' not '<=' though the extra blank line
when they're equal won't kill anyone.

Just to be clear, I'm rejecting this patch because of the incorrect
"char msg[1024];". The rest expresses my (strong) preferences but
are more matters of style.

Comment 9

[Alexander Opitz]

Attachment: patch v3.2

Excuse my fault Daniel :-)

>+    char msg[1024];

This is from another patch (which is on the blokked bug #69153)

> otherwise your new comment raises the question "stop doing WHAT at maxlines?"


I'd wrote it as comment in the first lin of the for ... but hope now it is more
clean.

> with a char* msgEnd = msgPtr + strlen(msg)

done

> you want '<' not '<=' though 

done

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 141266 [details] [diff] [review]
patch v3.2

>+                  msgPtr += 64;

The MAXCHARS changed back into a raw number again. I trust you to fix
that when you check in. I'll count Andrew's earlier review as the r=
and give you r/sr=dveditz

Who is going to check in? I don't have access to a Linux machine right
now and I don't feel good merging in patches without being able to
build and test them.

Comment 11

[Daniel Veditz [:dveditz]]

a=dveditz for 1.7a

Comment 12

[Alexander Opitz]

Attachment: patch v3.2.1 (see comment 10)

@Daniel

Thank you very much for all and excuse me little broken patches I'm a bit
nervous at the moment.

Comment 13

[Daniel Veditz [:dveditz]]

fix checked in by timeless