Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for message-tags #337

Merged
merged 3 commits into from
Nov 21, 2021
Merged

Conversation

progval
Copy link
Member

@progval progval commented Sep 18, 2021

Not doing anything with it yet, though.

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

Not doing anything with it yet, though.
@SilverRainZ
Copy link
Member

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

@SilverRainZ
Copy link
Member

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.

src/sirc/sirc_parse.c Outdated Show resolved Hide resolved
src/sirc/sirc_parse.c Outdated Show resolved Hide resolved
@SilverRainZ
Copy link
Member

Use strtok is safer than ptr++.

@progval
Copy link
Member Author

progval commented Oct 16, 2021

Use strtok is safer than ptr++.

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

@SilverRainZ
Copy link
Member

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
Copy link
Member Author

progval commented Oct 16, 2021

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
Copy link
Member Author

progval commented Oct 16, 2021

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

progval commented Oct 16, 2021

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
Copy link
Member

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
Copy link
Member Author

progval commented Oct 24, 2021

Done!

src/sirc/sirc_parse.c Show resolved Hide resolved
src/sirc/sirc_parse.c Show resolved Hide resolved
@SilverRainZ
Copy link
Member

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.

@SilverRainZ SilverRainZ merged commit 3ba1c82 into SrainApp:master Nov 21, 2021
@SilverRainZ SilverRainZ removed this from the 1.3.1 milestone Nov 21, 2021
@progval progval deleted the message-tags branch November 21, 2021 09:35
@SilverRainZ SilverRainZ linked an issue Dec 6, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parsing message tags
2 participants