If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Delete Dialog 64 char bug

RESOLVED FIXED

Status

SeaMonkey
Installer
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: Alexander Opitz, Assigned: Alexander Opitz)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

14 years ago
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
(Assignee)

Comment 1

14 years ago
Created attachment 141115 [details] [diff] [review]
patch diff -u8p (trunk)

Seems easiest patch to a very strang if condition

Comment 2

14 years ago
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;
(Assignee)

Comment 3

14 years ago
Created attachment 141144 [details] [diff] [review]
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
(Assignee)

Updated

14 years ago
Attachment #141115 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #141144 - Flags: review?(bsmedberg)
Attachment #141144 - Flags: approval1.7a?

Comment 4

14 years ago
deveditz, can you super review?
Attachment #141144 - Flags: superreview?(dveditz+bmo)
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.
Attachment #141144 - Flags: superreview?(dveditz+bmo) → superreview-
(Assignee)

Comment 6

14 years ago
Created attachment 141228 [details] [diff] [review]
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.
(Assignee)

Updated

14 years ago
Attachment #141144 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #141228 - Flags: superreview?(dveditz+bmo)
(Assignee)

Comment 7

14 years ago
Created attachment 141230 [details] [diff] [review]
patch v3.1

small fix, thx StefanB
Attachment #141228 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #141230 - Flags: superreview?(dveditz+bmo)
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.
Attachment #141230 - Flags: superreview?(dveditz+bmo) → superreview-
Attachment #141228 - Flags: superreview?(dveditz+bmo)
(Assignee)

Comment 9

14 years ago
Created attachment 141266 [details] [diff] [review]
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
(Assignee)

Updated

14 years ago
Attachment #141230 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #141266 - Flags: superreview?(dveditz+bmo)
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.
Attachment #141266 - Flags: superreview?(dveditz+bmo)
Attachment #141266 - Flags: superreview+
Attachment #141266 - Flags: review+
Attachment #141266 - Flags: approval1.7a+
a=dveditz for 1.7a
(Assignee)

Comment 12

14 years ago
Created attachment 141292 [details] [diff] [review]
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.
(Assignee)

Updated

14 years ago
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
fix checked in by timeless

Updated

14 years ago
Attachment #141144 - Flags: review?(bsmedberg)
Attachment #141144 - Flags: approval1.7a?
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.