Pau Espin Pedrol has abandoned this change.
Change subject: osmux: Re-write osmux_snprintf
..
Abandoned
3 patches were merged fixing a few bits remaining to be improved after Pablos'
patches. This patch can as a result be
Patch Set 3:
> @Pau: ping?
I am aware of this patch. I'll close/drop it once I merge the 3-patch patchset
I submitted today for osmux_snprintf.
--
To view, visit https://gerrit.osmocom.org/3537
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Patch Set 3:
@Pau: ping?
--
To view, visit https://gerrit.osmocom.org/3537
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I695771d099833842db37a415b636035d17f1bba7
Gerrit-PatchSet: 3
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Patch Set 3:
That sounds good. Split it in two patches I would suggest.
Thanks!
--
To view, visit https://gerrit.osmocom.org/3537
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I695771d099833842db37a415b636035d17f1bba7
Gerrit-PatchSet:
Patch Set 3:
https://gerrit.osmocom.org/#/c/3830/ has been merged, so some changes in this
patch can be re-submitted again on top it:
- documentation for the function.
- Add the "osmuxh->ft == OSMUX_FT_VOICE_AMR &&" check.
--
To view, visit https://gerrit.osmocom.org/3537
To unsubscribe,
Patch Set 3:
(1 comment)
https://gerrit.osmocom.org/#/c/3537/3/src/osmux.c
File src/osmux.c:
Line 918: SNPRINTF_BUFFER_SIZE(ret, buf_offset, size);
> Are you asking about this exact "No room for OSMUX payload" error case?
OK, if the problem are truncated packet, then fix
Patch Set 3:
(1 comment)
https://gerrit.osmocom.org/#/c/3537/3/src/osmux.c
File src/osmux.c:
Line 918: SNPRINTF_BUFFER_SIZE(ret, buf_offset, size);
> I'm telling this because I suspect this is papering a problem somewhere els
Are you asking about this exact "No room for OSMUX
Patch Set 3:
(2 comments)
https://gerrit.osmocom.org/#/c/3537/3/src/osmux.c
File src/osmux.c:
Line 851: return ret; \
> I strongly object this coding style. Please no hidden return statements in
Return hidden is macros is not good. I'm with
Patch Set 3:
(4 comments)
https://gerrit.osmocom.org/#/c/3537/3/src/osmux.c
File src/osmux.c:
Line 849: #define SNPRINTF_BUFFER_SIZE(ret, buffer_offset, size)
\
> agreed; yet when first reading it, it helps to have doc for these non-obvio
Agree, I'll add a short description of
Patch Set 3: Code-Review-1
(5 comments)
https://gerrit.osmocom.org/#/c/3537/3/src/osmux.c
File src/osmux.c:
Line 849: #define SNPRINTF_BUFFER_SIZE(ret, buffer_offset, size)
\
> At least now there's one parameter left storing state, so easier than befor
agreed; yet when first
Patch Set 3:
(6 comments)
https://gerrit.osmocom.org/#/c/3537/3/src/osmux.c
File src/osmux.c:
Line 849: #define SNPRINTF_BUFFER_SIZE(ret, buffer_offset, size)
\
> phew, can we document the arguments? my head is spinning...
At least now there's one parameter left storing state,
Hello Harald Welte, Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/3537
to look at the new patch set (#2).
osmux: Re-write osmux_snprintf
After last buffer overflow fix to osmux_snprintf in
7cca0da1cc58bd589989684147ae3a0cd5819902, it was
Patch Set 1: Code-Review+2
--
To view, visit https://gerrit.osmocom.org/3537
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I695771d099833842db37a415b636035d17f1bba7
Gerrit-PatchSet: 1
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Patch Set 1: Code-Review+1
description fixes requested by holger missing
--
To view, visit https://gerrit.osmocom.org/3537
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I695771d099833842db37a415b636035d17f1bba7
Gerrit-PatchSet: 1
Patch Set 1:
> @Pau: You're not the only one getting confused with it, snprintf()
> is a mess, it's hard to deal with it, hence this macro that retains
> semantics that aims to simplify things... if you find any better,
> let me know I'd be happy to reuse it in my code moving forward :-).
>
Patch Set 1:
@Pau: You're not the only one getting confused with it, snprintf() is a mess,
it's hard to deal with it, hence this macro that retains semantics that aims to
simplify things... if you find any better, let me know I'd be happy to reuse it
in my code moving forward :-).
@Holger:
Patch Set 1:
(1 comment)
Hi sorry, it seems I wrote a comment a few days ago but I didn't "submit it".
https://gerrit.osmocom.org/#/c/3537/1/src/osmux.c
File src/osmux.c:
PS1, Line 849:
> I'm still trying to understand this update, not sure what corner case you'r
I first submited this patch
Patch Set 1:
> SOrry, I mean: If you want to retain snprintf semantics, ie. return
> the number of bytes that would have been written, then you need to
> keep 'len' and offset, because offset should not ever go over the
> boundary.
>
> But 'len' may indeed go over the boundary, since it
Patch Set 1:
> size is the original buffer size, number of bytes you can fit into
> the buffer you provide.
right. but isn't size + ret then very fishy? (in the nftable macro too). How
does the buffer grow? Shouldn't it be like a "constant"?
--
To view, visit
Patch Set 1:
SOrry, I mean: If you want to retain snprintf semantics, ie. return the number
of bytes that would have been written, then you need to keep 'len' and offset,
because offset should not ever go over the boundary.
But 'len' may indeed go over the boundary, since it expresses this
Patch Set 1:
size is the original buffer size, number of bytes you can fit into the buffer
you provide.
len is the number of byte that has been written into the buffer.
offset is where you should continue to append more bytes.
If you want to retain snprintf semantics, ie. return the number
Patch Set 1:
@pau: "last small changes" -> link to commit or change-id. Most people will not
use git log --date to find it. )
@Pablo: https://gerrit.osmocom.org/#/c/3521/. I think size, len and offset
could be presented in two variables (or change the name). E.g. when reading is
"size" the
Patch Set 1:
(1 comment)
https://gerrit.osmocom.org/#/c/3537/1/src/osmux.c
File src/osmux.c:
PS1, Line 849:
I'm still trying to understand this update, not sure what corner case you're
trying to catch.
Why this code below not just enough to address this?
Review at https://gerrit.osmocom.org/3537
osmux: Re-write osmux_snprintf
After last small changes, it was spotted that some cases may still be
able to make osmux_snprintf acces unexpected memory. This patch attemps
to try harder at fixing those issue.
See OS-#2443 for more information.
24 matches
Mail list logo
