SrainApp / srain

Modern IRC client written in GTK
https://srain.silverrainz.me/
Other
305 stars 34 forks source link

Add support for message-tags #337

Closed progval closed 2 years ago

progval commented 3 years ago

Not doing anything with it yet, though.

If that's fine with you, I have another PR ready, to use server-time

SilverRainZ commented 3 years ago

Sorry for replying so late, I will review it ASAP.

SilverRainZ commented 3 years ago

I think we should check the boundary of current_tag_{key,value}_ptr at every time we bump the value of them, Otherwise, we may overflow our stack when encountering malformed messages.

SilverRainZ commented 3 years ago

Use strtok is safer than ptr++.

progval commented 3 years ago

Use strtok is safer than ptr++.

How? The loop needs to look for both ;/= and .

SilverRainZ commented 3 years ago

Use strtok is safer than ptr++.

How? The loop needs to look for both ;/= and .

ptr = strtok(line, delim)(first call), The delim parameter of can be ; = as you want.

See also: https://www.tutorialspoint.com/c_standard_library/c_function_strtok.htm

progval commented 3 years ago

I think we should check the boundary of current_tag_{key,value}_ptr at every time we bump the value of them, Otherwise, we may overflow our stack when encountering malformed messages.

Oh, good point. What if I add this at the beginning of the loop instead?

if (p >= (tags_ptr + TAGS_SIZE_LIMIT)) {
    g_free(imsg->tags);
    goto bad;

As current_tag_{key,value}_ptr are incremented at most once per loop iteration, this should guarantee they don't overflow.

progval commented 3 years ago

ptr = strtok(line, delim)(first call), The delim parameter of can be ; = as you want.

But it can't be all of them at the same time; so I would need to call strtok twice and discard the largest result, this seems wasteful

SilverRainZ commented 3 years ago

Oh, good point. What if I add this at the beginning of the loop instead?

if (p >= (tags_ptr + TAGS_SIZE_LIMIT)) {
    g_free(imsg->tags);
    goto bad;

It is okay and would be better if you print a related error message using `ERR_FR.

As current_tag_{key,value}_ptr are incremented at most once per loop iteration, this should guarantee they don't overflow.

When escaping chars, ptrs are incremented twice?

SilverRainZ commented 3 years ago

But it can't be all of them at the same time; so I would need to call strtok twice and discard the largest result, this seems wasteful

Yes, so there are 2 ways:

  1. use strtok, the flow of your code may changed: we can "check all of them" in same time
  2. don't change the code, but check whehter *p == '\0' every time after bump it.
progval commented 3 years ago

It is okay and would be better if you print a related error message using `ERR_FR.

Sure

When escaping chars, ptrs are incremented twice?

p is incremented twice, but current_tag_{key,value}_ptr only once

SilverRainZ commented 3 years ago

It is okay and would be better if you print a related error message using `ERR_FR.

Sure

When escaping chars, ptrs are incremented twice?

p is incremented twice, but current_tag_{key,value}_ptr only once

Ahhh, yes, my fault.

progval commented 3 years ago

Done!

SilverRainZ commented 2 years ago

Sorry for delay again, I am quite busy with both my life and work.

I will test the function of this PR weekend, then evently it can be merged.