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 aban
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
Gerrit-Chang
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
Ge
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, visit
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);
> OK, I'm reviewing this function.
Hm, forget this:
while (msg_len > 0) {
is already always positive, so this can't be the reason f
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 have no idea if the code initially fetching the AMR size from RTP is doin
OK, I'm reviewing this function.
It seems this check i
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);
> OK, if the problem are truncated packet, then fix _snprintf() function to u
I have no idea if the code initially fetching the AMR s
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 _snprintf
Patch Set 3:
guys, please put this in prespective. If I look at the amount of time all of
you are spending on an osmux-private version of printing strings to a buffer,
this is not worth our time. In case of doubt, consider creating an inefficient
dynamic-allocating version or the like, which
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 payl
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 Nee
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);
> That's not checking the string output buffer, but the packet input buffer,
How come is the buffer getting full? AMR payload is ver
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 e
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 rea
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, s
Patch Set 3:
(7 comments)
There's a lot going on in this patch, and it practically screams for a small
unit test explicitly checking osmux_snprintf() behavior for "all" the edge
cases. That would verify beyond doubt that you're not just shooting in the dark
but actually fixing behavior.
http
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 spot
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
G
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
Gerrit
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: Ei
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 ex
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 https://gerrit.osmocom.org/3537
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 "n
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 of
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 ca
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?
http://git.netfilter.org/nftables/t
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.
Change
30 matches
Mail list logo
